Prevent from revealing a length of the secrect#21
Conversation
|
I've corrected a typo in the documentation. |
|
This matches CVE-2024-13939 / https://lists.security.metacpan.org/cve-announce/msg/28284732/ :) EDIT: this was already written in the middle of the OP. |
|
Hi sorry for the delay! Thank you for this patch, but I'm hesitant to attempt to guarantee behaviour with variable-length inputs. As you say, we can't know in advance what compilers and architecture people will use (especially future compilers!). As stated in the documentation, this module only supports constant-time verification of known-size values. In practice this is never a restriction, because variable-length values can be hashed, and those hashes compared in constant-time. |
|
Would you accept a patch which would change the code, but kept the documentation intact? |
|
FWIW, In Debian we did apply the patch as proposed by @ppisar in the unstable and testing distribution already and as far I see we did not got negative feedback. (OTOH, I think to not diverge with upstream we might need to revert the patch if there is diagreement that it is the right thing to do) /cc @gregoa |
|
I also pondered how to write a deterministic test for this fix but gave up. If the test were based on statistical methods, it would be improbable, but still possible that the test would randomly fail, or worse that it would randomly pass regardless of efficiency of the fix. I know that some software projects use statistical tests, but I, as a package maintainer, don't like them because they exhibit random failures. Maybe the correct approach is drawing a line of the probability of a false negative/positive low enough that different problems become more probable, like a failure of the hardware where the test runs. I only don't dare to image how long the tests would have to run to achieve that level of certainty. |
|
Speaking for Fedora, I'm going to keep using that patch. The patched code does not make things worse. It fixes the vulnerability everywhere I tested it. |
Previously, equals() function short-cutted a code path if a lenght of
the secret and a lenght of the user-supplied data were different. As
a result, there was a timing side-channel revealing a length of the
secret.
This misfeature could be demonstrated with this code:
use String::Compare::ConstantTime ('equals');
use Benchmark;
timethese(1<<26, {
'0' => sub {equals(q{aaaa}, q{})},
'1' => sub {equals(q{aaaa}, q{a}x1)},
'2' => sub {equals(q{aaaa}, q{a}x2)},
'3' => sub {equals(q{aaaa}, q{a}x3)},
'4' => sub {equals(q{aaaa}, q{a}x4)},
'5' => sub {equals(q{aaaa}, q{a}x5)},
'6' => sub {equals(q{aaaa}, q{a}x6)},
'7' => sub {equals(q{aaaa}, q{a}x7)},
'8' => sub {equals(q{aaaa}, q{a}x8)},
});
The output was able to identify that the secret (first argument of
equals()) had 4 characters (observe the spike in "usr" value for
case hoytech#4):
Benchmark: timing 67108864 iterations of 0, 1, 2, 3, 4, 5, 6, 7, 8...
0: 2 wallclock secs ( 1.70 usr + 0.00 sys = 1.70 CPU) @ 39475802.35/s (n=67108864)
1: 4 wallclock secs ( 4.81 usr + 0.00 sys = 4.81 CPU) @ 13951946.78/s (n=67108864)
2: 5 wallclock secs ( 4.81 usr + 0.00 sys = 4.81 CPU) @ 13951946.78/s (n=67108864)
3: 5 wallclock secs ( 4.86 usr + 0.00 sys = 4.86 CPU) @ 13808408.23/s (n=67108864)
4: 5 wallclock secs ( 5.10 usr + 0.00 sys = 5.10 CPU) @ 13158600.78/s (n=67108864)
5: 4 wallclock secs ( 4.85 usr + 0.00 sys = 4.85 CPU) @ 13836879.18/s (n=67108864)
6: 5 wallclock secs ( 4.83 usr + 0.00 sys = 4.83 CPU) @ 13894174.74/s (n=67108864)
7: 5 wallclock secs ( 4.84 usr + 0.00 sys = 4.84 CPU) @ 13865467.77/s (n=67108864)
8: 5 wallclock secs ( 4.80 usr + 0.00 sys = 4.80 CPU) @ 13981013.33/s (n=67108864)
This misfeature was properly documented in the POD, yet it was
assigned CVE-2024-13939
<https://bugzilla.redhat.com/show_bug.cgi?id=2355663>.
There was a similar issue reported against and fixed in Mojolicous
<mojolicious/mojo#1599>.
This patch fixes this misfeature in similar way. After applying the
fix the benchmark becomes proportional to a length of the user's input
if compiled with -O2 GCC option:
Benchmark: timing 67108864 iterations of 0, 1, 2, 3, 4, 5, 6, 7, 8...
0: 2 wallclock secs ( 1.75 usr + 0.00 sys = 1.75 CPU) @ 38347922.29/s (n=67108864)
1: 6 wallclock secs ( 5.01 usr + 0.00 sys = 5.01 CPU) @ 13394982.83/s (n=67108864)
2: 5 wallclock secs ( 5.00 usr + 0.00 sys = 5.00 CPU) @ 13421772.80/s (n=67108864)
3: 5 wallclock secs ( 5.05 usr + -0.01 sys = 5.04 CPU) @ 13315250.79/s (n=67108864)
4: 5 wallclock secs ( 5.08 usr + 0.00 sys = 5.08 CPU) @ 13210406.30/s (n=67108864)
5: 5 wallclock secs ( 5.11 usr + 0.00 sys = 5.11 CPU) @ 13132850.10/s (n=67108864)
6: 5 wallclock secs ( 5.11 usr + 0.00 sys = 5.11 CPU) @ 13132850.10/s (n=67108864)
7: 5 wallclock secs ( 5.12 usr + 0.00 sys = 5.12 CPU) @ 13107200.00/s (n=67108864)
8: 5 wallclock secs ( 5.12 usr + 0.00 sys = 5.12 CPU) @ 13107200.00/s (n=67108864)
Or inconlusive if compiled with -O3 (vertorized code):
Benchmark: timing 67108864 iterations of 0, 1, 2, 3, 4, 5, 6, 7, 8...
0: 2 wallclock secs ( 1.79 usr + 0.00 sys = 1.79 CPU) @ 37490985.47/s (n=67108864)
1: 5 wallclock secs ( 5.09 usr + 0.00 sys = 5.09 CPU) @ 13184452.65/s (n=67108864)
2: 5 wallclock secs ( 4.99 usr + 0.00 sys = 4.99 CPU) @ 13448670.14/s (n=67108864)
3: 5 wallclock secs ( 5.00 usr + 0.00 sys = 5.00 CPU) @ 13421772.80/s (n=67108864)
4: 4 wallclock secs ( 5.01 usr + 0.00 sys = 5.01 CPU) @ 13394982.83/s (n=67108864)
5: 6 wallclock secs ( 5.01 usr + 0.00 sys = 5.01 CPU) @ 13394982.83/s (n=67108864)
6: 5 wallclock secs ( 5.03 usr + 0.00 sys = 5.03 CPU) @ 13341722.47/s (n=67108864)
7: 5 wallclock secs ( 5.01 usr + 0.00 sys = 5.01 CPU) @ 13394982.83/s (n=67108864)
8: 5 wallclock secs ( 4.99 usr + 0.00 sys = 4.99 CPU) @ 13448670.14/s (n=67108864)
Implementation details: Originally, I called do_compare(b, b, b_len,
r) from equals(), but modern compilers (GCC 15) are too smart, and
optimized that call to a constant. So I moved the length comparison
inside do_compare(). That sufficed until -O3. Then the compiler
started to optimize data aliasing inside functions. The final
implementation replaced a ternary compare-and-assign with
a branch-less algebraic expression which works for me on x86_64 even
with -O3 optimization level.
When the compilers start recognizing these tricks and optimizing them
out too, we will need to disable the optimizations with
compiler-specific extensions. (__attribute__((noinline)),
__attribute__((noipa)), or a dummy asm("")). C.f.
<https://doi.org/10.48550/arXiv.2410.13489>
6 participants
Previously, equals() function short-cutted a code path if a lenght of the secret and a lenght of the user-supplied data were different. As a result, there was a timing side-channel revealing a length of the secret.
This misfeature could be demonstrated with this code:
The output was able to identify that the secret (first argument of equals()) had 4 characters (observe the spike in "usr" value for case #4):
This misfeature was properly documented in the POD, yet it was assigned CVE-2024-13939
https://bugzilla.redhat.com/show_bug.cgi?id=2355663.
There was a similar issue reported against and fixed in Mojolicous mojolicious/mojo#1599.
This patch fixes this misfeature in similar way. After applying the fix the benchmark becomes proportional to a length of the user's input if compiled with -O2 GCC option:
Or inconlusive if compiled with -O3 (vertorized code):
Implementation details: Originally, I called do_compare(b, b, b_len, r) from equals(), but modern compilers (GCC 15) are too smart, and optimized that call to a constant. So I moved the length comparison inside do_compare(). That sufficed until -O3. Then the compiler started to optimize data aliasing inside functions. The final implementation replaced a ternary compare-and-assign with a branch-less algebraic expression which works for me on x86_64 even with -O3 optimization level.
When the compilers start recognizing these tricks and optimizing them out too, we will need to disable the optimizations with compiler-specific extensions. (attribute((noinline)), attribute((noipa)), or a dummy asm("")). C.f.
https://doi.org/10.48550/arXiv.2410.13489