Add more value barriers to avoid compiler induced side channels #4395
Conversation
|
Despite the claims in the paper, the authors never contacted me about this 😞 I guess we're lucky I happened to see it on a Saturday morning just before the release. |
randombit
force-pushed
the
jack/more-value-barrier
branch
2 times, most recently
from
ef3d203
to
b7bd953
Compare
| const uint64_t X0MASK = (ALL_BITS + (X[0] >> 63)) ^ ALL_BITS; | ||
| const uint64_t X1MASK = (ALL_BITS + (X[1] >> 63)) ^ ALL_BITS; | ||
| const auto X0MASK = CT::Mask<uint64_t>::expand_top_bit(X[0]); | ||
| const auto X1MASK = CT::Mask<uint64_t>::expand_top_bit(X[1]); |
There was a problem hiding this comment.
This area was not flagged in the paper but seems an obvious target for Clang's range analysis
|
I wonder if it is possible to come up with (say) a custom clang-tidy rule that flags such locations for potential range analysis optimizations or the like. |
|
Something that flags the obvious cases would be nice to have. One search, that would have caught the ghash issue already, is |
|
I feel we should build a nightly test matrix that runs the valgrind suite on a wider spectrum of compilers, targets and optimization levels, in the hope that this increases the signal of such vulnerabilities. Or at least see if we can build regression tests based on our existing tools. |
|
We're practically limited by what architectures are natively supported; running our tests inside qemu inside valgrind would be ... something. But adding even just x86-32 and aarch64 to the valgrind runs would improve coverage quite a bit. |
src/lib/pubkey/x25519/donna.cpp
Outdated
| inline void swap_conditional(uint64_t a[5], uint64_t b[5], uint64_t c[5], uint64_t d[5], uint64_t iswap) { | ||
| const uint64_t swap = 0 - iswap; | ||
| const auto swap = CT::Mask<uint64_t>::expand(iswap); |
There was a problem hiding this comment.
Nit-picky, but should't swap_conditional take a CT::Choice for iswap here?
There was a problem hiding this comment.
A Choice would logically make sense but we'd then have to convert it to a Mask<uint64_t> to perform the swap so I changed iswap arg to a Mask<uint64_t>
src/lib/pubkey/x25519/donna.cpp
Outdated
| const uint64_t bit0 = CT::value_barrier((n[31 - i] >> 7) & 1); | ||
| const uint64_t bit1 = CT::value_barrier((n[31 - i] >> 6) & 1); | ||
| const uint64_t bit2 = CT::value_barrier((n[31 - i] >> 5) & 1); | ||
| const uint64_t bit3 = CT::value_barrier((n[31 - i] >> 4) & 1); | ||
| const uint64_t bit4 = CT::value_barrier((n[31 - i] >> 3) & 1); | ||
| const uint64_t bit5 = CT::value_barrier((n[31 - i] >> 2) & 1); | ||
| const uint64_t bit6 = CT::value_barrier((n[31 - i] >> 1) & 1); | ||
| const uint64_t bit7 = CT::value_barrier((n[31 - i] >> 0) & 1); |
There was a problem hiding this comment.
Perhaps worth adding a higher-level CT::is_even() for this (if it doesn't exist; I didn't check), to make this:
const CT::Choice bit0 = CT::is_odd(n[31 - i] >> 7). This would also play nicely with my other comment.
There was a problem hiding this comment.
Already existing was CT::Mask::expand_bit which I used here.
The paper https://arxiv.org/pdf/2410.13489 claims that on specific architectures Clang and GCC may introduce jumps here. The donna128 issues only affect 32-bit processors, which explains why we would not see it in the x86-64 valgrind runs. The GHASH leak would seem to be generic but the authors only observed it on RISC-V.
No indications that Clang is currently converting this to conditional jumps, but this construct seems prone to such problems.
There has been no indication that any compiler is converting this code to assembly that uses conditional jumps, but it has the style of code that would be vulnerable to Clang's range analysis optimizations.
randombit
force-pushed
the
jack/more-value-barrier
branch
from
5ca26fb
to
8771ae0
Compare
The paper https://arxiv.org/pdf/2410.13489 claims that on specific architectures Clang and GCC may introduce jumps here. The donna128 issues only affect 32-bit processors, which explains why we would not see it in the x86-64 valgrind runs.
The GHASH leak would seem to be generic but the authors only observed it on RISC-V.