| From 98df8cd09ec7a5b91f05c665529ed6f579f231d9 Mon Sep 17 00:00:00 2001 |
| From: Serge Bazanski <serge@monogon.tech> |
| Date: Tue, 4 Jun 2024 13:53:48 +0200 |
| Subject: [PATCH 5/6] boringssl compat: remove constant time flags (UNSAFE) |
| |
| OpenSSL has a quirky little API to mark bignums as 'secret' ie. |
| 'constant time' which is supposed to influence operations performed on |
| them to be constant time. |
| |
| This API was tricky to use and caused security issues, so it was removed |
| by BoringSSL. |
| |
| https://github.com/google/boringssl/commit/0a211dfe91588d2986a8735e1969dd9202a8b025 |
| |
| Ideally we would replace all relevent BN_mod_exp calls with |
| constant-time versions, but that's not trivial to do: the constant time |
| versions of modular exponentiation and multiplicative inverse operations |
| rely on Montgomery modular multiplication which seems to reduce the |
| domain of the exponent to |0, N>. Unfortunately libtpms has plenty of |
| eg. ModExp operations that work on exponents outside this range. OpenSSL |
| seems to not have applied the constant time request to BN_mod_exp if |
| that was the case, but BoringSSL refuses to perform constant time |
| operations then. |
| |
| As I'm not a cryptographer and not able to fix this properly (or even |
| fully reason about this), I'm just adding a big fat warning to be shown |
| whenever potentially unsafe operations are now performed. |
| --- |
| src/monogon_unsafe.c | 28 ++++++++++++++++++++++++++ |
| src/tpm2/crypto/openssl/BnToOsslMath.c | 10 +++++---- |
| src/tpm2/crypto/openssl/ExpDCache.c | 5 +++-- |
| 3 files changed, 37 insertions(+), 6 deletions(-) |
| create mode 100644 src/monogon_unsafe.c |
| |
| diff --git a/src/monogon_unsafe.c b/src/monogon_unsafe.c |
| new file mode 100644 |
| index 0000000..abaef79 |
| --- /dev/null |
| +++ b/src/monogon_unsafe.c |
| @@ -0,0 +1,28 @@ |
| +#include <stdio.h> |
| +#include <stdlib.h> |
| + |
| +// This library was built against BoringSSL without the BN constant time API, |
| +// thus all cryptographic operations are performed timing-unsafe which might |
| +// lead to side channel leaks. This is fine for Monogon's usecase (swtpm in |
| +// tests) but this code must not end up being used to secure any real systems. |
| +// |
| +// Note: I am not sure this code was safe from side channels in the first |
| +// place. See RsaPrivateKeyOp and compare with BoringSSL's |
| +// rsa_default_private_transform implementation... ~q3k |
| + |
| +static int _warned = 0; |
| + |
| +void monogon_warn_unsafe_library(void) |
| +{ |
| + if (getenv("MONOGON_LIBTPMS_ACKNOWLEDGE_UNSAFE") != NULL) { |
| + return; |
| + } |
| + if (_warned) { |
| + return; |
| + } |
| + _warned = 1; |
| + fprintf(stderr, "--------------------------------------------------------------------------------\n"); |
| + fprintf(stderr, "WARNING: This fork of libtpms/swtpm contains UNSAFE cryptographic operations and\n"); |
| + fprintf(stderr, " MUST NOT be used to secure sensitive data.\n"); |
| + fprintf(stderr, "--------------------------------------------------------------------------------\n"); |
| +} |
| diff --git a/src/tpm2/crypto/openssl/BnToOsslMath.c b/src/tpm2/crypto/openssl/BnToOsslMath.c |
| index 7d13ce8..54d5916 100644 |
| --- a/src/tpm2/crypto/openssl/BnToOsslMath.c |
| +++ b/src/tpm2/crypto/openssl/BnToOsslMath.c |
| @@ -83,6 +83,8 @@ |
| //#include "Tpm.h" |
| #include "BnOssl.h" |
| |
| +extern void monogon_warn_unsafe_library(); |
| + |
| #ifdef MATH_LIB_OSSL |
| # include "BnToOsslMath_fp.h" |
| |
| @@ -133,6 +135,7 @@ BOOL OsslToTpmBn(bigNum bn, const BIGNUM* osslBn) // libtpms: added 'const' |
| // function prototype. Instead, use BnNewVariable(). |
| BIGNUM* BigInitialized(BIGNUM* toInit, bigConst initializer) |
| { |
| + monogon_warn_unsafe_library(); |
| #if 1 // libtpms: added begin |
| BIGNUM *_toInit; |
| unsigned char buffer[LARGEST_NUMBER + 1]; |
| @@ -147,7 +150,6 @@ BIGNUM* BigInitialized(BIGNUM* toInit, bigConst initializer) |
| #if 1 // libtpms: added begin |
| BnToBytes(initializer, buffer, &buffer_len); /* TPM to bin */ |
| _toInit = BN_bin2bn(buffer, buffer_len, NULL); /* bin to ossl */ |
| - BN_set_flags(_toInit, BN_FLG_CONSTTIME); |
| BN_copy(toInit, _toInit); |
| BN_clear_free(_toInit); |
| #else // libtpms: added end |
| @@ -355,13 +357,13 @@ LIB_EXPORT BOOL BnGcd(bigNum gcd, // OUT: the common divisor |
| bigConst number2 // IN: |
| ) |
| { |
| + monogon_warn_unsafe_library(); |
| OSSL_ENTER(); |
| BIGNUM* bnGcd = BN_NEW(); |
| BOOL OK = TRUE; |
| BIG_INITIALIZED(bn1, number1); |
| BIG_INITIALIZED(bn2, number2); |
| // |
| - BN_set_flags(bn1, BN_FLG_CONSTTIME); // number1 is secret prime number |
| GOTO_ERROR_UNLESS(BN_gcd(bnGcd, bn1, bn2, CTX)); |
| GOTO_ERROR_UNLESS(OsslToTpmBn(gcd, bnGcd)); |
| goto Exit; |
| @@ -387,6 +389,7 @@ LIB_EXPORT BOOL BnModExp(bigNum result, // OUT: the result |
| bigConst modulus // IN: |
| ) |
| { |
| + monogon_warn_unsafe_library(); |
| OSSL_ENTER(); |
| BIGNUM* bnResult = BN_NEW(); |
| BOOL OK = TRUE; |
| @@ -394,7 +397,6 @@ LIB_EXPORT BOOL BnModExp(bigNum result, // OUT: the result |
| BIG_INITIALIZED(bnE, exponent); |
| BIG_INITIALIZED(bnM, modulus); |
| // |
| - BN_set_flags(bnE, BN_FLG_CONSTTIME); // exponent may be private |
| GOTO_ERROR_UNLESS(BN_mod_exp(bnResult, bnN, bnE, bnM, CTX)); |
| GOTO_ERROR_UNLESS(OsslToTpmBn(result, bnResult)); |
| goto Exit; |
| @@ -416,13 +418,13 @@ LIB_EXPORT BOOL BnModExp(bigNum result, // OUT: the result |
| // FALSE(0) failure in operation |
| LIB_EXPORT BOOL BnModInverse(bigNum result, bigConst number, bigConst modulus) |
| { |
| + monogon_warn_unsafe_library(); |
| OSSL_ENTER(); |
| BIGNUM* bnResult = BN_NEW(); |
| BOOL OK = TRUE; |
| BIG_INITIALIZED(bnN, number); |
| BIG_INITIALIZED(bnM, modulus); |
| // |
| - BN_set_flags(bnN, BN_FLG_CONSTTIME); // number may be private |
| GOTO_ERROR_UNLESS(BN_mod_inverse(bnResult, bnN, bnM, CTX) != NULL); |
| GOTO_ERROR_UNLESS(OsslToTpmBn(result, bnResult)); |
| goto Exit; |
| diff --git a/src/tpm2/crypto/openssl/ExpDCache.c b/src/tpm2/crypto/openssl/ExpDCache.c |
| index 5aeaf14..133e9ed 100644 |
| --- a/src/tpm2/crypto/openssl/ExpDCache.c |
| +++ b/src/tpm2/crypto/openssl/ExpDCache.c |
| @@ -61,6 +61,8 @@ |
| #include "Tpm.h" |
| #include "ExpDCache_fp.h" |
| |
| +extern void monogon_warn_unsafe_library(void); |
| + |
| /* Implement a cache for the private exponent D so it doesn't need to be |
| * recalculated every time from P, Q, E and N (modulus). The cache has a |
| * number of entries that cache D and use P, Q, and E for lookup. |
| @@ -169,6 +171,7 @@ BIGNUM *ExpDCacheFind(const BIGNUM *P, const BIGNUM *N, const BIGNUM *E, BIGNUM |
| unsigned myage; |
| BIGNUM *D; |
| |
| + monogon_warn_unsafe_library(); |
| for (i = 0; i < ARRAY_SIZE(ExpDCache); i++) { |
| if (BN_cmp(ExpDCache[i].P, P) == 0 && BN_cmp(ExpDCache[i].N, N) == 0 && |
| BN_cmp(ExpDCache[i].E, E) == 0) { |
| @@ -190,8 +193,6 @@ BIGNUM *ExpDCacheFind(const BIGNUM *P, const BIGNUM *N, const BIGNUM *E, BIGNUM |
| *Q = NULL; |
| return NULL; |
| } |
| - BN_set_flags(*Q, BN_FLG_CONSTTIME); |
| - BN_set_flags(D, BN_FLG_CONSTTIME); |
| return D; |
| } |
| } |
| -- |
| 2.42.0 |
| |