From 33a5ed65919d9b3223bb57fef7b10b6ee9290f2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= Date: Fri, 28 Mar 2025 14:22:03 +0100 Subject: [PATCH] Prevent from revealing a length of the secrect 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 #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 . There was a similar issue reported against and fixed in Mojolicous . 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. --- ConstantTime.xs | 25 +++++++++++++++---------- lib/String/Compare/ConstantTime.pm | 4 +--- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/ConstantTime.xs b/ConstantTime.xs index 79ab132..227d230 100644 --- a/ConstantTime.xs +++ b/ConstantTime.xs @@ -4,12 +4,21 @@ -static int do_compare(unsigned char *a, unsigned char *b, size_t n) { +static int do_compare(unsigned char *a, size_t a_len, unsigned char *b, size_t b_len) { size_t i; - unsigned char r = 0; - - for (i = 0; i < n; i++) { - r |= *a++ ^ *b++; + unsigned char *s; + unsigned char r; + uintptr_t mask; + + /* Orchestrate a dummy compare which never matches and whose run-time does + * not stand out if a_len != b_len */ + r = (a_len != b_len); + /* Branching-less: s = (r) ? b : a */ + mask = (uintptr_t)0u - r; + s = (unsigned char *)(((uintptr_t)b & mask) | ((uintptr_t)a & ~mask)); + + for (i = 0; i < b_len; i++) { + r |= *s++ ^ *b++; } return r; @@ -41,11 +50,7 @@ equals(a, b) ap = SvPV(a, alen); bp = SvPV(b, blen); - if (alen == blen) { - r = !do_compare(ap, bp, alen); - } else { - r = 0; - } + r = !do_compare(ap, alen, bp, blen); } else if (SvOK(a) || SvOK(b)) { r = 0; } else { diff --git a/lib/String/Compare/ConstantTime.pm b/lib/String/Compare/ConstantTime.pm index 7aef0de..d3db545 100644 --- a/lib/String/Compare/ConstantTime.pm +++ b/lib/String/Compare/ConstantTime.pm @@ -51,7 +51,7 @@ An example with HMACs: This module provides one function, C (not exported by default). -You should pass this function two strings of the same length. Just like perl's C, it will return true if they are string-wise identical and false otherwise. However, comparing any two differing strings of the same length will take a fixed amount of time. If the lengths of the strings are different, C will return false right away. +You should pass this function two strings of the same length. Just like perl's C, it will return true if they are string-wise identical and false otherwise. However, comparing any two differing strings of the same length will take a fixed amount of time. If the lengths of the strings are different, C will spend time proportional to a length of the user-supplied string and return false anyway. B: This does byte-wise comparison of the underlying string storage, meaning that comparing strings with non-ASCII data with different states of the internal UTF-8 flag is not reliable. You should always encode your data to bytes before comparing. @@ -62,8 +62,6 @@ Some programs take different amounts of time to run depending on the input value Most routines that compare strings (like perl's C and C and C's C and C) start scanning from the start of the strings and terminate as soon as they determine the strings won't match. This is good for efficiency but bad because it opens a timing side-channel. If one of the strings being compared is a secret and the other is controlled by some untrusted party, it is sometimes possible for this untrusted party to learn the secret using a timing side-channel. -If the lengths of the strings are different, because C returns false right away the size of the secret string may be leaked (but not its contents). - =head1 HMAC