Skip to content

Commit

Permalink
crypto: fix weak randomness in WebCrypto keygen
Browse files Browse the repository at this point in the history
Commit dae283d 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: #35093

Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: #346
Backport-PR-URL: #351
CVE-ID: CVE-2022-35255
  • Loading branch information
bnoordhuis authored and RafaelGSS committed Sep 22, 2022
1 parent ffb6f4d commit 0c2a572
Show file tree
Hide file tree
Showing 9 changed files with 67 additions and 78 deletions.
2 changes: 2 additions & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,8 @@
'openssl_default_cipher_list%': '',
},

'cflags': ['-Werror=unused-result'],

'defines': [
'NODE_ARCH="<(target_arch)"',
'NODE_PLATFORM="<(OS)"',
Expand Down
19 changes: 11 additions & 8 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -541,9 +541,9 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& 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");
}
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 7 additions & 1 deletion src/crypto/crypto_keygen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ KeyGenJobStatus SecretKeyGenTraits::DoKeyGen(
SecretKeyGenConfig* params) {
CHECK_LE(params->length, INT_MAX);
params->out = MallocOpenSSL<char>(params->length);
EntropySource(reinterpret_cast<unsigned char*>(params->out), params->length);
if (CSPRNG(reinterpret_cast<unsigned char*>(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;
}

Expand Down
3 changes: 0 additions & 3 deletions src/crypto/crypto_keygen.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@ class KeyGenJob final : public CryptoJob<KeyGenTraits> {
std::move(params)) {}

void DoThreadPoolWork() override {
// Make sure the CSPRNG is properly seeded so the results are secure.
CheckEntropy();

AdditionalParams* params = CryptoJob<KeyGenTraits>::params();

switch (KeyGenTraits::DoKeyGen(AsyncWrap::env(), params)) {
Expand Down
15 changes: 7 additions & 8 deletions src/crypto/crypto_random.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -156,12 +155,12 @@ Maybe<bool> 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(),
Expand Down
28 changes: 8 additions & 20 deletions src/crypto/crypto_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<unsigned char*>(buffer), length))
return {true};
} while (1 == RAND_poll());

return {false};
}

int PasswordCallback(char* buf, int size, int rwflag, void* u) {
Expand Down
37 changes: 12 additions & 25 deletions src/crypto/crypto_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
3 changes: 1 addition & 2 deletions src/inspector_io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<unsigned char*>(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",
Expand Down
30 changes: 19 additions & 11 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<int>(per_process::cli_options->v8_thread_pool_size));
if (init_flags & kInitializeV8) {
Expand Down

0 comments on commit 0c2a572

Please sign in to comment.