chrome.platformKeys: Require a algorithm name to getKeyPair().

chrome/renderer/resources/extensions/platform_keys/get_public_key.js

Index: chrome/renderer/resources/extensions/platform_keys/get_public_key.js
diff --git a/chrome/renderer/resources/extensions/platform_keys/get_public_key.js b/chrome/renderer/resources/extensions/platform_keys/get_public_key.js
index 290d8f8dfd3cdec6902ad33d0c7095f4de124c1a..4803f2d201ed6851cb86a9370014b06717d34043 100644
--- a/chrome/renderer/resources/extensions/platform_keys/get_public_key.js
+++ b/chrome/renderer/resources/extensions/platform_keys/get_public_key.js
@@ -7,44 +7,50 @@ var internalAPI = require('platformKeys.internalAPI');
 var normalizeAlgorithm =
     requireNative('platform_keys_natives').NormalizeAlgorithm;
 
-function combineAlgorithms(algorithm, importParams) {
-  if (importParams.name === undefined) {
-    importParams.name = algorithm.name;
+function normalizeImportParams(importParams) {
+  if (!importParams.name) {
+    throw new Error('Algorithm: name: Missing or not a string');

[Andrew T Wilson (Slow), 2015/03/19 07:31:49]

I guess some other code is checking if the supplied name was a string? Because
this code doesn't check this (will happily accept importParams.name === 1.

[pneubeck (no reviews), 2015/03/23 13:20:59]

Done.

   }
 
-  // Verify whether importParams.hash equals
+  // Check whether importParams.hash equals
   //   { name: 'none' }
+  // and temporarily replace it by a valid WebCrypto Hash for normalization.
+  var hashIsNone = false;
   if (importParams.hash &&
       importParams.hash.name.toLowerCase() === 'none') {

[Andrew T Wilson (Slow), 2015/03/19 07:31:49]

Also, note that there's kind of an implicit contract in this function that the
passed-in importParams object won't be modified. But if you pass in hash =
{name: none} this contract is violated.

Can you copy importParams before modifying it and just modify the copy?

[Andrew T Wilson (Slow), 2015/03/19 07:31:49]

SO, if I pass in :

importParams.hash = { name: 'none', foobar: 'philipp' }

This code will happily pass this through and change it to {name: 'none'}

Is that really what you want? I wonder if what you should do instead is:

if (hash.name.toLowerCase() === 'none') {
  hashIsNone = true;
  hash.name = 'SHA-1';
}

normalizeAlgorithm(...)
if (hashIsNone) {
   hash.name = 'none';
}

That way you still get normalization/rejection of any extraneous attributes?

[pneubeck (no reviews), 2015/03/23 13:20:59]

Done.

[pneubeck (no reviews), 2015/03/23 13:20:59]

yes. extraneous attributes shall be removed. I added documentation for that.

-    if (Object.keys(importParams.hash).length != 1 ||
-        Object.keys(importParams).length != 2) {
-      // 'name' must be the only hash property in this case.
-      throw new Error('A required parameter was missing or out-of-range');
-    }
-    importParams.hash.name = 'none';
-  } else {
-    // Otherwise apply WebCrypto's algorithm normalization.
-    importParams = normalizeAlgorithm(importParams, 'ImportKey');
-    if (!importParams) {
-      //    throw CreateSyntaxError();
-      throw new Error('A required parameter was missing or out-of-range');
-    }
+    hashIsNone = true;
+    // Will be reverted to 'none' after normalization.
+    importParams.hash = { name: 'SHA-1' };
+  }
+
+  // Apply WebCrypto's algorithm normalization.
+  importParams = normalizeAlgorithm(importParams, 'ImportKey');
+  if (!importParams) {
+    throw new Error('A required parameter was missing or out-of-range');
+  }
+  if (hashIsNone) {
+    importParams.hash = { name: 'none' };
   }
+  return importParams;
+}
 
+function combineAlgorithms(algorithm, importParams) {
   // internalAPI.getPublicKey returns publicExponent as ArrayBuffer, but it
   // should be a Uint8Array.
   if (algorithm.publicExponent) {
     algorithm.publicExponent = new Uint8Array(algorithm.publicExponent);
   }
 
-  for (var key in importParams) {
-    algorithm[key] = importParams[key];
-  }
-
+  algorithm.hash = importParams.hash;
   return algorithm;
 }
 
 function getPublicKey(cert, importParams, callback) {
+  importParams = normalizeImportParams(importParams);
+  // TODO(pneubeck): pass importParams.name to the internal getPublicKey and
+  // verify on the C++ side that the requested algorithm is compatible with the
+  // given SubjectPublicKeyInfo of the certificate.
+  // https://crbug.com/466584
   internalAPI.getPublicKey(cert, function(publicKey, algorithm) {
     var combinedAlgorithm = combineAlgorithms(algorithm, importParams);
     callback(publicKey, combinedAlgorithm);