| Serge Bazanski | 72c1f2b | 2024-06-04 13:42:48 +0000 | [diff] [blame] | 1 | From 98df8cd09ec7a5b91f05c665529ed6f579f231d9 Mon Sep 17 00:00:00 2001 |
| 2 | From: Serge Bazanski <serge@monogon.tech> |
| 3 | Date: Tue, 4 Jun 2024 13:53:48 +0200 |
| 4 | Subject: [PATCH 5/6] boringssl compat: remove constant time flags (UNSAFE) |
| 5 | |
| 6 | OpenSSL has a quirky little API to mark bignums as 'secret' ie. |
| 7 | 'constant time' which is supposed to influence operations performed on |
| 8 | them to be constant time. |
| 9 | |
| 10 | This API was tricky to use and caused security issues, so it was removed |
| 11 | by BoringSSL. |
| 12 | |
| 13 | https://github.com/google/boringssl/commit/0a211dfe91588d2986a8735e1969dd9202a8b025 |
| 14 | |
| 15 | Ideally we would replace all relevent BN_mod_exp calls with |
| 16 | constant-time versions, but that's not trivial to do: the constant time |
| 17 | versions of modular exponentiation and multiplicative inverse operations |
| 18 | rely on Montgomery modular multiplication which seems to reduce the |
| 19 | domain of the exponent to |0, N>. Unfortunately libtpms has plenty of |
| 20 | eg. ModExp operations that work on exponents outside this range. OpenSSL |
| 21 | seems to not have applied the constant time request to BN_mod_exp if |
| 22 | that was the case, but BoringSSL refuses to perform constant time |
| 23 | operations then. |
| 24 | |
| 25 | As I'm not a cryptographer and not able to fix this properly (or even |
| 26 | fully reason about this), I'm just adding a big fat warning to be shown |
| 27 | whenever potentially unsafe operations are now performed. |
| 28 | --- |
| 29 | src/monogon_unsafe.c | 28 ++++++++++++++++++++++++++ |
| 30 | src/tpm2/crypto/openssl/BnToOsslMath.c | 10 +++++---- |
| 31 | src/tpm2/crypto/openssl/ExpDCache.c | 5 +++-- |
| 32 | 3 files changed, 37 insertions(+), 6 deletions(-) |
| 33 | create mode 100644 src/monogon_unsafe.c |
| 34 | |
| 35 | diff --git a/src/monogon_unsafe.c b/src/monogon_unsafe.c |
| 36 | new file mode 100644 |
| 37 | index 0000000..abaef79 |
| 38 | --- /dev/null |
| 39 | +++ b/src/monogon_unsafe.c |
| 40 | @@ -0,0 +1,28 @@ |
| 41 | +#include <stdio.h> |
| 42 | +#include <stdlib.h> |
| 43 | + |
| 44 | +// This library was built against BoringSSL without the BN constant time API, |
| 45 | +// thus all cryptographic operations are performed timing-unsafe which might |
| 46 | +// lead to side channel leaks. This is fine for Monogon's usecase (swtpm in |
| 47 | +// tests) but this code must not end up being used to secure any real systems. |
| 48 | +// |
| 49 | +// Note: I am not sure this code was safe from side channels in the first |
| 50 | +// place. See RsaPrivateKeyOp and compare with BoringSSL's |
| 51 | +// rsa_default_private_transform implementation... ~q3k |
| 52 | + |
| 53 | +static int _warned = 0; |
| 54 | + |
| 55 | +void monogon_warn_unsafe_library(void) |
| 56 | +{ |
| 57 | + if (getenv("MONOGON_LIBTPMS_ACKNOWLEDGE_UNSAFE") != NULL) { |
| 58 | + return; |
| 59 | + } |
| 60 | + if (_warned) { |
| 61 | + return; |
| 62 | + } |
| 63 | + _warned = 1; |
| 64 | + fprintf(stderr, "--------------------------------------------------------------------------------\n"); |
| 65 | + fprintf(stderr, "WARNING: This fork of libtpms/swtpm contains UNSAFE cryptographic operations and\n"); |
| 66 | + fprintf(stderr, " MUST NOT be used to secure sensitive data.\n"); |
| 67 | + fprintf(stderr, "--------------------------------------------------------------------------------\n"); |
| 68 | +} |
| 69 | diff --git a/src/tpm2/crypto/openssl/BnToOsslMath.c b/src/tpm2/crypto/openssl/BnToOsslMath.c |
| 70 | index 7d13ce8..54d5916 100644 |
| 71 | --- a/src/tpm2/crypto/openssl/BnToOsslMath.c |
| 72 | +++ b/src/tpm2/crypto/openssl/BnToOsslMath.c |
| 73 | @@ -83,6 +83,8 @@ |
| 74 | //#include "Tpm.h" |
| 75 | #include "BnOssl.h" |
| 76 | |
| 77 | +extern void monogon_warn_unsafe_library(); |
| 78 | + |
| 79 | #ifdef MATH_LIB_OSSL |
| 80 | # include "BnToOsslMath_fp.h" |
| 81 | |
| 82 | @@ -133,6 +135,7 @@ BOOL OsslToTpmBn(bigNum bn, const BIGNUM* osslBn) // libtpms: added 'const' |
| 83 | // function prototype. Instead, use BnNewVariable(). |
| 84 | BIGNUM* BigInitialized(BIGNUM* toInit, bigConst initializer) |
| 85 | { |
| 86 | + monogon_warn_unsafe_library(); |
| 87 | #if 1 // libtpms: added begin |
| 88 | BIGNUM *_toInit; |
| 89 | unsigned char buffer[LARGEST_NUMBER + 1]; |
| 90 | @@ -147,7 +150,6 @@ BIGNUM* BigInitialized(BIGNUM* toInit, bigConst initializer) |
| 91 | #if 1 // libtpms: added begin |
| 92 | BnToBytes(initializer, buffer, &buffer_len); /* TPM to bin */ |
| 93 | _toInit = BN_bin2bn(buffer, buffer_len, NULL); /* bin to ossl */ |
| 94 | - BN_set_flags(_toInit, BN_FLG_CONSTTIME); |
| 95 | BN_copy(toInit, _toInit); |
| 96 | BN_clear_free(_toInit); |
| 97 | #else // libtpms: added end |
| 98 | @@ -355,13 +357,13 @@ LIB_EXPORT BOOL BnGcd(bigNum gcd, // OUT: the common divisor |
| 99 | bigConst number2 // IN: |
| 100 | ) |
| 101 | { |
| 102 | + monogon_warn_unsafe_library(); |
| 103 | OSSL_ENTER(); |
| 104 | BIGNUM* bnGcd = BN_NEW(); |
| 105 | BOOL OK = TRUE; |
| 106 | BIG_INITIALIZED(bn1, number1); |
| 107 | BIG_INITIALIZED(bn2, number2); |
| 108 | // |
| 109 | - BN_set_flags(bn1, BN_FLG_CONSTTIME); // number1 is secret prime number |
| 110 | GOTO_ERROR_UNLESS(BN_gcd(bnGcd, bn1, bn2, CTX)); |
| 111 | GOTO_ERROR_UNLESS(OsslToTpmBn(gcd, bnGcd)); |
| 112 | goto Exit; |
| 113 | @@ -387,6 +389,7 @@ LIB_EXPORT BOOL BnModExp(bigNum result, // OUT: the result |
| 114 | bigConst modulus // IN: |
| 115 | ) |
| 116 | { |
| 117 | + monogon_warn_unsafe_library(); |
| 118 | OSSL_ENTER(); |
| 119 | BIGNUM* bnResult = BN_NEW(); |
| 120 | BOOL OK = TRUE; |
| 121 | @@ -394,7 +397,6 @@ LIB_EXPORT BOOL BnModExp(bigNum result, // OUT: the result |
| 122 | BIG_INITIALIZED(bnE, exponent); |
| 123 | BIG_INITIALIZED(bnM, modulus); |
| 124 | // |
| 125 | - BN_set_flags(bnE, BN_FLG_CONSTTIME); // exponent may be private |
| 126 | GOTO_ERROR_UNLESS(BN_mod_exp(bnResult, bnN, bnE, bnM, CTX)); |
| 127 | GOTO_ERROR_UNLESS(OsslToTpmBn(result, bnResult)); |
| 128 | goto Exit; |
| 129 | @@ -416,13 +418,13 @@ LIB_EXPORT BOOL BnModExp(bigNum result, // OUT: the result |
| 130 | // FALSE(0) failure in operation |
| 131 | LIB_EXPORT BOOL BnModInverse(bigNum result, bigConst number, bigConst modulus) |
| 132 | { |
| 133 | + monogon_warn_unsafe_library(); |
| 134 | OSSL_ENTER(); |
| 135 | BIGNUM* bnResult = BN_NEW(); |
| 136 | BOOL OK = TRUE; |
| 137 | BIG_INITIALIZED(bnN, number); |
| 138 | BIG_INITIALIZED(bnM, modulus); |
| 139 | // |
| 140 | - BN_set_flags(bnN, BN_FLG_CONSTTIME); // number may be private |
| 141 | GOTO_ERROR_UNLESS(BN_mod_inverse(bnResult, bnN, bnM, CTX) != NULL); |
| 142 | GOTO_ERROR_UNLESS(OsslToTpmBn(result, bnResult)); |
| 143 | goto Exit; |
| 144 | diff --git a/src/tpm2/crypto/openssl/ExpDCache.c b/src/tpm2/crypto/openssl/ExpDCache.c |
| 145 | index 5aeaf14..133e9ed 100644 |
| 146 | --- a/src/tpm2/crypto/openssl/ExpDCache.c |
| 147 | +++ b/src/tpm2/crypto/openssl/ExpDCache.c |
| 148 | @@ -61,6 +61,8 @@ |
| 149 | #include "Tpm.h" |
| 150 | #include "ExpDCache_fp.h" |
| 151 | |
| 152 | +extern void monogon_warn_unsafe_library(void); |
| 153 | + |
| 154 | /* Implement a cache for the private exponent D so it doesn't need to be |
| 155 | * recalculated every time from P, Q, E and N (modulus). The cache has a |
| 156 | * number of entries that cache D and use P, Q, and E for lookup. |
| 157 | @@ -169,6 +171,7 @@ BIGNUM *ExpDCacheFind(const BIGNUM *P, const BIGNUM *N, const BIGNUM *E, BIGNUM |
| 158 | unsigned myage; |
| 159 | BIGNUM *D; |
| 160 | |
| 161 | + monogon_warn_unsafe_library(); |
| 162 | for (i = 0; i < ARRAY_SIZE(ExpDCache); i++) { |
| 163 | if (BN_cmp(ExpDCache[i].P, P) == 0 && BN_cmp(ExpDCache[i].N, N) == 0 && |
| 164 | BN_cmp(ExpDCache[i].E, E) == 0) { |
| 165 | @@ -190,8 +193,6 @@ BIGNUM *ExpDCacheFind(const BIGNUM *P, const BIGNUM *N, const BIGNUM *E, BIGNUM |
| 166 | *Q = NULL; |
| 167 | return NULL; |
| 168 | } |
| 169 | - BN_set_flags(*Q, BN_FLG_CONSTTIME); |
| 170 | - BN_set_flags(D, BN_FLG_CONSTTIME); |
| 171 | return D; |
| 172 | } |
| 173 | } |
| 174 | -- |
| 175 | 2.42.0 |
| 176 | |