From 5651758f69e1f7c84fffc8043458e9db966f76d5 Mon Sep 17 00:00:00 2001 From: Emma Stensland Date: Mon, 21 Sep 2026 13:47:07 -0600 Subject: [PATCH] misc: reject a negative length in ConstantCompare() Returning 0 for a negative length reported "equal" for a comparison that never ran. The sentinel is positive because AES_GCM_decrypt_C() folds this return into a mask that reads a negative value as a match. --- ChangeLog.md | 9 +++++ wolfcrypt/src/aes.c | 8 +--- wolfcrypt/src/misc.c | 16 ++++++-- wolfcrypt/test/test.c | 79 ++++++++++++++++++++++++++++++++++++++ wolfcrypt/test/test.h | 15 ++++++++ wolfssl/wolfcrypt/memory.h | 2 + 6 files changed, 119 insertions(+), 10 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index c17e2e2e511..6164703d6ca 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -101,6 +101,15 @@ leak must stop doing so, and the returned key is shared with the certificate, so it must be treated as read only. +* **Behavioral change (`ConstantCompare()` rejects a negative length)**: + `ConstantCompare()` and `wc_ConstantCompare()` now return 1, not 0 + ("equal"), for a negative `length`, so a caller whose length arithmetic + underflows no longer gets a false match. Differences are normalized to 1, + so wide-byte targets (such as C28x) never return negative on a high-bit + difference. `length == 0` still returns 0. The sentinel is positive + because callers fold the result into constant-time masks that read a + negative value as a match. + ## Post-Quantum Cryptography (PQC) * Added opt-in per-key Falcon signing caches (`--enable-falcon=cache-key`, `cache-basis`), roughly doubling signing speed with the default integer fpr backend. by @Frauschi diff --git a/wolfcrypt/src/aes.c b/wolfcrypt/src/aes.c index 254d595fbbe..06c48d12376 100644 --- a/wolfcrypt/src/aes.c +++ b/wolfcrypt/src/aes.c @@ -12602,9 +12602,7 @@ int WARN_UNUSED_RESULT AES_GCM_decrypt_C( return ret; xorbuf(Tprime, EKY0, sizeof(Tprime)); #ifdef WC_AES_GCM_DEC_AUTH_EARLY - /* ConstantCompare returns the cumulative bitwise or of the bitwise xor of - * the pairwise bytes in the strings. - */ + /* ConstantCompare returns 0 when the tags match and 1 otherwise. */ res = ConstantCompare(authTag, Tprime, authTagSz); /* convert positive retval from ConstantCompare() to all-1s word, in * constant time. @@ -12685,9 +12683,7 @@ int WARN_UNUSED_RESULT AES_GCM_decrypt_C( } #ifndef WC_AES_GCM_DEC_AUTH_EARLY - /* ConstantCompare returns the cumulative bitwise or of the bitwise xor of - * the pairwise bytes in the strings. - */ + /* ConstantCompare returns 0 when the tags match and 1 otherwise. */ res = ConstantCompare(authTag, Tprime, (int)authTagSz); /* convert positive retval from ConstantCompare() to all-1s word, in * constant time. diff --git a/wolfcrypt/src/misc.c b/wolfcrypt/src/misc.c index f4b81fb8373..91b9334e84b 100644 --- a/wolfcrypt/src/misc.c +++ b/wolfcrypt/src/misc.c @@ -826,18 +826,26 @@ WC_MISC_STATIC WC_INLINE void ForceZero(void* mem, size_t len) #ifndef WOLFSSL_NO_CONST_CMP -/* check all length bytes for equality, return 0 on success */ +/* Compare length bytes of a and b in constant time. + * Returns 0 if equal, 1 if not or if length is negative. */ WC_MISC_STATIC WC_INLINE int ConstantCompare(const byte* a, const byte* b, int length) { int i; - int compareSum = 0; + word32 compareSum = 0; + + /* length is public, so branching is safe. Return positive: callers + * such as AES_GCM_decrypt_C() build masks that read negative as equal. */ + if (length < 0) { + return 1; + } for (i = 0; i < length; i++) { - compareSum |= a[i] ^ b[i]; + compareSum |= (word32)(a[i] ^ b[i]); } - return compareSum; + /* Normalize to 0/1 without a compare on secret data. */ + return (int)(((compareSum | ((word32)0 - compareSum)) >> 31) & 1U); } #endif diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index 7c675236b75..b5ac2d10e17 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -864,6 +864,9 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t macro_test(void); WOLFSSL_TEST_SUBROUTINE wc_test_ret_t error_test(void); WOLFSSL_TEST_SUBROUTINE wc_test_ret_t flags_test(void); WOLFSSL_TEST_SUBROUTINE wc_test_ret_t octets_test(void); +#ifdef WC_TEST_CONSTANTCOMPARE +WOLFSSL_TEST_SUBROUTINE wc_test_ret_t constantcompare_test(void); +#endif WOLFSSL_TEST_SUBROUTINE wc_test_ret_t base64_test(void); WOLFSSL_TEST_SUBROUTINE wc_test_ret_t base16_test(void); WOLFSSL_TEST_SUBROUTINE wc_test_ret_t asn_test(void); @@ -2729,6 +2732,13 @@ options: [-s max_relative_stack_bytes] [-m max_relative_heap_memory_bytes]\n\ else TEST_PASS("MEMORY test passed!\n"); +#ifdef WC_TEST_CONSTANTCOMPARE + if ( (ret = constantcompare_test()) != 0) + TEST_FAIL("constantcompare test failed!\n", ret); + else + TEST_PASS("constantcompare test passed!\n"); +#endif + if ( (ret = octets_test()) != 0) TEST_FAIL("octets test failed!\n", ret); else @@ -5523,6 +5533,75 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t octets_test(void) return 0; } +#ifdef WC_TEST_CONSTANTCOMPARE + +typedef int (*constantcompare_fn)(const byte* a, const byte* b, int length); + +/* Run the ConstantCompare() checks against cmp. + * Returns 0 on success, a test error code on failure. */ +static wc_test_ret_t constantcompare_sweep(constantcompare_fn cmp) +{ + byte a[9] = { 0, 1, 2, 3, 4, 5, 6, 7, 8 }; + byte b[9]; + int i; + int length = (int)sizeof(a) - 1; /* leave a[8]/b[8] out of range */ + + XMEMCPY(b, a, sizeof(a)); + if (cmp(a, b, length) != 0) + return WC_TEST_RET_ENC_NC; + b[0] ^= 0x01; /* length 0 must ignore even a differing first byte */ + if (cmp(a, b, 0) != 0) + return WC_TEST_RET_ENC_NC; + + /* A difference at any index must return exactly 1. + * Exercise both low and high bits (e.g. bit 15 on WOLFSSL_WIDE_BYTE). */ + for (i = 0; i < length; i++) { + XMEMCPY(b, a, sizeof(a)); + b[i] ^= 0x01; + if (cmp(a, b, length) != 1) + return WC_TEST_RET_ENC_I(i); + b[i] ^= (byte)~0; + if (cmp(a, b, length) != 1) + return WC_TEST_RET_ENC_I(i); + } + + /* A difference past length must be ignored. */ + XMEMCPY(b, a, sizeof(a)); + b[length] ^= 0x01; + if (cmp(a, b, length) != 0) + return WC_TEST_RET_ENC_NC; + + /* Negative length must return 1, not 0. */ + XMEMSET(b, (byte)~a[0], sizeof(b)); + if (cmp(a, b, -1) != 1) + return WC_TEST_RET_ENC_NC; + + return 0; +} + +/* Test ConstantCompare() and wc_ConstantCompare(). + * Returns 0 on success, a test error code on failure. */ +WOLFSSL_TEST_SUBROUTINE wc_test_ret_t constantcompare_test(void) +{ + wc_test_ret_t ret = 0; + + WOLFSSL_ENTER("constantcompare_test"); + + /* Inline copy from misc.c. Under NO_INLINE it is hidden in the library + * and cannot be linked; wc_ConstantCompare() covers the same code. */ +#ifndef NO_INLINE + ret = constantcompare_sweep(ConstantCompare); + if (ret != 0) + return ret; +#endif + +#ifdef WC_TEST_HAVE_WC_CONSTANTCOMPARE + ret = constantcompare_sweep(wc_ConstantCompare); +#endif + return ret; +} +#endif /* WC_TEST_CONSTANTCOMPARE */ + #ifndef NO_CODING WOLFSSL_TEST_SUBROUTINE wc_test_ret_t base64_test(void) diff --git a/wolfcrypt/test/test.h b/wolfcrypt/test/test.h index b63b5810bd6..19835020272 100644 --- a/wolfcrypt/test/test.h +++ b/wolfcrypt/test/test.h @@ -138,6 +138,18 @@ wc_static_assert(-(long)MIN_CODE_E < 0x7ffL); #endif /* !WC_TEST_RET_HAVE_CUSTOM_MACROS */ +#ifndef WOLFSSL_NO_CONST_CMP + /* wc_ConstantCompare() lives in memory.c, which is not built under + * NO_WOLFSSL_MEMORY or, with autotools, WOLFSSL_LEANPSK. */ + #if !defined(NO_WOLFSSL_MEMORY) && !defined(WOLFSSL_LEANPSK) + #define WC_TEST_HAVE_WC_CONSTANTCOMPARE + #endif + /* Test only when ConstantCompare() or wc_ConstantCompare() is linkable. */ + #if !defined(NO_INLINE) || defined(WC_TEST_HAVE_WC_CONSTANTCOMPARE) + #define WC_TEST_CONSTANTCOMPARE + #endif +#endif + #ifdef WC_TEST_EXPORT_SUBTESTS #if defined(NO_FILESYSTEM) || defined(WC_NO_RNG) @@ -158,6 +170,9 @@ extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t macro_test(void); extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t error_test(void); extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t flags_test(void); extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t octets_test(void); +#ifdef WC_TEST_CONSTANTCOMPARE +extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t constantcompare_test(void); +#endif extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t base64_test(void); extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t base16_test(void); extern WOLFSSL_TEST_SUBROUTINE wc_test_ret_t asn_test(void); diff --git a/wolfssl/wolfcrypt/memory.h b/wolfssl/wolfcrypt/memory.h index 91200566acb..ff8ba85d9f2 100644 --- a/wolfssl/wolfcrypt/memory.h +++ b/wolfssl/wolfcrypt/memory.h @@ -336,6 +336,8 @@ WOLFSSL_API void wc_ForceZero(void *mem, size_t len); #endif #ifndef WOLFSSL_NO_CONST_CMP +/* Compare length bytes of a and b in constant time. + * Returns 0 if equal, 1 if they differ or if length is negative. */ WOLFSSL_API int wc_ConstantCompare(const byte* a, const byte* b, int length); #endif