From 12a2465a9d263f5005c42fcac147b0bde62fd707 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Mon, 23 Aug 2021 12:34:30 +0000
Subject: [PATCH] crypto: add RSA-PSS params to asymmetricKeyDetails

Fixes: https://github.com/nodejs/node/issues/39837
Refs: https://github.com/openssl/openssl/pull/10568

PR-URL: https://github.com/nodejs/node/pull/39851
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
---
 doc/api/crypto.md                             | 16 +++-
 src/crypto/crypto_rsa.cc                      | 82 ++++++++++++++++++-
 src/env.h                                     |  3 +
 test/fixtures/keys/Makefile                   |  8 ++
 .../rsa_pss_private_2048_sha1_sha1_20.pem     | 28 +++++++
 .../keys/rsa_pss_public_2048_sha1_sha1_20.pem |  9 ++
 test/parallel/test-crypto-key-objects.js      | 52 ++++++++++++
 test/parallel/test-crypto-keygen.js           | 10 ++-
 8 files changed, 200 insertions(+), 8 deletions(-)
 create mode 100644 test/fixtures/keys/rsa_pss_private_2048_sha1_sha1_20.pem
 create mode 100644 test/fixtures/keys/rsa_pss_public_2048_sha1_sha1_20.pem

diff --git a/doc/api/crypto.md b/doc/api/crypto.md
index 8318b20ca3003f..ae38a31beb09eb 100644
--- a/doc/api/crypto.md
+++ b/doc/api/crypto.md
@@ -1908,11 +1908,20 @@ const {
 ### `keyObject.asymmetricKeyDetails`
 <!-- YAML
 added: v15.7.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/39851
+    description: Expose `RSASSA-PSS-params` sequence parameters
+                 for RSA-PSS keys.
 -->
 
 * {Object}
   * `modulusLength`: {number} Key size in bits (RSA, DSA).
   * `publicExponent`: {bigint} Public exponent (RSA).
+  * `hashAlgorithm`: {string} Name of the message digest (RSA-PSS).
+  * `mgf1HashAlgorithm`: {string} Name of the message digest used by
+    MGF1 (RSA-PSS).
+  * `saltLength`: {number} Minimal salt length in bytes (RSA-PSS).
   * `divisorLength`: {number} Size of `q` in bits (DSA).
   * `namedCurve`: {string} Name of the curve (EC).
 
@@ -1921,8 +1930,11 @@ this object contains information about the key. None of the information obtained
 through this property can be used to uniquely identify a key or to compromise
 the security of the key.
 
-RSA-PSS parameters, DH, or any future key type details might be exposed via this
-API using additional attributes.
+For RSA-PSS keys, if the key material contains a `RSASSA-PSS-params` sequence,
+the `hashAlgorithm`, `mgf1HashAlgorithm`, and `saltLength` properties will be
+set.
+
+Other key details might be exposed via this API using additional attributes.
 
 ### `keyObject.asymmetricKeyType`
 <!-- YAML
diff --git a/src/crypto/crypto_rsa.cc b/src/crypto/crypto_rsa.cc
index 5bbeb01ab58ac7..1bbf9a1753e4e2 100644
--- a/src/crypto/crypto_rsa.cc
+++ b/src/crypto/crypto_rsa.cc
@@ -547,10 +547,84 @@ Maybe<bool> GetRsaKeyDetail(
       reinterpret_cast<unsigned char*>(public_exponent.data());
   CHECK_EQ(BN_bn2binpad(e, data, len), len);
 
-  return target->Set(
-      env->context(),
-      env->public_exponent_string(),
-      public_exponent.ToArrayBuffer());
+  if (target
+          ->Set(
+              env->context(),
+              env->public_exponent_string(),
+              public_exponent.ToArrayBuffer())
+          .IsNothing()) {
+    return Nothing<bool>();
+  }
+
+  if (type == EVP_PKEY_RSA_PSS) {
+    // Due to the way ASN.1 encoding works, default values are omitted when
+    // encoding the data structure. However, there are also RSA-PSS keys for
+    // which no parameters are set. In that case, the ASN.1 RSASSA-PSS-params
+    // sequence will be missing entirely and RSA_get0_pss_params will return
+    // nullptr. If parameters are present but all parameters are set to their
+    // default values, an empty sequence will be stored in the ASN.1 structure.
+    // In that case, RSA_get0_pss_params does not return nullptr but all fields
+    // of the returned RSA_PSS_PARAMS will be set to nullptr.
+
+    const RSA_PSS_PARAMS* params = RSA_get0_pss_params(rsa);
+    if (params != nullptr) {
+      int hash_nid = NID_sha1;
+      int mgf_nid = NID_mgf1;
+      int mgf1_hash_nid = NID_sha1;
+      int64_t salt_length = 20;
+
+      if (params->hashAlgorithm != nullptr) {
+        hash_nid = OBJ_obj2nid(params->hashAlgorithm->algorithm);
+      }
+
+      if (target
+              ->Set(
+                  env->context(),
+                  env->hash_algorithm_string(),
+                  OneByteString(env->isolate(), OBJ_nid2ln(hash_nid)))
+              .IsNothing()) {
+        return Nothing<bool>();
+      }
+
+      if (params->maskGenAlgorithm != nullptr) {
+        mgf_nid = OBJ_obj2nid(params->maskGenAlgorithm->algorithm);
+        if (mgf_nid == NID_mgf1) {
+          mgf1_hash_nid = OBJ_obj2nid(params->maskHash->algorithm);
+        }
+      }
+
+      // If, for some reason, the MGF is not MGF1, then the MGF1 hash function
+      // is intentionally not added to the object.
+      if (mgf_nid == NID_mgf1) {
+        if (target
+                ->Set(
+                    env->context(),
+                    env->mgf1_hash_algorithm_string(),
+                    OneByteString(env->isolate(), OBJ_nid2ln(mgf1_hash_nid)))
+                .IsNothing()) {
+          return Nothing<bool>();
+        }
+      }
+
+      if (params->saltLength != nullptr) {
+        if (ASN1_INTEGER_get_int64(&salt_length, params->saltLength) != 1) {
+          ThrowCryptoError(env, ERR_get_error(), "ASN1_INTEGER_get_in64 error");
+          return Nothing<bool>();
+        }
+      }
+
+      if (target
+              ->Set(
+                  env->context(),
+                  env->salt_length_string(),
+                  Number::New(env->isolate(), static_cast<double>(salt_length)))
+              .IsNothing()) {
+        return Nothing<bool>();
+      }
+    }
+  }
+
+  return Just<bool>(true);
 }
 
 namespace RSAAlg {
diff --git a/src/env.h b/src/env.h
index 8760bc4361f199..4fd5be8e15029b 100644
--- a/src/env.h
+++ b/src/env.h
@@ -268,6 +268,7 @@ constexpr size_t kFsStatsBufferLength =
   V(gid_string, "gid")                                                         \
   V(h2_string, "h2")                                                           \
   V(handle_string, "handle")                                                   \
+  V(hash_algorithm_string, "hashAlgorithm")                                    \
   V(help_text_string, "helpText")                                              \
   V(homedir_string, "homedir")                                                 \
   V(host_string, "host")                                                       \
@@ -316,6 +317,7 @@ constexpr size_t kFsStatsBufferLength =
   V(message_port_string, "messagePort")                                        \
   V(message_string, "message")                                                 \
   V(messageerror_string, "messageerror")                                       \
+  V(mgf1_hash_algorithm_string, "mgf1HashAlgorithm")                           \
   V(minttl_string, "minttl")                                                   \
   V(module_string, "module")                                                   \
   V(modulus_string, "modulus")                                                 \
@@ -385,6 +387,7 @@ constexpr size_t kFsStatsBufferLength =
   V(replacement_string, "replacement")                                         \
   V(require_string, "require")                                                 \
   V(retry_string, "retry")                                                     \
+  V(salt_length_string, "saltLength")                                          \
   V(scheme_string, "scheme")                                                   \
   V(scopeid_string, "scopeid")                                                 \
   V(serial_number_string, "serialNumber")                                      \
diff --git a/test/fixtures/keys/Makefile b/test/fixtures/keys/Makefile
index 1c6c9c520870ae..a825cace1bf7bb 100644
--- a/test/fixtures/keys/Makefile
+++ b/test/fixtures/keys/Makefile
@@ -64,9 +64,11 @@ all: \
   rsa_pss_private_2048.pem \
   rsa_pss_private_2048_sha256_sha256_16.pem \
   rsa_pss_private_2048_sha512_sha256_20.pem \
+  rsa_pss_private_2048_sha1_sha1_20.pem \
   rsa_pss_public_2048.pem \
   rsa_pss_public_2048_sha256_sha256_16.pem \
   rsa_pss_public_2048_sha512_sha256_20.pem \
+  rsa_pss_public_2048_sha1_sha1_20.pem \
   ed25519_private.pem \
   ed25519_public.pem \
   x25519_private.pem \
@@ -708,6 +710,9 @@ rsa_pss_private_2048_sha256_sha256_16.pem:
 rsa_pss_private_2048_sha512_sha256_20.pem:
 	openssl genpkey -algorithm RSA-PSS -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 -pkeyopt rsa_pss_keygen_md:sha512 -pkeyopt rsa_pss_keygen_mgf1_md:sha256 -pkeyopt rsa_pss_keygen_saltlen:20 -out rsa_pss_private_2048_sha512_sha256_20.pem
 
+rsa_pss_private_2048_sha1_sha1_20.pem:
+	openssl genpkey -algorithm RSA-PSS -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537 -pkeyopt rsa_pss_keygen_md:sha1 -pkeyopt rsa_pss_keygen_mgf1_md:sha1 -pkeyopt rsa_pss_keygen_saltlen:20 -out rsa_pss_private_2048_sha1_sha1_20.pem
+
 rsa_pss_public_2048.pem: rsa_pss_private_2048.pem
 	openssl pkey -in rsa_pss_private_2048.pem -pubout -out rsa_pss_public_2048.pem
 
@@ -717,6 +722,9 @@ rsa_pss_public_2048_sha256_sha256_16.pem: rsa_pss_private_2048_sha256_sha256_16.
 rsa_pss_public_2048_sha512_sha256_20.pem: rsa_pss_private_2048_sha512_sha256_20.pem
 	openssl pkey -in rsa_pss_private_2048_sha512_sha256_20.pem -pubout -out rsa_pss_public_2048_sha512_sha256_20.pem
 
+rsa_pss_public_2048_sha1_sha1_20.pem: rsa_pss_private_2048_sha1_sha1_20.pem
+	openssl pkey -in rsa_pss_private_2048_sha1_sha1_20.pem -pubout -out rsa_pss_public_2048_sha1_sha1_20.pem
+
 ed25519_private.pem:
 	openssl genpkey -algorithm ED25519 -out ed25519_private.pem
 
diff --git a/test/fixtures/keys/rsa_pss_private_2048_sha1_sha1_20.pem b/test/fixtures/keys/rsa_pss_private_2048_sha1_sha1_20.pem
new file mode 100644
index 00000000000000..6b92715eba15db
--- /dev/null
+++ b/test/fixtures/keys/rsa_pss_private_2048_sha1_sha1_20.pem
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvQIBADANBgkqhkiG9w0BAQowAASCBKcwggSjAgEAAoIBAQCpdutzsPFQ1100
+ouR5aAwYry8aAtG0c+zX9UqNXGCpRDWzPPpXHUZSB1BmTTL4EhK2tkAfblYNqzRu
+CAYlKHbFpFLs2zLEorfp0WsFNPaBHE9JHpLIM4oXxPCUypZ7JAn56ZYonYCZ8Il5
+8SzD9aoF41RTEmpcx3XkL2RQa022RiSccYZKx/yzskUUAdTvTvYyujH1MkvsfVP+
+Ns5bRL6IVqowFd3xv6ctvfQMxz0rltgTC+wOm3CFtn+G63y6P/Z0U2DRdacsNkN6
+PFGXAIB0kSvKzs8gVocEBiSwMkcT/KD3R68PY18b2auqaGcm8gA+gaVJ36KAW4dO
+AjbY+YitAgMBAAECggEAfPvfFXln0Ra1gE+vMDdjzITPuWBg57Uj9fbMIEwEYnKT
+JHmRrNRDe9Y3HuxK7hjuQmFSE5xdzUD6rzgtyBP63TOfkV7tJ4dXGxS/2JxCPeDy
+PNxWp18Ttwoh4as0pudikDYN8DCRm3eC/TO5r2EtH6CVHZuUZI8bTMsDMiihrQ8F
+B8+KucBG5DDy/OlDeieAZxZA4Y0/c+W0DNZ/LIPGwaqMzYCSZJXyV0t33HytUwM2
+QZ+RbWqcUcrCI3lFAO8IyEULCi+RnSByZeJ0xwUkdQTI5jT6+G8BrO70Oiab8g+Q
+Rx2s7PxWpIMVS7/JD1PsL4hLrVh3uqh8PZl3/FG9IQKBgQDZWkOR2LA+ixmD6XJb
+Q+7zW2guHnK6wDrQFKmBGLaDdAER64WL1Unt6Umu7FPxth2niYMEgRexBgnj5hQN
+LfPYTiIeXs5ErrU96fVQABsV0Hra1M2Rhve5nynjFFpbHjDXtizzLpE30MsC7YkN
+EqD4YYzjWHrbk/UlQ7tx3eAvtQKBgQDHmNM4TRuyH2yaYxDqnho6fgJv7Z4KgbM0
+1wcUxi5kPDQsFtaVOzFhNserzsWvotQjLkC2+CK5qlCdm59ZlpUqszF6+YyUs5Gq
+WmHdqryduT1VxSV/pd6wGEQo27fxFV7LsT1JhVMh9Iri8MK0b1BD6+kVUf5NcKDB
+Od2o8A1gGQKBgA5Y3Pj1mrymJesFL91CYLWDpR7WN7CIG9m8Y2v4G6QVtjRenZQb
+YiPoMErxoqDj6pUyiIl1lADFa0W13ED6dYwjrDDhBTCXb7NEjELZnvATsOhc/6zJ
+gfSowvUQVN6K4aJ7jgAHZOKQT7ZDw7YvMpzyo4AmSQXRgG8TR34+rRu5AoGACApP
+9+SjSPmbFl0HQWw9Aj4xOvEHfMTcwzQmRN/23nLOZzhETJ6lzpS2VmVt8TVN9lzW
+nohAXdpOhQrP0HwQZjfxtlJ3J0ZUh9g8OQG3t2LO5bWbXRkBb3aKyFqRflSuDOaG
+4X9NagC/14R7U2loglPuf71d0SDIWQBLvZJt94ECgYEAnY7aKHnWdLszcB8uyEkJ
+EJkUEaa+K/nTqOzqffZ01cTWJmUG7a2KuvQ+UQM2BHk2+wBmUo45Iz/dyePOJY0B
+Fu2agiV4+R4z2XVQnIvXgY5HaPxvLz0THksY/pD58gBmFaLMx4ADEwQ+s4Y2g12H
+ABsKNRHfSnKTwOm/dYvcVqs=
+-----END PRIVATE KEY-----
diff --git a/test/fixtures/keys/rsa_pss_public_2048_sha1_sha1_20.pem b/test/fixtures/keys/rsa_pss_public_2048_sha1_sha1_20.pem
new file mode 100644
index 00000000000000..68231d60c168b3
--- /dev/null
+++ b/test/fixtures/keys/rsa_pss_public_2048_sha1_sha1_20.pem
@@ -0,0 +1,9 @@
+-----BEGIN PUBLIC KEY-----
+MIIBIjANBgkqhkiG9w0BAQowAAOCAQ8AMIIBCgKCAQEAqXbrc7DxUNddNKLkeWgM
+GK8vGgLRtHPs1/VKjVxgqUQ1szz6Vx1GUgdQZk0y+BIStrZAH25WDas0bggGJSh2
+xaRS7NsyxKK36dFrBTT2gRxPSR6SyDOKF8TwlMqWeyQJ+emWKJ2AmfCJefEsw/Wq
+BeNUUxJqXMd15C9kUGtNtkYknHGGSsf8s7JFFAHU7072Mrox9TJL7H1T/jbOW0S+
+iFaqMBXd8b+nLb30DMc9K5bYEwvsDptwhbZ/hut8uj/2dFNg0XWnLDZDejxRlwCA
+dJErys7PIFaHBAYksDJHE/yg90evD2NfG9mrqmhnJvIAPoGlSd+igFuHTgI22PmI
+rQIDAQAB
+-----END PUBLIC KEY-----
diff --git a/test/parallel/test-crypto-key-objects.js b/test/parallel/test-crypto-key-objects.js
index aa03a0379a291a..c2c47a9ce72f12 100644
--- a/test/parallel/test-crypto-key-objects.js
+++ b/test/parallel/test-crypto-key-objects.js
@@ -581,11 +581,21 @@ const privateDsa = fixtures.readKey('dsa_private_encrypted_1025.pem',
     const publicKey = createPublicKey(publicPem);
     const privateKey = createPrivateKey(privatePem);
 
+    // Because no RSASSA-PSS-params appears in the PEM, no defaults should be
+    // added for the PSS parameters. This is different from an empty
+    // RSASSA-PSS-params sequence (see test below).
+    const expectedKeyDetails = {
+      modulusLength: 2048,
+      publicExponent: 65537n
+    };
+
     assert.strictEqual(publicKey.type, 'public');
     assert.strictEqual(publicKey.asymmetricKeyType, 'rsa-pss');
+    assert.deepStrictEqual(publicKey.asymmetricKeyDetails, expectedKeyDetails);
 
     assert.strictEqual(privateKey.type, 'private');
     assert.strictEqual(privateKey.asymmetricKeyType, 'rsa-pss');
+    assert.deepStrictEqual(privateKey.asymmetricKeyDetails, expectedKeyDetails);
 
     assert.throws(
       () => publicKey.export({ format: 'jwk' }),
@@ -623,6 +633,38 @@ const privateDsa = fixtures.readKey('dsa_private_encrypted_1025.pem',
     });
   }
 
+  {
+    // This key pair enforces sha1 as the message digest and the MGF1
+    // message digest and a salt length of 20 bytes.
+
+    const publicPem = fixtures.readKey('rsa_pss_public_2048_sha1_sha1_20.pem');
+    const privatePem =
+        fixtures.readKey('rsa_pss_private_2048_sha1_sha1_20.pem');
+
+    const publicKey = createPublicKey(publicPem);
+    const privateKey = createPrivateKey(privatePem);
+
+    // Unlike the previous key pair, this key pair contains an RSASSA-PSS-params
+    // sequence. However, because all values in the RSASSA-PSS-params are set to
+    // their defaults (see RFC 3447), the ASN.1 structure contains an empty
+    // sequence. Node.js should add the default values to the key details.
+    const expectedKeyDetails = {
+      modulusLength: 2048,
+      publicExponent: 65537n,
+      hashAlgorithm: 'sha1',
+      mgf1HashAlgorithm: 'sha1',
+      saltLength: 20
+    };
+
+    assert.strictEqual(publicKey.type, 'public');
+    assert.strictEqual(publicKey.asymmetricKeyType, 'rsa-pss');
+    assert.deepStrictEqual(publicKey.asymmetricKeyDetails, expectedKeyDetails);
+
+    assert.strictEqual(privateKey.type, 'private');
+    assert.strictEqual(privateKey.asymmetricKeyType, 'rsa-pss');
+    assert.deepStrictEqual(privateKey.asymmetricKeyDetails, expectedKeyDetails);
+  }
+
   {
     // This key pair enforces sha256 as the message digest and the MGF1
     // message digest and a salt length of at least 16 bytes.
@@ -681,11 +723,21 @@ const privateDsa = fixtures.readKey('dsa_private_encrypted_1025.pem',
     const publicKey = createPublicKey(publicPem);
     const privateKey = createPrivateKey(privatePem);
 
+    const expectedKeyDetails = {
+      modulusLength: 2048,
+      publicExponent: 65537n,
+      hashAlgorithm: 'sha512',
+      mgf1HashAlgorithm: 'sha256',
+      saltLength: 20
+    };
+
     assert.strictEqual(publicKey.type, 'public');
     assert.strictEqual(publicKey.asymmetricKeyType, 'rsa-pss');
+    assert.deepStrictEqual(publicKey.asymmetricKeyDetails, expectedKeyDetails);
 
     assert.strictEqual(privateKey.type, 'private');
     assert.strictEqual(privateKey.asymmetricKeyType, 'rsa-pss');
+    assert.deepStrictEqual(privateKey.asymmetricKeyDetails, expectedKeyDetails);
 
     // Node.js usually uses the same hash function for the message and for MGF1.
     // However, when a different MGF1 message digest algorithm has been
diff --git a/test/parallel/test-crypto-keygen.js b/test/parallel/test-crypto-keygen.js
index ed5986e6bfd421..3392284f8d1963 100644
--- a/test/parallel/test-crypto-keygen.js
+++ b/test/parallel/test-crypto-keygen.js
@@ -309,14 +309,20 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);
     assert.strictEqual(publicKey.asymmetricKeyType, 'rsa-pss');
     assert.deepStrictEqual(publicKey.asymmetricKeyDetails, {
       modulusLength: 512,
-      publicExponent: 65537n
+      publicExponent: 65537n,
+      hashAlgorithm: 'sha256',
+      mgf1HashAlgorithm: 'sha256',
+      saltLength: 16
     });
 
     assert.strictEqual(privateKey.type, 'private');
     assert.strictEqual(privateKey.asymmetricKeyType, 'rsa-pss');
     assert.deepStrictEqual(privateKey.asymmetricKeyDetails, {
       modulusLength: 512,
-      publicExponent: 65537n
+      publicExponent: 65537n,
+      hashAlgorithm: 'sha256',
+      mgf1HashAlgorithm: 'sha256',
+      saltLength: 16
     });
 
     // Unlike RSA, RSA-PSS does not allow encryption.