Simplify Makefile and allow for better incremental builds. #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| CHACHA_OBJS := $(addprefix objs/rand_urandom_chacha20/, \ | ||
| rand_urandom_chacha20.o chacha20.o) | ||
|
|
||
| $(CHACHAOBJS): src/rand_urandom_chacha20/rand_urandom_chacha20.h |
There was a problem hiding this comment.
I'm not too familiar with Makefile syntax. Do you intentionally have CHACHA_OBJS in one spot and CHACHAOBJS in another?
Also add -std=gnu11 option since some older compilers default to c99 which causes compilation to fail.
|
Good catch I fixed the commit. Unfortunately make assumes variables which are not set are empty and doesn't respond with an error or warning :(. That line is declaring the dependencies of the chacha object files. I didn't catch it because make compiles things fine without knowing the dependencies. It could have caused issues for incremental recompilation though. |
Caused by method_name not being freed if OQS_RAND_urandom_chacha20_ctx_new() fails. Also accounts for the fact that strdup can fail and return NULL.
mingtaoy
pushed a commit
to mingtaoy/liboqs
that referenced
this pull request
Sep 4, 2024
The FIPS202x2 implementation does not play well with ASAN. Specifically, when handling the trailing blocks that are not divisible by 8, the implementation uses an 8 byte load followed by a mask to process the trailing bytes of the input (https://github.com/open-quantum-safe/liboqs/blob/d93a431aaf9ac929f267901509e968a5727c053c/src/kem/kyber/oldpqclean_kyber512_aarch64/fips202x2.c#L475-L476) While this may work in practice, performing an 8 byte load on an input with fewer than 8 bytes remaining leads to UB (and an ASAN error) when the input size cannot accomodate such a load. A simple repro, built with `-fsanitize=address` shows this: ```c static void keypair(const char* alg) { OQS_KEM* kem = OQS_KEM_new(alg); unsigned char* public = malloc(kem->length_public_key); unsigned char* sec = malloc(kem->length_secret_key); kem->keypair(public, sec); free(sec); free(public); OQS_KEM_free(kem); } int main() { keypair(OQS_KEM_alg_kyber_512); keypair(OQS_KEM_alg_kyber_768); keypair(OQS_KEM_alg_kyber_1024); return 0; } ``` results in the following error: ``` ================================================================= ==613880==ERROR: AddressSanitizer: stack-buffer-overflow on address 0xfffff57a5160 at pc 0x0000004e9ef4 bp 0xfffff57a5030 sp 0xfffff57a5020 READ of size 8 at 0xfffff57a5160 thread T0 #0 0x4e9ef0 in keccakx2_absorb.constprop.1 (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4e9ef0) open-quantum-safe#1 0x4a608c in PQCLEAN_KYBER512_AARCH64_neon_kyber_shake128_absorb (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a608c) open-quantum-safe#2 0x4a51a8 in PQCLEAN_KYBER512_AARCH64_gen_matrix (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a51a8) open-quantum-safe#3 0x4a5360 in PQCLEAN_KYBER512_AARCH64_indcpa_keypair (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a5360) open-quantum-safe#4 0x45e460 in PQCLEAN_KYBER512_AARCH64_crypto_kem_keypair (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x45e460) open-quantum-safe#5 0x4075c0 in keypair (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4075c0) open-quantum-safe#6 0x405598 in main (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x405598) open-quantum-safe#7 0xfffcbbd772fc in __libc_start_call_main (/lib64/libc.so.6+0x272fc) open-quantum-safe#8 0xfffcbbd773d4 in __libc_start_main_impl (/lib64/libc.so.6+0x273d4) open-quantum-safe#9 0x40746c in _start (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x40746c) Address 0xfffff57a5160 is located in stack of thread T0 at offset 64 in frame #0 0x4a5efc in PQCLEAN_KYBER512_AARCH64_neon_kyber_shake128_absorb (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a5efc) This frame has 2 object(s): [32, 66) 'extseed1' (line 59) <== Memory access at offset 64 partially overflows this variable [112, 146) 'extseed2' (line 60) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4e9ef0) in keccakx2_absorb.constprop.1 Shadow bytes around the buggy address: 0x200ffeaf49d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf49e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf49f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x200ffeaf4a20: 00 00 00 00 f1 f1 f1 f1 00 00 00 00[02]f2 f2 f2 0x200ffeaf4a30: f2 f2 00 00 00 00 02 f3 f3 f3 f3 f3 00 00 00 00 0x200ffeaf4a40: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 0x200ffeaf4a50: f1 f1 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==613880==ABORTING ``` This PR sidesteps this issue by ensuring that `extseed1` and `extseed2` are sized to be divisible by 8, which avoids the problematic codepath. Other alternatives considered for this PR were to modify the `keccakx2_absorb` implementation (e.g. to memcpy the remaining data into a local 8 byte buffer first) -- however this approach inhibited inlining of the absorb implementation and resulted in a call to `memcpy` which could not be optimized out due to the variable-length `inlen`. Tested with `ninja run_tests` and ensured that ASAN repro no longer fires. Co-Authored-By: Kyle Nekritz <knekritz@meta.com>
mingtaoy
pushed a commit
to mingtaoy/liboqs
that referenced
this pull request
Sep 4, 2024
The FIPS202x2 implementation does not play well with ASAN. Specifically, when handling the trailing blocks that are not divisible by 8, the implementation uses an 8 byte load followed by a mask to process the trailing bytes of the input (https://github.com/open-quantum-safe/liboqs/blob/d93a431aaf9ac929f267901509e968a5727c053c/src/kem/kyber/oldpqclean_kyber512_aarch64/fips202x2.c#L475-L476) While this may work in practice, performing an 8 byte load on an input with fewer than 8 bytes remaining leads to UB (and an ASAN error) when the input size cannot accomodate such a load. A simple repro, built with `-fsanitize=address` shows this: ```c static void keypair(const char* alg) { OQS_KEM* kem = OQS_KEM_new(alg); unsigned char* public = malloc(kem->length_public_key); unsigned char* sec = malloc(kem->length_secret_key); kem->keypair(public, sec); free(sec); free(public); OQS_KEM_free(kem); } int main() { keypair(OQS_KEM_alg_kyber_512); keypair(OQS_KEM_alg_kyber_768); keypair(OQS_KEM_alg_kyber_1024); return 0; } ``` results in the following error: ``` ================================================================= ==613880==ERROR: AddressSanitizer: stack-buffer-overflow on address 0xfffff57a5160 at pc 0x0000004e9ef4 bp 0xfffff57a5030 sp 0xfffff57a5020 READ of size 8 at 0xfffff57a5160 thread T0 #0 0x4e9ef0 in keccakx2_absorb.constprop.1 (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4e9ef0) open-quantum-safe#1 0x4a608c in PQCLEAN_KYBER512_AARCH64_neon_kyber_shake128_absorb (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a608c) open-quantum-safe#2 0x4a51a8 in PQCLEAN_KYBER512_AARCH64_gen_matrix (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a51a8) open-quantum-safe#3 0x4a5360 in PQCLEAN_KYBER512_AARCH64_indcpa_keypair (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a5360) open-quantum-safe#4 0x45e460 in PQCLEAN_KYBER512_AARCH64_crypto_kem_keypair (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x45e460) open-quantum-safe#5 0x4075c0 in keypair (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4075c0) open-quantum-safe#6 0x405598 in main (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x405598) open-quantum-safe#7 0xfffcbbd772fc in __libc_start_call_main (/lib64/libc.so.6+0x272fc) open-quantum-safe#8 0xfffcbbd773d4 in __libc_start_main_impl (/lib64/libc.so.6+0x273d4) open-quantum-safe#9 0x40746c in _start (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x40746c) Address 0xfffff57a5160 is located in stack of thread T0 at offset 64 in frame #0 0x4a5efc in PQCLEAN_KYBER512_AARCH64_neon_kyber_shake128_absorb (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a5efc) This frame has 2 object(s): [32, 66) 'extseed1' (line 59) <== Memory access at offset 64 partially overflows this variable [112, 146) 'extseed2' (line 60) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow (/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4e9ef0) in keccakx2_absorb.constprop.1 Shadow bytes around the buggy address: 0x200ffeaf49d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf49e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf49f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x200ffeaf4a20: 00 00 00 00 f1 f1 f1 f1 00 00 00 00[02]f2 f2 f2 0x200ffeaf4a30: f2 f2 00 00 00 00 02 f3 f3 f3 f3 f3 00 00 00 00 0x200ffeaf4a40: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 0x200ffeaf4a50: f1 f1 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200ffeaf4a70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==613880==ABORTING ``` This PR sidesteps this issue by ensuring that `extseed1` and `extseed2` are sized to be divisible by 8, which avoids the problematic codepath. Other alternatives considered for this PR were to modify the `keccakx2_absorb` implementation (e.g. to memcpy the remaining data into a local 8 byte buffer first) -- however this approach inhibited inlining of the absorb implementation and resulted in a call to `memcpy` which could not be optimized out due to the variable-length `inlen`. Tested with `ninja run_tests` and ensured that ASAN repro no longer fires. Co-Authored-By: Kyle Nekritz <knekritz@meta.com> Signed-off-by: Mingtao Yang <mingtao@meta.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also add -std=gnu11 option since some older compilers default to c99 which causes compilation to fail.
I also turned on some warnings.