crypto: fix webcrypto HMAC "get key length" in deriveKey and generateKey

nodejs · Oct 7, 2022 · f979ef3 · f979ef3
1 parent 4d94be3
commit f979ef3
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 40 deletions.
diff --git a/lib/internal/crypto/mac.js b/lib/internal/crypto/mac.js
@@ -14,7 +14,7 @@ const {
 } = internalBinding('crypto');
 
 const {
-  getHashLength,
+  getBlockSize,
   hasAnyNotIn,
   jobPromise,
   normalizeHashName,
@@ -54,7 +54,7 @@ async function hmacGenerateKey(algorithm, extractable, keyUsages) {
     throw new ERR_MISSING_OPTION('algorithm.hash');
 
   if (length === undefined)
-    length = getHashLength(hash.name);
+    length = getBlockSize(hash.name);
 
   validateBitLength(length, 'algorithm.length', true);
 

diff --git a/lib/internal/crypto/pbkdf2.js b/lib/internal/crypto/pbkdf2.js
@@ -116,23 +116,19 @@ async function pbkdf2DeriveBits(algorithm, baseKey, length) {
 
   const raw = baseKey[kKeyObject].export();
 
-  let byteLength = 64;  // the default
-  if (length !== undefined) {
-    if (length === 0)
-      throw lazyDOMException('length cannot be zero', 'OperationError');
-    if (length === null)
-      throw lazyDOMException('length cannot be null', 'OperationError');
-    validateUint32(length, 'length');
-    if (length % 8) {
-      throw lazyDOMException(
-        'length must be a multiple of 8',
-        'OperationError');
-    }
-    byteLength = length / 8;
+  if (length === 0)
+    throw lazyDOMException('length cannot be zero', 'OperationError');
+  if (length === null)
+    throw lazyDOMException('length cannot be null', 'OperationError');
+  validateUint32(length, 'length');
+  if (length % 8) {
+    throw lazyDOMException(
+      'length must be a multiple of 8',
+      'OperationError');
   }
 
   return new Promise((resolve, reject) => {
-    pbkdf2(raw, salt, iterations, byteLength, hash, (err, result) => {
+    pbkdf2(raw, salt, iterations, length / 8, hash, (err, result) => {
       if (err) return reject(err);
       resolve(result.buffer);
     });

diff --git a/lib/internal/crypto/util.js b/lib/internal/crypto/util.js
@@ -341,12 +341,12 @@ function getUsagesUnion(usageSet, ...usages) {
   return newset;
 }
 
-function getHashLength(name) {
+function getBlockSize(name) {
   switch (name) {
-    case 'SHA-1': return 160;
-    case 'SHA-256': return 256;
-    case 'SHA-384': return 384;
-    case 'SHA-512': return 512;
+    case 'SHA-1': return 512;
+    case 'SHA-256': return 512;
+    case 'SHA-384': return 1024;
+    case 'SHA-512': return 1024;
   }
 }
 
@@ -431,8 +431,8 @@ module.exports = {
   validateMaxBufferLength,
   bigIntArrayToUnsignedBigInt,
   bigIntArrayToUnsignedInt,
+  getBlockSize,
   getStringOption,
   getUsagesUnion,
-  getHashLength,
   secureHeapUsed,
 };
diff --git a/lib/internal/crypto/webcrypto.js b/lib/internal/crypto/webcrypto.js
@@ -56,6 +56,7 @@ const {
 
 const {
   getArrayBufferOrView,
+  getBlockSize,
   hasAnyNotIn,
   lazyRequire,
   normalizeAlgorithm,
@@ -200,16 +201,7 @@ function getKeyLength({ name, length, hash }) {
       return length;
     case 'HMAC':
       if (length === undefined) {
-        switch (hash?.name) {
-          case 'SHA-1':
-            return 160;
-          case 'SHA-256':
-            return 256;
-          case 'SHA-384':
-            return 384;
-          case 'SHA-512':
-            return 512;
-        }
+        return getBlockSize(hash?.name);
       }
 
       if (typeof length === 'number' && length !== 0) {

diff --git a/test/parallel/test-webcrypto-derivekey.js b/test/parallel/test-webcrypto-derivekey.js
@@ -124,10 +124,11 @@ const { webcrypto: { subtle }, KeyObject } = require('crypto');
   const vectors = [
     ['PBKDF2', 'deriveKey', 528],
     ['HKDF', 'deriveKey', 528],
-    [{ name: 'HMAC', hash: 'SHA-1' }, 'sign', 160],
-    [{ name: 'HMAC', hash: 'SHA-256' }, 'sign', 256],
-    [{ name: 'HMAC', hash: 'SHA-384' }, 'sign', 384],
-    [{ name: 'HMAC', hash: 'SHA-512' }, 'sign', 512],
+    [{ name: 'HMAC', hash: 'SHA-1' }, 'sign', 512],
+    [{ name: 'HMAC', hash: 'SHA-256' }, 'sign', 512],
+    // Not long enough secret generated by ECDH
+    // [{ name: 'HMAC', hash: 'SHA-384' }, 'sign', 1024],
+    // [{ name: 'HMAC', hash: 'SHA-512' }, 'sign', 1024],
   ];
 
   (async () => {
@@ -151,6 +152,34 @@ const { webcrypto: { subtle }, KeyObject } = require('crypto');
   })().then(common.mustCall());
 }
 
+{
+  const vectors = [
+    [{ name: 'HMAC', hash: 'SHA-1' }, 'sign', 512],
+    [{ name: 'HMAC', hash: 'SHA-256' }, 'sign', 512],
+    [{ name: 'HMAC', hash: 'SHA-384' }, 'sign', 1024],
+    [{ name: 'HMAC', hash: 'SHA-512' }, 'sign', 1024],
+  ];
+
+  (async () => {
+    for (const [derivedKeyAlgorithm, usage, expected] of vectors) {
+      const derived = await subtle.deriveKey(
+        { name: 'PBKDF2', salt: new Uint8Array([]), hash: 'SHA-256', iterations: 20 },
+        await subtle.importKey('raw', new Uint8Array([]), { name: 'PBKDF2' }, false, ['deriveKey']),
+        derivedKeyAlgorithm,
+        false,
+        [usage]);
+
+      if (derived.algorithm.name === 'HMAC') {
+        assert.strictEqual(derived.algorithm.length, expected);
+      } else {
+        // KDFs cannot be exportable and do not indicate their length
+        const secretKey = KeyObject.from(derived);
+        assert.strictEqual(secretKey.symmetricKeySize, expected / 8);
+      }
+    }
+  })().then(common.mustCall());
+}
+
 // Test X25519 and X448 key derivation
 {
   async function test(name) {

diff --git a/test/parallel/test-webcrypto-keygen.js b/test/parallel/test-webcrypto-keygen.js
@@ -551,10 +551,10 @@ const vectors = {
 
     if (length === undefined) {
       switch (hash) {
-        case 'SHA-1': length = 160; break;
-        case 'SHA-256': length = 256; break;
-        case 'SHA-384': length = 384; break;
-        case 'SHA-512': length = 512; break;
+        case 'SHA-1': length = 512; break;
+        case 'SHA-256': length = 512; break;
+        case 'SHA-384': length = 1024; break;
+        case 'SHA-512': length = 1024; break;
       }
     }