crypto: don't crash on unknown asymmetricKeyType

PR-URL: #26786 Fixes: #26775 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
nodejs · Apr 1, 2019 · 7c1fc93 · 7c1fc93
1 parent 66b9536
commit 7c1fc93
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 5 deletions.
diff --git a/doc/api/crypto.md b/doc/api/crypto.md
@@ -1129,18 +1129,23 @@ passing keys as strings or `Buffer`s due to improved security features.
 <!-- YAML
 added: v11.6.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/26786
+    description: This property now returns `undefined` for KeyObject
+                 instances of unrecognized type instead of aborting.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/26774
     description: Added support for `'x25519'` and `'x448'`
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/26319
-    description: Added support for `'ed25519'` and `'ed448'`
+    description: Added support for `'ed25519'` and `'ed448'`.
 -->
 * {string}
 
 For asymmetric keys, this property represents the type of the embedded key
 (`'rsa'`, `'dsa'`, `'ec'`, `'ed25519'`, `'ed448'`, `'x25519'` or `'x448'`).
-This property is `undefined` for symmetric keys.
+This property is `undefined` for unrecognized `KeyObject` types and symmetric
+keys.
 
 ### keyObject.export([options])
 <!-- YAML

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -3690,7 +3690,7 @@ void KeyObject::InitPrivate(const ManagedEVPPKey& pkey) {
   this->asymmetric_key_ = pkey;
 }
 
-Local<String> KeyObject::GetAsymmetricKeyType() const {
+Local<Value> KeyObject::GetAsymmetricKeyType() const {
   CHECK_NE(this->key_type_, kKeyTypeSecret);
   switch (EVP_PKEY_id(this->asymmetric_key_.get())) {
   case EVP_PKEY_RSA:
@@ -3708,7 +3708,7 @@ Local<String> KeyObject::GetAsymmetricKeyType() const {
   case EVP_PKEY_X448:
     return env()->crypto_x448_string();
   default:
-    CHECK(false);
+    return Undefined(env()->isolate());
   }
 }
 

diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -470,7 +470,7 @@ class KeyObject : public BaseObject {
 
   static void GetAsymmetricKeyType(
       const v8::FunctionCallbackInfo<v8::Value>& args);
-  v8::Local<v8::String> GetAsymmetricKeyType() const;
+  v8::Local<v8::Value> GetAsymmetricKeyType() const;
 
   static void GetSymmetricKeySize(
       const v8::FunctionCallbackInfo<v8::Value>& args);

diff --git a/test/fixtures/test_unknown_privkey.pem b/test/fixtures/test_unknown_privkey.pem
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEugIBADALBgkqhkiG9w0BAQoEggSmMIIEogIBAAKCAQEAwMSNbT9SbSHvXmPt
+j1x2Ipk1tUM06301UD91xGcA0232zrIQcKjsPM7bE6YXN0zRxfLJUqalewCk80Ct
+6V+E5XtMHUFQt1Ne8HW9U930KnfnQEyU8UwRPoWWeZQhs+sa8ZfggtfN7gq4/wiS
+KFNNtSJb24NKoLis31P0nILGC4/JewgE0QaFUoOL+Oc3dMhwWg9/H64sSjhI/SGW
+9Sv3M6WcSn7vQCe8oM2vslf3Xm8rHNqZMlXujs7zhRtcr5alKz9BwMJIoGouQrk9
+9cgupdYsddgNh2bC4TQR9BMKHj8tV5Uf3Pbf4EoZOFffCbyBZxmKtsYsmhh2FDLA
+RzNhKwIDAQABAoIBAHBRlj4ziSmBfmG3Q/ImY8chEkQ9lpYn7GqHr2zyv25yQj6J
+Tj72jj+YH9pBCoH0Rr5aCqgX5Y/X/kSmSS8TsvGrd9wL9KX88/KUB+7YAq7EEoBK
+nvZB5kJRwC2y/DhDIv3mCrDyYVDz+nrPWaoZb8u861zqEQ+4yzGNT5fqMs8Ewm8A
+hxg3GA2R6FC2CymZO884XOxlVac6SNURfA+U+xrcMIXbXpok2Z5eh/kMOeIKwmL0
+QEO0U6DEnZm4rJjywu8fEkKbX00YfaDQaiGzRZfvmzkTPIQemXPWARdIvFtJU8Fx
+OWWeMumJD1KiU9ISW4e76l7F8UOviT6jEg9rxFECgYEA96WCEIB+O4aO7+s56kOv
+vQkEXn959lz7e++S9AV3R19PpBCh50l5v9NSjGQlA4FU4AdBB5EmiX/bLZRHFwHI
+KLDsMFuq9id3OPHYIzFP4YjVHTGRPZToJHwy4ePIdZEaeJHY39EEz8oHsSSJlLdm
+o0417RsFAfApW2VN63c3JFcCgYEAx0Um/ATsT4ELguVQ+XlquLQdS12XS7zjcwWv
+PL8UyooSxcjcbLcJB6DRWXM0NOry7KPUCIF4m3KSjIZypV/v2KVFPCfD3vxZcdB7
+xgccqXJMUx7MSs9AMZXTtv5hG7RS5z+ig7Yi/6nzBm21jKYKbFDbqfq8MSfiR6cT
+KjR+RU0CgYAm/iFnlcPKfZpd/mylDTlLi3Lrqii6+NMEJam+0GmCjGhOzeugLjqE
+ULLLtiz5y1Bg4eOEXH9z4PTSzWkQH1Czz3+w8Y4OqhIknjfI+se4HEJqEVbsGlke
+/YtJdAMpN8qyN0ytmQyn5wilBLrA9surZPIqvjlgn77zTBUjwSamiwKBgAqIVS8s
+83CgWYNpq4YELOfmXUYGhGC0czE5M7H6R5cNBUD/BOeaJRgKIAaiWDgT0xM+9Y4d
+icptm+Fhmd2z3HGPCsHLOEco/3FMm74z0ggCypX6IsIxgiscyDv75hYYyej/LA/a
+KK9qxDWqxtXQUOy4uWOapSfT+9ndst2gOKxhAoGAVFcfedCLxummgTtZE91n59pL
+TWTk4GgYpWyv6XbHjYrFW2y18qmn0hmEpO+440So0NmGGDtNnPYNUKY/MPjHScwC
+FoZMFqqnkmshXz0uDx3gMQK2JDmdF+s3VwZq4Rtb3NJ9v4/WMgWftxaUpAm1/aRC
+IHc67mAAez4i8fg2wTQ=
+-----END PRIVATE KEY-----
diff --git a/test/parallel/test-crypto-key-objects.js b/test/parallel/test-crypto-key-objects.js
@@ -178,6 +178,13 @@ const privateDsa = fixtures.readKey('dsa_private_encrypted_1025.pem',
   });
 }
 
+{
+  // This should not cause a crash: https://github.com/nodejs/node/pull/26786
+  const pem = fixtures.readSync('test_unknown_privkey.pem', 'ascii');
+  const key = createPrivateKey(pem);
+  assert.strictEqual(key.asymmetricKeyType, undefined);
+}
+
 [
   { private: fixtures.readSync('test_ed25519_privkey.pem', 'ascii'),
     public: fixtures.readSync('test_ed25519_pubkey.pem', 'ascii'),