Various improvements related to PR #11705

nodejs · tniessen · Mar 5, 2017 · Mar 7, 2017 · Mar 7, 2017 · Mar 8, 2017
commit e467227106c975ff65d079afcae3f42429b85a6a
diff --git a/doc/api/crypto.md b/doc/api/crypto.md
@@ -961,7 +961,7 @@ pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==
 console.log(sign.sign(privateKey).toString('hex'));
 ```
 
-### sign.sign(private_key[, output_format][, options])
+### sign.sign(private_key[, output_format])
 <!-- YAML
 added: v0.1.92
 -->
@@ -975,24 +975,20 @@ Calculates the signature on all the data passed through using either
 
 The `private_key` argument can be an object or a string. If `private_key` is a
 string, it is treated as a raw key with no passphrase. If `private_key` is an
-object, it is interpreted as a hash containing two properties:
+object, it is interpreted as a hash containing some of these properties:
 
-* `key`: {string} - PEM encoded private key
+* `key`: {string} - PEM encoded private key (required)
 * `passphrase`: {string} - passphrase for the private key
+* `padding`: {String} - RSA padding, either `RSA_PKCS1_PADDING` (default) or
+  `RSA_PKCS1_PSS_PADDING`
+* `saltLength`: {number} - salt length for RSASSA-PSS. The special value
+  `RSA_PSS_SALTLEN_DIGEST` sets the salt length to the digest size,
+  `RSA_PSS_SALTLEN_MAX_SIGN` sets it to the maximum permissible value.
 
 The `output_format` can specify one of `'latin1'`, `'hex'` or `'base64'`. If
 `output_format` is provided a string is returned; otherwise a [`Buffer`][] is
 returned.
 
-The optional `options` argument is an object which specifies additional
-cryptographic parameters. Currently, the following options are supported:
-
-* `padding` : {String} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
-  (default) or `'pss'` for RSASSA-PSS
-* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
-  the salt length will be set to the digest size. If this is set to `-2`
-  (default), the salt length will be set to the maximum permissible value.
-
 The `Sign` object can not be again used after `sign.sign()` method has been
 called. Multiple calls to `sign.sign()` will result in an error being thrown.
 
@@ -1079,7 +1075,7 @@ then `input_encoding` is ignored.
 
 This can be called many times with new data as it is streamed.
 
-### verifier.verify(object, signature[, signature_format][, options])
+### verifier.verify(object, signature[, signature_format])
 <!-- YAML
 added: v0.1.92
 -->
@@ -1088,24 +1084,24 @@ added: v0.1.92
 - `signature_format` {string}
 
 Verifies the provided data using the given `object` and `signature`.
-The `object` argument is a string containing a PEM encoded object, which can be
-an RSA public key, a DSA public key, or an X.509 certificate.
-The `signature` argument is the previously calculated signature for the data, in
-the `signature_format` which can be `'latin1'`, `'hex'` or `'base64'`.
-If a `signature_format` is specified, the `signature` is expected to be a
-string; otherwise `signature` is expected to be a [`Buffer`][] or
-`Uint8Array`.
-
-The optional `options` argument is an object which specifies additional
-cryptographic parameters. Currently, the following options are supported:
+The `object` argument can be either a string containing a PEM encoded object,
+which can be an RSA public key, a DSA public key, or an X.509 certificate,
+or an object with some of the following properties:
+>>>>>>> Various improvements related to PR #11705
 
-* `padding` : {String} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
+* `key`: {string} - PEM encoded private key (required)
+* `padding`: {string} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
   (default) or `'pss'` for RSASSA-PSS
-* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
+* `saltLength`: {number} - salt length for RSASSA-PSS. If this is set to `-1`,
   the salt length will be set to the digest size. A value of `-2` (default)
   causes the salt length to be automatically determined based on the PSS block
   structure.
 
+The `signature` argument is the previously calculated signature for the data, in
+the `signature_format` which can be `'latin1'`, `'hex'` or `'base64'`.
+If a `signature_format` is specified, the `signature` is expected to be a
+string; otherwise `signature` is expected to be a [`Buffer`][].
+
 Returns `true` or `false` depending on the validity of the signature for
 the data and public key.
 
@@ -2063,6 +2059,18 @@ the `crypto`, `tls`, and `https` modules and are generally specific to OpenSSL.
     <td><code>RSA_PKCS1_PSS_PADDING</code></td>
     <td></td>
   </tr>
+  <tr>
+    <td><code>RSA_PSS_SALTLEN_DIGEST</code></td>
+    <td></td>
+  </tr>
+  <tr>
+    <td><code>RSA_PSS_SALTLEN_MAX_SIGN</code></td>
+    <td></td>
+  </tr>
+  <tr>
+    <td><code>RSA_PSS_SALTLEN_AUTO</code></td>
+    <td></td>
+  </tr>
   <tr>
     <td><code>POINT_CONVERSION_COMPRESSED</code></td>
     <td></td>

diff --git a/lib/crypto.js b/lib/crypto.js
@@ -297,17 +297,12 @@ Sign.prototype._write = function _write(chunk, encoding, callback) {
 
 Sign.prototype.update = Hash.prototype.update;
 
-Sign.prototype.sign = function sign(privateKey, encoding, options) {
-  if (!privateKey)
+Sign.prototype.sign = function sign(options, encoding) {
+  if (!options)
     throw new Error('No key provided to sign');
 
-  if (typeof encoding == 'object' && !options) {
-    options = encoding;
-    encoding = null;
-  }
-
-  var key = privateKey.key || privateKey;
-  var passphrase = privateKey.passphrase || null;
+  var key = options.key || options;
+  var passphrase = options.passphrase || null;
   var ret = this._handle.sign(toBuf(key), null, passphrase, options);
 
   encoding = encoding || exports.DEFAULT_ENCODING;
@@ -334,16 +329,11 @@ util.inherits(Verify, stream.Writable);
 Verify.prototype._write = Sign.prototype._write;
 Verify.prototype.update = Sign.prototype.update;
 
-Verify.prototype.verify = function verify(object, signature, sigEncoding,
-                                          options) {
-  if (typeof sigEncoding == 'object' && !options) {
-    options = sigEncoding;
-    sigEncoding = null;
-  }
-
+Verify.prototype.verify = function verify(options, signature, sigEncoding) {
+  var key = options.key || options;
   sigEncoding = sigEncoding || exports.DEFAULT_ENCODING;
-  return this._handle.verify(toBuf(object), toBuf(signature, sigEncoding),
-                             null, options);
+  return this._handle.verify(toBuf(key), toBuf(signature, sigEncoding), null,
+                             options);
 };
 
 function rsaPublic(method, defaultPadding) {

diff --git a/src/env.h b/src/env.h
@@ -179,6 +179,7 @@ namespace node {
   V(output_string, "output")                                                  \
   V(order_string, "order")                                                    \
   V(owner_string, "owner")                                                    \
+  V(padding_string, "padding")                                                \
   V(parse_error_string, "Parse Error")                                        \
   V(path_string, "path")                                                      \
   V(pbkdf2_error_string, "PBKDF2 Error")                                      \
@@ -196,6 +197,7 @@ namespace node {
   V(rename_string, "rename")                                                  \
   V(replacement_string, "replacement")                                        \
   V(retry_string, "retry")                                                    \
+  V(salt_length_string, "saltLength")                                         \
   V(serial_string, "serial")                                                  \
   V(scopeid_string, "scopeid")                                                \
   V(sent_shutdown_string, "sentShutdown")                                     \

diff --git a/src/node_constants.cc b/src/node_constants.cc
@@ -997,6 +997,21 @@ void DefineOpenSSLConstants(Local<Object> target) {
     NODE_DEFINE_CONSTANT(target, RSA_PKCS1_PSS_PADDING);
 #endif
 
+#ifndef RSA_PSS_SALTLEN_DIGEST
+#define RSA_PSS_SALTLEN_DIGEST -1
+#endif
+    NODE_DEFINE_CONSTANT(target, RSA_PSS_SALTLEN_DIGEST);
+
+#ifndef RSA_PSS_SALTLEN_MAX_SIGN
+#define RSA_PSS_SALTLEN_MAX_SIGN -2
+#endif
+    NODE_DEFINE_CONSTANT(target, RSA_PSS_SALTLEN_MAX_SIGN);
+
+#ifndef RSA_PSS_SALTLEN_AUTO
+#define RSA_PSS_SALTLEN_AUTO -2
+#endif
+    NODE_DEFINE_CONSTANT(target, RSA_PSS_SALTLEN_AUTO);
+
 #if HAVE_OPENSSL
   // NOTE: These are not defines
   NODE_DEFINE_CONSTANT(target, POINT_CONVERSION_COMPRESSED);

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -3975,34 +3975,29 @@ void SignBase::CheckThrow(SignBase::Error error) {
 
 int SignBase::GetRSAOptions(Environment *env, v8::Local<v8::Object> options,
                             int *padding, int *saltlen) {
-  Local<Value> key = String::NewFromUtf8(env->isolate(), "padding");
-  MaybeLocal<Value> maybePadding = options->Get(key);
-  if (!maybePadding.IsEmpty()) {
-    Local<Value> paddingValue = maybePadding.ToLocalChecked();
-    if (paddingValue->IsString()) {
-      v8::String::Utf8Value paddingUtf8(paddingValue);
-      const char *paddingName = *paddingUtf8;
-      if (strcmp(paddingName, "pkcs1") == 0) {
-        *padding = RSA_PKCS1_PADDING;
-      }
-      else if (strcmp(paddingName, "pss") == 0) {
-        *padding = RSA_PKCS1_PSS_PADDING;
-      }
-      else {
-        env->ThrowError("Padding must be 'pkcs1' or 'pss'");
-        return 0;
-      }
+  MaybeLocal<Value> maybePadding = options->Get(env->padding_string());
+  if (maybePadding.IsEmpty()) return 0;
+
+  Local<Value> paddingValue = maybePadding.ToLocalChecked();
+  if (paddingValue->IsNumber()) {
+    switch (paddingValue->Int32Value()) {
+    case RSA_PKCS1_PADDING:
+    case RSA_PKCS1_PSS_PADDING:
+      *padding = paddingValue->Int32Value();
+      break;
+    default:
+      env->ThrowError("Padding must be RSA_PKCS1_PADDING or RSA_PKCS1_PSS_PADDING");
+      return 0;
     }
   }
 
   if (*padding == RSA_PKCS1_PSS_PADDING) {
-    key = String::NewFromUtf8(env->isolate(), "saltLength");
-    MaybeLocal<Value> maybeSaltlen = options->Get(key);
-    if (!maybeSaltlen.IsEmpty()) {
-      Local<Value> saltlenValue = maybeSaltlen.ToLocalChecked();
-      if (saltlenValue->IsNumber()) {
-        *saltlen = saltlenValue->Int32Value();
-      }
+    MaybeLocal<Value> maybeSaltlen = options->Get(env->salt_length_string());
+    if (maybeSaltlen.IsEmpty()) return 0;
+
+    Local<Value> saltlenValue = maybeSaltlen.ToLocalChecked();
+    if (saltlenValue->IsNumber()) {
+      *saltlen = saltlenValue->Int32Value();
     }
   }
 
@@ -4103,10 +4098,10 @@ static int Node_SignFinal(EVP_MD_CTX *mdctx, unsigned char *md, unsigned int *s,
 
   *s = 0;
   if (!EVP_DigestFinal_ex(mdctx, &(m[0]), &m_len))
-    goto err;
+    return i;
 
   if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
-    size_t sltmp = (size_t)EVP_PKEY_size(pkey);
+    size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
     i = 0;
     pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
     if (!pkctx)
@@ -4116,9 +4111,10 @@ static int Node_SignFinal(EVP_MD_CTX *mdctx, unsigned char *md, unsigned int *s,
     if (pkey->type == EVP_PKEY_RSA || pkey->type == EVP_PKEY_RSA2) {
       if (EVP_PKEY_CTX_set_rsa_padding(pkctx, padding) <= 0)
         goto err;
-      if (padding == RSA_PKCS1_PSS_PADDING)
+      if (padding == RSA_PKCS1_PSS_PADDING) {
         if (EVP_PKEY_CTX_set_rsa_pss_saltlen(pkctx, pss_saltlen) <= 0)
           goto err;
+      }
     }
     if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx->digest) <= 0)
       goto err;
@@ -4237,8 +4233,9 @@ void Sign::SignFinal(const FunctionCallbackInfo<Value>& args) {
 
   int padding = RSA_PKCS1_PADDING;
   int saltlen = -2;
-  if (len >= 4 && args[3]->IsObject()) {
-    sign->GetRSAOptions(env, Local<Object>::Cast(args[3]), &padding, &saltlen);
+  if (args[3]->IsObject()) {
+    if (!sign->GetRSAOptions(env, Local<Object>::Cast(args[3]), &padding, &saltlen))
+      return;
   }
 
   md_len = 8192;  // Maximum key size is 8192 bits
@@ -4427,9 +4424,10 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
   if (pkey->type == EVP_PKEY_RSA || pkey->type == EVP_PKEY_RSA2) {
     if (EVP_PKEY_CTX_set_rsa_padding(pkctx, padding) <= 0)
       goto err;
-    if (padding == RSA_PKCS1_PSS_PADDING)
+    if (padding == RSA_PKCS1_PSS_PADDING) {
       if (EVP_PKEY_CTX_set_rsa_pss_saltlen(pkctx, saltlen) <= 0)
         goto err;
+    }
   }
   if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx_.digest) <= 0)
     goto err;
@@ -4500,7 +4498,8 @@ void Verify::VerifyFinal(const FunctionCallbackInfo<Value>& args) {
   int padding = RSA_PKCS1_PADDING;
   int saltlen = -2;
   if (args.Length() >= 4 && args[3]->IsObject()) {
-    verify->GetRSAOptions(env, args[3]->ToObject(), &padding, &saltlen);
+    if (!verify->GetRSAOptions(env, args[3]->ToObject(), &padding, &saltlen))
+      return;
   }
 
   bool verify_result;

diff --git a/test/parallel/test-crypto-sign-verify.js b/test/parallel/test-crypto-sign-verify.js
@@ -79,17 +79,30 @@ const keyPem = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii');
       // Test sign and verify with the given parameters
       const s4 = crypto.createSign(algo)
                        .update('Test123')
-                       .sign(keyPem, null, { padding: 'pss', saltLength });
+                       .sign({
+                         key: keyPem,
+                         padding: crypto.constants.RSA_PKCS1_PSS_PADDING,
+                         saltLength
+                       });
       verified = crypto.createVerify(algo)
                        .update('Test')
                        .update('123')
-                       .verify(certPem, s4, { padding: 'pss', saltLength });
+                       .verify({
+                         key: certPem,
+                         padding: crypto.constants.RSA_PKCS1_PSS_PADDING,
+                         saltLength
+                       }, s4);
       assert.strictEqual(verified, true, 'sign and verify (buffer, PSS)');
 
-      // Setting the salt length to -2 should always work for verification
+      // Setting the salt length to RSA_PSS_SALTLEN_AUTO should always work for
+      // verification
       verified = crypto.createVerify(algo)
                        .update('Test123')
-                       .verify(certPem, s4, { padding: 'pss', saltLength: -2 });
+                       .verify({
+                         key: certPem,
+                         padding: crypto.constants.RSA_PKCS1_PSS_PADDING,
+                         saltLength: crypto.constants.RSA_PSS_SALTLEN_AUTO
+                       }, s4);
       assert.strictEqual(verified, true, 'sign and verify (buffer, PSS)');
     });
   });
@@ -99,8 +112,11 @@ const keyPem = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii');
   assert.throws(() => {
     crypto.createSign('RSA-SHA1')
       .update('Test123')
-      .sign(keyPem, { padding: 'foo' });
-  }, /^Error: Padding must be 'pkcs1' or 'pss'$/);
+      .sign({
+        key: keyPem,
+        padding: crypto.constants.RSA_PKCS1_OAEP_PADDING
+      });
+  }, /^Error: Padding must be RSA_PKCS1_PADDING or RSA_PKCS1_PSS_PADDING$/);
 }
 
 // Test throws exception when key options is null