diff --git a/node.gyp b/node.gyp index 347d82d2e1e100..8f131acf661b8e 100644 --- a/node.gyp +++ b/node.gyp @@ -683,6 +683,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 7eab9de37cb3a1..e70ddc64f3685b 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 af489967144d32..8def58a5f10cd9 100644 --- a/src/crypto/crypto_keygen.cc +++ b/src/crypto/crypto_keygen.cc @@ -81,7 +81,13 @@ KeyGenJobStatus SecretKeyGenTraits::DoKeyGen( SecretKeyGenConfig* params) { CHECK_LE(params->length, INT_MAX); params->out = MallocOpenSSL(params->length); - EntropySource(reinterpret_cast(params->out), params->length); + if (CSPRNG(reinterpret_cast(params->out), + params->length).is_err()) { + OPENSSL_clear_free(params->out, params->length); + params->out = nullptr; + params->length = 0; + return KeyGenJobStatus::FAILED; + } return KeyGenJobStatus::OK; } diff --git a/src/crypto/crypto_keygen.h b/src/crypto/crypto_keygen.h index ed7d5c0601fff1..1a933a247800d6 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 d0736a9cf1277a..2f9e9aacb1e652 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 805f8978b2ad1b..e878c5ea15d58f 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -60,26 +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; - - // 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(); - // RAND_bytes() can return 0 to indicate that the entropy data is not truly - // random. That's okay, it's still better than V8's stock source of entropy, - // which is /dev/urandom on UNIX platforms and the current time on Windows. - return RAND_bytes(buffer, length) != -1; +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()); + + 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 4afae1884fe40e..dc3bb15cfb48a8 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(); - -// Generate length bytes of random data. If this returns false, the data -// may not be truly random but it's still generally good enough. -bool EntropySource(unsigned char* buffer, size_t length); +struct CSPRNGResult { + const bool ok; + MUST_USE_RESULT bool is_ok() const { return ok; } + MUST_USE_RESULT bool is_err() const { return !ok; } +}; + +// 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 7f52fc605933da..7e0b3ea63cff25 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 71b3f7dcfe3cdd..75423472f0f301 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1056,11 +1056,10 @@ InitializationResult 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, @@ -1105,19 +1104,28 @@ InitializationResult InitializeOncePerProcess( OPENSSL_init(); } #endif - if (!crypto::ProcessFipsOptions()) { + if (!crypto::ProcessFipsOptions()) { result.exit_code = ERR_GET_REASON(ERR_peek_error()); result.early_return = true; fprintf(stderr, "OpenSSL error when trying to enable FIPS:\n"); ERR_print_errors_fp(stderr); return result; - } + } - // V8 on Windows doesn't have a good source of entropy. Seed it from - // OpenSSL's pool. - 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) -} + } + per_process::v8_platform.Initialize( static_cast(per_process::cli_options->v8_thread_pool_size)); if (init_flags & kInitializeV8) {