Fix BIKE constant-time errors #1632
Conversation
|
Following up on discussion in today's OQS meeting: I am running into a snag when attempting to resolve the constant-time error raised for this line of code: My initial thought was that the error was raised because of the use of "<" operator for comparison. My "quick fix" of using a bitshift to check the If somebody more familiar with the inner workings of BIKE could take a look and offer input on
Feedback on the other constant-time errors (and my proposed fixes) would also be very welcome! |
|
Tagging @crockeea who kindly offered to pass this along to somebody who knows about BIKE. |
| for (size_t i = 0; i < 8; ++i) { | ||
| // use a mask for secret-independent memory access | ||
| mask = LSB3(a0) - i; | ||
| l ^= (u[i] & (uint64_t)(0 - (1 - ((uint64_t)(mask | (0 - mask)) >> 63)))); |
There was a problem hiding this comment.
maybe we can extract the mask computation to a function in utilities.h, something like:
// Returns 0xfff..ff if a == b, otherwise returns 0.
uint64_t secure_cmpeq64_mask(uint64_t a, uint64_t b) {
uint64_t tmp = a - b;
return (uint64_t)(0 - (1 - ((uint64_t)(tmp | (0 - tmp)) >> 63))));
}
and then use it here and in other places below like this:
l ^= u[i] & secure_cmpeq64_mask(LSB3(a0), i);
It would constitute secret-dependent behaviour if the condition could ever fail. however, we are calling the function here: https://github.com/open-quantum-safe/liboqs/blob/main/src/kem/bike/additional_r4/decode_portable.c#L62 where we explicitly reduce the input mod 64.
we should just remove the assert, it's pointless because we reduce the argument mod 64 before calling the function. |
|
Thanks very much for the quick review @dkostic. I've made your suggested changes and confirmed that the constant-time tests now pass for BIKE on my machine (the CI run is a weekly scheduled job targeting Would it be helpful if I submit a corresponding PR to the awslabs/bike-kem repo? |
That'd be awesome and helpful! |
) This PR addresses two issues which were caught by liboqs automated constant-time testing. One of these involved a number of secret-dependent array accesses in gf2x_mul_base_portable. This can be fixed by putting the array accesses inside short loops and using a mask. The second involved an unnecessary static_assert statement; the fix was to remove it. See also the analogous liboqs PR: open-quantum-safe/liboqs#1632
|
This has now been fixed upstream as well: awslabs/bike-kem@96aeca0 |
* Document BIKE CT issues * Document / fix BIKE constant-time errors * Revert "< 64" comparison change * Add and use secure_cmpeq64_mask function * Remove unnecessary static_assert
I have reviewed the BIKE constant-time test failures and identified the offending lines of code. They are summarized below.
liboqs/src/kem/bike/additional_r4/sampling_portable.c
Line 44 in 8449e54
This appears to be triggered by the array access
pos_qw[j]. After reviewing the function's code, I believe this to be a false positive and have documented it as a "pass". There's an existing pass for the AVX2 version of this function.liboqs/src/kem/bike/additional_r4/decode_portable.c
Line 40 in 8449e54
The issue here seems to be the "<" operator. I changed this to
!(bits >> 6)and it now passes (on my machine).liboqs/src/kem/bike/additional_r4/gf2x_mul_base_portable.c
Line 44 in 8449e54
liboqs/src/kem/bike/additional_r4/gf2x_mul_base_portable.c
Line 45 in 8449e54
liboqs/src/kem/bike/additional_r4/gf2x_mul_base_portable.c
Line 50 in 8449e54
liboqs/src/kem/bike/additional_r4/gf2x_mul_base_portable.c
Line 51 in 8449e54
These array accesses depend on the value of
a0, which is one of the function arguments. Hence, I believe this to be (possible) secret-dependent behaviour. I added short loops to perform the array access in a secret-independent fashion, as is done in HQC.After these changes, the constant-time tests pass for all variants of BIKE on my machine.
Fixes #1624.