From 367e36ae612f6cb2189cf2c5bfd30154e106ab8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 21 Feb 2019 09:28:16 +0100 Subject: [PATCH 1/2] crypto: fix unencrypted DER PKCS8 parsing The previously used OpenSSL call only supports encrypted PKCS8, this commit adds support for unencrypted PKCS8. --- src/node_crypto.cc | 105 +++++++++++++++++----------- src/node_crypto.h | 1 + test/parallel/test-crypto-keygen.js | 67 ++++++++++++++++++ 3 files changed, 134 insertions(+), 39 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index aaba05b984f54f..b4cdeda3620786 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2848,6 +2848,59 @@ static MaybeLocal WritePublicKey(Environment* env, return BIOToStringOrBuffer(env, bio.get(), config.format_); } +static bool IsASN1Sequence(const unsigned char* data, size_t size, + size_t* data_offset, size_t* data_size) { + if (size < 2 || data[0] != 0x30) + return false; + + if (data[1] & 0x80) { + // Long form. + size_t n_bytes = data[1] & ~0x80; + if (n_bytes + 2 > size || n_bytes > sizeof(size_t)) + return false; + size_t i, length = 0; + for (i = 0; i < n_bytes; i++) + length = (length << 8) | data[i + 2]; + *data_offset = 2 + n_bytes; + *data_size = std::min(size - 2 - n_bytes, length); + } else { + // Short form. + *data_offset = 2; + *data_size = std::min(size - 2, data[1]); + } + + return true; +} + +static bool IsRSAPrivateKey(const unsigned char* data, size_t size) { + // Both RSAPrivateKey and RSAPublicKey structures start with a SEQUENCE. + size_t offset, len; + if (!IsASN1Sequence(data, size, &offset, &len)) + return false; + + // An RSAPrivateKey sequence always starts with a single-byte integer whose + // value is either 0 or 1, whereas an RSAPublicKey starts with the modulus + // (which is the product of two primes and therefore at least 4), so we can + // decide the type of the structure based on the first three bytes of the + // sequence. + return len >= 3 && + data[offset] == 2 && + data[offset + 1] == 1 && + !(data[offset + 2] & 0xfe); +} + +static bool IsEncryptedPrivateKeyInfo(const unsigned char* data, size_t size) { + // Both PrivateKeyInfo and EncryptedPrivateKeyInfo start with a SEQUENCE. + size_t offset, len; + if (!IsASN1Sequence(data, size, &offset, &len)) + return false; + + // A PrivateKeyInfo sequence always starts with an integer whereas an + // EncryptedPrivateKeyInfo starts with an AlgorithmIdentifier. + return len >= 1 && + data[offset] != 2; +} + static EVPKeyPointer ParsePrivateKey(const PrivateKeyEncodingConfig& config, const char* key, size_t key_len) { @@ -2873,11 +2926,19 @@ static EVPKeyPointer ParsePrivateKey(const PrivateKeyEncodingConfig& config, BIOPointer bio(BIO_new_mem_buf(key, key_len)); if (!bio) return pkey; - char* pass = const_cast(config.passphrase_.get()); - pkey.reset(d2i_PKCS8PrivateKey_bio(bio.get(), - nullptr, - PasswordCallback, - pass)); + + if (IsEncryptedPrivateKeyInfo( + reinterpret_cast(key), key_len)) { + char* pass = const_cast(config.passphrase_.get()); + pkey.reset(d2i_PKCS8PrivateKey_bio(bio.get(), + nullptr, + PasswordCallback, + pass)); + } else { + PKCS8Pointer p8inf(d2i_PKCS8_PRIV_KEY_INFO_bio(bio.get(), nullptr)); + if (p8inf) + pkey.reset(EVP_PKCS82PKEY(p8inf.get())); + } } else { CHECK_EQ(config.type_.ToChecked(), kKeyEncodingSEC1); const unsigned char* p = reinterpret_cast(key); @@ -3093,40 +3154,6 @@ static ManagedEVPPKey GetPrivateKeyFromJs( } } -static bool IsRSAPrivateKey(const unsigned char* data, size_t size) { - // Both RSAPrivateKey and RSAPublicKey structures start with a SEQUENCE. - if (size >= 2 && data[0] == 0x30) { - size_t offset; - if (data[1] & 0x80) { - // Long form. - size_t n_bytes = data[1] & ~0x80; - if (n_bytes + 2 > size || n_bytes > sizeof(size_t)) - return false; - size_t i, length = 0; - for (i = 0; i < n_bytes; i++) - length = (length << 8) | data[i + 2]; - offset = 2 + n_bytes; - size = std::min(size, length + 2); - } else { - // Short form. - offset = 2; - size = std::min(size, data[1] + 2); - } - - // An RSAPrivateKey sequence always starts with a single-byte integer whose - // value is either 0 or 1, whereas an RSAPublicKey starts with the modulus - // (which is the product of two primes and therefore at least 4), so we can - // decide the type of the structure based on the first three bytes of the - // sequence. - return size - offset >= 3 && - data[offset] == 2 && - data[offset + 1] == 1 && - !(data[offset + 2] & 0xfe); - } - - return false; -} - static ManagedEVPPKey GetPublicOrPrivateKeyFromJs( const FunctionCallbackInfo& args, unsigned int* offset, diff --git a/src/node_crypto.h b/src/node_crypto.h index 7c346a6c1435d1..e9862ff1bc5879 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -77,6 +77,7 @@ using BIOPointer = DeleteFnPtr; using SSLCtxPointer = DeleteFnPtr; using SSLSessionPointer = DeleteFnPtr; using SSLPointer = DeleteFnPtr; +using PKCS8Pointer = DeleteFnPtr; using EVPKeyPointer = DeleteFnPtr; using EVPKeyCtxPointer = DeleteFnPtr; using EVPMDPointer = DeleteFnPtr; diff --git a/test/parallel/test-crypto-keygen.js b/test/parallel/test-crypto-keygen.js index ebbac7606f4230..7b3eee570ddf40 100644 --- a/test/parallel/test-crypto-keygen.js +++ b/test/parallel/test-crypto-keygen.js @@ -174,6 +174,73 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher); testEncryptDecrypt(publicKey, key); testSignVerify(publicKey, key); })); + + // Now do the same with an encrypted private key, but encoded as DER. + generateKeyPair('rsa', { + publicExponent: 0x10001, + modulusLength: 512, + publicKeyEncoding, + privateKeyEncoding: { + type: 'pkcs8', + format: 'der', + cipher: 'aes-256-cbc', + passphrase: 'secret' + } + }, common.mustCall((err, publicKeyDER, privateKeyDER) => { + assert.ifError(err); + + assert(Buffer.isBuffer(publicKeyDER)); + assertApproximateSize(publicKeyDER, 74); + + assert(Buffer.isBuffer(privateKeyDER)); + + // Since the private key is encrypted, signing shouldn't work anymore. + const publicKey = { key: publicKeyDER, ...publicKeyEncoding }; + assert.throws(() => { + testSignVerify(publicKey, { + key: privateKeyDER, + format: 'der', + type: 'pkcs8' + }); + }, /bad decrypt|asn1 encoding routines/); + + const privateKey = { + key: privateKeyDER, + format: 'der', + type: 'pkcs8', + passphrase: 'secret' + }; + testEncryptDecrypt(publicKey, privateKey); + testSignVerify(publicKey, privateKey); + })); + + // Now do the same with an encrypted private key, but encoded as DER. + generateKeyPair('rsa', { + publicExponent: 0x10001, + modulusLength: 512, + publicKeyEncoding, + privateKeyEncoding: { + type: 'pkcs8', + format: 'der' + } + }, common.mustCall((err, publicKeyDER, privateKeyDER) => { + assert.ifError(err); + + assert(Buffer.isBuffer(publicKeyDER)); + assertApproximateSize(publicKeyDER, 74); + + assert(Buffer.isBuffer(privateKeyDER)); + + const publicKey = { key: publicKeyDER, ...publicKeyEncoding }; + const privateKey = { + key: privateKeyDER, + format: 'der', + type: 'pkcs8', + passphrase: 'secret' + }; + testEncryptDecrypt(publicKey, privateKey); + testSignVerify(publicKey, privateKey); + })); } { From c10f406388f4f7dcb99139d3c128c7dcfd37e932 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 21 Feb 2019 17:12:08 +0100 Subject: [PATCH 2/2] fixup! crypto: fix unencrypted DER PKCS8 parsing --- src/node_crypto.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b4cdeda3620786..db21916f68dcf2 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2858,8 +2858,8 @@ static bool IsASN1Sequence(const unsigned char* data, size_t size, size_t n_bytes = data[1] & ~0x80; if (n_bytes + 2 > size || n_bytes > sizeof(size_t)) return false; - size_t i, length = 0; - for (i = 0; i < n_bytes; i++) + size_t length = 0; + for (size_t i = 0; i < n_bytes; i++) length = (length << 8) | data[i + 2]; *data_offset = 2 + n_bytes; *data_size = std::min(size - 2 - n_bytes, length);