From 5cc36c39d2a91e0f526249248961ec9b5fa31c2d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sun, 11 Sep 2022 10:48:34 +0200 Subject: [PATCH] crypto: fix weak randomness in WebCrypto keygen Commit dae283d96f from August 2020 introduced a call to EntropySource() in SecretKeyGenTraits::DoKeyGen() in src/crypto/crypto_keygen.cc. There are two problems with that: 1. It does not check the return value, it assumes EntropySource() always succeeds, but it can (and sometimes will) fail. 2. The random data returned byEntropySource() may not be cryptographically strong and therefore not suitable as keying material. An example is a freshly booted system or a system without /dev/random or getrandom(2). EntropySource() calls out to openssl's RAND_poll() and RAND_bytes() in a best-effort attempt to obtain random data. OpenSSL has a built-in CSPRNG but that can fail to initialize, in which case it's possible either: 1. No random data gets written to the output buffer, i.e., the output is unmodified, or 2. Weak random data is written. It's theoretically possible for the output to be fully predictable because the CSPRNG starts from a predictable state. Replace EntropySource() and CheckEntropy() with new function CSPRNG() that enforces checking of the return value. Abort on startup when the entropy pool fails to initialize because that makes it too easy to compromise the security of the process. Refs: https://hackerone.com/bugs?report_id=1690000 Refs: https://github.com/nodejs/node/pull/35093 --- node.gyp | 2 ++ src/crypto/crypto_context.cc | 19 +++++++++++-------- src/crypto/crypto_keygen.cc | 3 ++- src/crypto/crypto_keygen.h | 3 --- src/crypto/crypto_random.cc | 15 +++++++-------- src/crypto/crypto_util.cc | 32 +++++++------------------------- src/crypto/crypto_util.h | 35 +++++++++++------------------------ src/inspector_io.cc | 3 +-- src/node.cc | 25 +++++++++++++++---------- 9 files changed, 56 insertions(+), 81 deletions(-) diff --git a/node.gyp b/node.gyp index 51bdae5770..9a0263ab0e 100644 --- a/node.gyp +++ b/node.gyp @@ -681,6 +681,8 @@ 'openssl_default_cipher_list%': '', }, + 'cflags': ['-Werror=unused-result'], + 'defines': [ 'NODE_ARCH="<(target_arch)"', 'NODE_PLATFORM="<(OS)"', diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 7eab9de37c..e70ddc64f3 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -541,9 +541,9 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { // OpenSSL 1.1.0 changed the ticket key size, but the OpenSSL 1.0.x size was // exposed in the public API. To retain compatibility, install a callback // which restores the old algorithm. - if (RAND_bytes(sc->ticket_key_name_, sizeof(sc->ticket_key_name_)) <= 0 || - RAND_bytes(sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_)) <= 0 || - RAND_bytes(sc->ticket_key_aes_, sizeof(sc->ticket_key_aes_)) <= 0) { + if (CSPRNG(sc->ticket_key_name_, sizeof(sc->ticket_key_name_)).is_err() || + CSPRNG(sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_)).is_err() || + CSPRNG(sc->ticket_key_aes_, sizeof(sc->ticket_key_aes_)).is_err()) { return THROW_ERR_CRYPTO_OPERATION_FAILED( env, "Error generating ticket keys"); } @@ -1241,11 +1241,14 @@ int SecureContext::TicketCompatibilityCallback(SSL* ssl, if (enc) { memcpy(name, sc->ticket_key_name_, sizeof(sc->ticket_key_name_)); - if (RAND_bytes(iv, 16) <= 0 || - EVP_EncryptInit_ex(ectx, EVP_aes_128_cbc(), nullptr, - sc->ticket_key_aes_, iv) <= 0 || - HMAC_Init_ex(hctx, sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_), - EVP_sha256(), nullptr) <= 0) { + if (CSPRNG(iv, 16).is_err() || + EVP_EncryptInit_ex( + ectx, EVP_aes_128_cbc(), nullptr, sc->ticket_key_aes_, iv) <= 0 || + HMAC_Init_ex(hctx, + sc->ticket_key_hmac_, + sizeof(sc->ticket_key_hmac_), + EVP_sha256(), + nullptr) <= 0) { return -1; } return 1; diff --git a/src/crypto/crypto_keygen.cc b/src/crypto/crypto_keygen.cc index 684cf8033e..cf78524446 100644 --- a/src/crypto/crypto_keygen.cc +++ b/src/crypto/crypto_keygen.cc @@ -80,7 +80,8 @@ KeyGenJobStatus SecretKeyGenTraits::DoKeyGen( SecretKeyGenConfig* params) { CHECK_LE(params->length, INT_MAX); ByteSource::Builder bytes(params->length); - EntropySource(bytes.data(), params->length); + if (CSPRNG(bytes.data(), params->length).is_err()) + return KeyGenJobStatus::FAILED; params->out = std::move(bytes).release(); return KeyGenJobStatus::OK; } diff --git a/src/crypto/crypto_keygen.h b/src/crypto/crypto_keygen.h index c9c1b3e5ff..389b6b5e8d 100644 --- a/src/crypto/crypto_keygen.h +++ b/src/crypto/crypto_keygen.h @@ -75,9 +75,6 @@ class KeyGenJob final : public CryptoJob { std::move(params)) {} void DoThreadPoolWork() override { - // Make sure the CSPRNG is properly seeded so the results are secure. - CheckEntropy(); - AdditionalParams* params = CryptoJob::params(); switch (KeyGenTraits::DoKeyGen(AsyncWrap::env(), params)) { diff --git a/src/crypto/crypto_random.cc b/src/crypto/crypto_random.cc index d0736a9cf1..2f9e9aacb1 100644 --- a/src/crypto/crypto_random.cc +++ b/src/crypto/crypto_random.cc @@ -66,8 +66,7 @@ bool RandomBytesTraits::DeriveBits( Environment* env, const RandomBytesConfig& params, ByteSource* unused) { - CheckEntropy(); // Ensure that OpenSSL's PRNG is properly seeded. - return RAND_bytes(params.buffer, params.size) != 0; + return CSPRNG(params.buffer, params.size).is_ok(); } void RandomPrimeConfig::MemoryInfo(MemoryTracker* tracker) const { @@ -156,12 +155,12 @@ Maybe RandomPrimeTraits::AdditionalConfig( return Just(true); } -bool RandomPrimeTraits::DeriveBits( - Environment* env, - const RandomPrimeConfig& params, - ByteSource* unused) { - - CheckEntropy(); +bool RandomPrimeTraits::DeriveBits(Environment* env, + const RandomPrimeConfig& params, + ByteSource* unused) { + // BN_generate_prime_ex() calls RAND_bytes_ex() internally. + // Make sure the CSPRNG is properly seeded. + CHECK(CSPRNG(nullptr, 0).is_ok()); if (BN_generate_prime_ex( params.prime.get(), diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 7255344276..8bdbe1f1a8 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -60,32 +60,14 @@ int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) { return 1; } -void CheckEntropy() { - for (;;) { - int status = RAND_status(); - CHECK_GE(status, 0); // Cannot fail. - if (status != 0) - break; +MUST_USE_RESULT CSPRNGResult CSPRNG(void* buffer, size_t length) { + do { + if (1 == RAND_status()) + if (1 == RAND_bytes(static_cast(buffer), length)) + return {true}; + } while (1 == RAND_poll()); - // Give up, RAND_poll() not supported. - if (RAND_poll() == 0) - break; - } -} - -bool EntropySource(unsigned char* buffer, size_t length) { - // Ensure that OpenSSL's PRNG is properly seeded. - CheckEntropy(); - // If RAND_bytes() returns 0 or -1, the data might not be random at all. In - // that case, return false, which causes V8 to use its own entropy source. The - // quality of V8's entropy source depends on multiple factors and we should - // not assume that it is cryptographically secure (even though it often is). - // However, even if RAND_bytes() fails and V8 resorts to its potentially weak - // entropy source, it really does not matter much: V8 only uses the entropy - // source to seed its own PRNG, which itself is not cryptographically secure. - // In other words, even a cryptographically secure entropy source would not - // guarantee cryptographically secure random numbers in V8. - return RAND_bytes(buffer, length) == 1; + return {false}; } int PasswordCallback(char* buf, int size, int rwflag, void* u) { diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h index f1fbd6c247..dc3bb15cfb 100644 --- a/src/crypto/crypto_util.h +++ b/src/crypto/crypto_util.h @@ -111,31 +111,18 @@ struct MarkPopErrorOnReturn { ~MarkPopErrorOnReturn() { ERR_pop_to_mark(); } }; -// Ensure that OpenSSL has enough entropy (at least 256 bits) for its PRNG. -// The entropy pool starts out empty and needs to fill up before the PRNG -// can be used securely. Once the pool is filled, it never dries up again; -// its contents is stirred and reused when necessary. -// -// OpenSSL normally fills the pool automatically but not when someone starts -// generating random numbers before the pool is full: in that case OpenSSL -// keeps lowering the entropy estimate to thwart attackers trying to guess -// the initial state of the PRNG. -// -// When that happens, we will have to wait until enough entropy is available. -// That should normally never take longer than a few milliseconds. -// -// OpenSSL draws from /dev/random and /dev/urandom. While /dev/random may -// block pending "true" randomness, /dev/urandom is a CSPRNG that doesn't -// block under normal circumstances. -// -// The only time when /dev/urandom may conceivably block is right after boot, -// when the whole system is still low on entropy. That's not something we can -// do anything about. -void CheckEntropy(); +struct CSPRNGResult { + const bool ok; + MUST_USE_RESULT bool is_ok() const { return ok; } + MUST_USE_RESULT bool is_err() const { return !ok; } +}; -// Generate length bytes of random data. If this returns false, the data -// might not be random at all and should not be used. -bool EntropySource(unsigned char* buffer, size_t length); +// Either succeeds with exactly |length| bytes of cryptographically +// strong pseudo-random data, or fails. This function may block. +// Don't assume anything about the contents of |buffer| on error. +// As a special case, |length == 0| can be used to check if the CSPRNG +// is properly seeded without consuming entropy. +MUST_USE_RESULT CSPRNGResult CSPRNG(void* buffer, size_t length); int PasswordCallback(char* buf, int size, int rwflag, void* u); diff --git a/src/inspector_io.cc b/src/inspector_io.cc index 7f52fc6059..7e0b3ea63c 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -46,8 +46,7 @@ std::string ScriptPath(uv_loop_t* loop, const std::string& script_name) { // Used ver 4 - with numbers std::string GenerateID() { uint16_t buffer[8]; - CHECK(crypto::EntropySource(reinterpret_cast(buffer), - sizeof(buffer))); + CHECK(crypto::CSPRNG(buffer, sizeof(buffer)).is_ok()); char uuid[256]; snprintf(uuid, sizeof(uuid), "%04x%04x-%04x-%04x-%04x-%04x%04x%04x", diff --git a/src/node.cc b/src/node.cc index 4e48a3ade9..cf840fd106 100644 --- a/src/node.cc +++ b/src/node.cc @@ -913,11 +913,10 @@ std::unique_ptr InitializeOncePerProcess( // fipsinstall command. If the path to this file is incorrect no error // will be reported. // - // For Node.js this will mean that EntropySource will be called by V8 as - // part of its initialization process, and EntropySource will in turn call - // CheckEntropy. CheckEntropy will call RAND_status which will now always - // return 0, leading to an endless loop and the node process will appear to - // hang/freeze. + // For Node.js this will mean that CSPRNG() will be called by V8 as + // part of its initialization process, and CSPRNG() will in turn call + // call RAND_status which will now always return 0, leading to an endless + // loop and the node process will appear to hang/freeze. // Passing NULL as the config file will allow the default openssl.cnf file // to be loaded, but the default section in that file will not be used, @@ -975,11 +974,17 @@ std::unique_ptr InitializeOncePerProcess( return result; } - // Seed V8's PRNG from OpenSSL's pool. This is recommended by V8 and a - // potentially better source of randomness than what V8 uses by default, but - // it does not guarantee that pseudo-random values produced by V8 will be - // cryptograhically secure. - V8::SetEntropySource(crypto::EntropySource); + // Ensure CSPRNG is properly seeded. + CHECK(crypto::CSPRNG(nullptr, 0).is_ok()); + + V8::SetEntropySource([](unsigned char* buffer, size_t length) { + // V8 falls back to very weak entropy when this function fails + // and /dev/urandom isn't available. That wouldn't be so bad if + // the entropy was only used for Math.random() but it's also used for + // hash table and address space layout randomization. Better to abort. + CHECK(crypto::CSPRNG(buffer, length).is_ok()); + return true; + }); #endif // HAVE_OPENSSL && !defined(OPENSSL_IS_BORINGSSL) }