[webcrypto] Add JWK import for HMAC and AES-CBC key.

content/renderer/webcrypto/webcrypto_impl.cc

Index: content/renderer/webcrypto/webcrypto_impl.cc
diff --git a/content/renderer/webcrypto/webcrypto_impl.cc b/content/renderer/webcrypto/webcrypto_impl.cc
index 9275783bab77ce6fcec25fb69656a4203c834a7d..e51d9a39beee137f4934afc51d179822ddd3d703 100644
--- a/content/renderer/webcrypto/webcrypto_impl.cc
+++ b/content/renderer/webcrypto/webcrypto_impl.cc
@@ -4,35 +4,127 @@
 
 #include "content/renderer/webcrypto/webcrypto_impl.h"
 
+#include <algorithm>
+#include <functional>
+#include <map>
+#include "base/json/json_reader.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
-#include "third_party/WebKit/public/platform/WebArrayBuffer.h"
+#include "base/strings/string_piece.h"
+#include "base/values.h"
+#include "content/renderer/webcrypto/webcrypto_util.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
+#include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 #include "third_party/WebKit/public/platform/WebCryptoKey.h"
 
 namespace content {
 
+namespace {
+
+// Binds a specific key length value to a compatible factory function.
+typedef WebKit::WebCryptoAlgorithm (*AlgFactoryFuncWithOneShortArg)(
+    unsigned short);
+template <AlgFactoryFuncWithOneShortArg func, unsigned short key_length>
+WebKit::WebCryptoAlgorithm BindAlgFactoryWithKeyLen() {
+  return func(key_length);
+}
+
+// Binds a WebCryptoAlgorithmId value to a compatible factory function.
+typedef WebKit::WebCryptoAlgorithm (*AlgFactoryFuncWithWebCryptoAlgIdArg)(
+    WebKit::WebCryptoAlgorithmId);
+template <AlgFactoryFuncWithWebCryptoAlgIdArg func,
+          WebKit::WebCryptoAlgorithmId algorithm_id>
+WebKit::WebCryptoAlgorithm BindAlgFactoryAlgorithmId() {
+  return func(algorithm_id);
+}
+
+// Defines a map between a JWK 'alg' string ID and a corresponding Web Crypto
+// factory function.
+typedef WebKit::WebCryptoAlgorithm (*AlgFactoryFuncNoArgs)();
+typedef std::map<std::string, AlgFactoryFuncNoArgs> JwkAlgFactoryMap;
+
+class JwkAlgorithmFactoryMap {
+ public:
+  JwkAlgorithmFactoryMap() {
+    map_["HS256"] =
+        &BindAlgFactoryWithKeyLen<CreateHmacAlgorithmByDigestLen, 256>;
+    map_["HS256"] =
+        &BindAlgFactoryWithKeyLen<CreateHmacAlgorithmByDigestLen, 256>;
+    map_["HS384"] =
+        &BindAlgFactoryWithKeyLen<CreateHmacAlgorithmByDigestLen, 384>;
+    map_["HS384"] =
+        &BindAlgFactoryWithKeyLen<CreateHmacAlgorithmByDigestLen, 512>;
+    map_["RS256"] =
+        &BindAlgFactoryAlgorithmId<CreateRsaSsaAlgorithm,
+                                   WebKit::WebCryptoAlgorithmIdSha256>;
+    map_["RS384"] =
+        &BindAlgFactoryAlgorithmId<CreateRsaSsaAlgorithm,
+                                   WebKit::WebCryptoAlgorithmIdSha384>;
+    map_["RS512"] =
+        &BindAlgFactoryAlgorithmId<CreateRsaSsaAlgorithm,
+                                   WebKit::WebCryptoAlgorithmIdSha512>;
+    map_["RSA1_5"] = &CreateRsaEsAlgorithm;
+    map_["RSA-OAEP"] =
+        &BindAlgFactoryAlgorithmId<CreateRsaOaepAlgorithm,
+                                   WebKit::WebCryptoAlgorithmIdSha1>;
+    map_["A128KW"] = &WebKit::WebCryptoAlgorithm::createNull;
+    map_["A256KW"] = &WebKit::WebCryptoAlgorithm::createNull;
+    map_["A128GCM"] =
+        &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 256>;

[eroman, 2013/11/01 20:59:58]

Shouldn't this be 128?

[padolph, 2013/11/01 23:08:50]

Done.

+    map_["A256GCM"] =
+        &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 256>;
+    map_["A128CBC"] =
+        &BindAlgFactoryWithKeyLen<CreateAesCbcKeyGenAlgorithm, 128>;
+    map_["A256CBC"] =
+        &BindAlgFactoryWithKeyLen<CreateAesCbcKeyGenAlgorithm, 256>;
+    map_["A384CBC"] =
+        &BindAlgFactoryWithKeyLen<CreateAesCbcKeyGenAlgorithm, 384>;
+    map_["A512CBC"] =
+        &BindAlgFactoryWithKeyLen<CreateAesCbcKeyGenAlgorithm, 512>;
+  }
+  WebKit::WebCryptoAlgorithm CreateAlgorithmFromName(

[eroman, 2013/11/01 20:59:58]

[optional] Can you add a newlinea above this?

[padolph, 2013/11/01 23:08:50]

Done.

+      const std::string& alg_id) {
+    const JwkAlgFactoryMap::const_iterator pos = map_.find(alg_id);
+    if (pos == map_.end())
+      return WebKit::WebCryptoAlgorithm::createNull();
+    return pos->second();
+  }
+
+ private:
+  JwkAlgFactoryMap map_;
+};
+base::LazyInstance<JwkAlgorithmFactoryMap> jwk_alg_factory =

[eroman, 2013/11/01 20:59:58]

ditto on newline.

[padolph, 2013/11/01 23:08:50]

Done.

+    LAZY_INSTANCE_INITIALIZER;
+
+// TODO(padolph): Verify this logic is sufficient to judge algorithm
+// "consistency" for JWK import, and for all supported algorithms.
+bool WebCryptoAlgorithmsConsistent(const WebKit::WebCryptoAlgorithm& lhs,

[eroman, 2013/11/01 20:59:58]

Rather than "lhs" and "rhs" I suggest something more general like "a" and "b" or
"agl1" / "alg2".

[padolph, 2013/11/01 23:08:50]

Done.

+                                   const WebKit::WebCryptoAlgorithm& rhs) {
+  DCHECK(!lhs.isNull());
+  DCHECK(!rhs.isNull());
+  if (lhs.id() == rhs.id()) {
+    if (lhs.id() == WebKit::WebCryptoAlgorithmIdHmac ||
+        lhs.id() == WebKit::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 ||
+        lhs.id() == WebKit::WebCryptoAlgorithmIdRsaOaep) {
+      if (WebCryptoAlgorithmsConsistent(GetInnerHashAlgorithm(lhs),
+                                        GetInnerHashAlgorithm(rhs))) {
+        return true;
+      }
+      return false;
+    }
+    return true;
+  }
+  return false;
+}
+
+}  // namespace
+
 WebCryptoImpl::WebCryptoImpl() {
   Init();
 }
 
-// static
-// TODO(eroman): This works by re-allocating a new buffer. It would be better if
-//               the WebArrayBuffer could just be truncated instead.
-void WebCryptoImpl::ShrinkBuffer(
-    WebKit::WebArrayBuffer* buffer,
-    unsigned new_size) {
-  DCHECK_LE(new_size, buffer->byteLength());
-
-  if (new_size == buffer->byteLength())
-    return;
-
-  WebKit::WebArrayBuffer new_buffer =
-      WebKit::WebArrayBuffer::create(new_size, 1);
-  DCHECK(!new_buffer.isNull());
-  memcpy(new_buffer.data(), buffer->data(), new_size);
-  *buffer = new_buffer;
-}
+WebCryptoImpl::~WebCryptoImpl() {}
 
 // static
 // TODO(eroman): Expose functionality in Blink instead.
@@ -115,15 +207,25 @@ void WebCryptoImpl::importKey(
     WebKit::WebCryptoKeyUsageMask usage_mask,
     WebKit::WebCryptoResult result) {
   WebKit::WebCryptoKey key = NullKey();
-  if (!ImportKeyInternal(format,
-                         key_data,
-                         key_data_size,
-                         algorithm_or_null,
-                         extractable,
-                         usage_mask,
-                         &key)) {
-    result.completeWithError();
-    return;
+  if (format == WebKit::WebCryptoKeyFormatJwk) {
+    if (!ImportKeyJwk(key_data,
+                      key_data_size,
+                      algorithm_or_null,
+                      extractable,
+                      usage_mask,
+                      &key)) {
+      result.completeWithError();
+    }
+  } else {
+    if (!ImportKeyInternal(format,
+                           key_data,
+                           key_data_size,
+                           algorithm_or_null,
+                           extractable,
+                           usage_mask,
+                           &key)) {
+      result.completeWithError();
+    }
   }
   DCHECK(key.handle());
   DCHECK(!key.algorithm().isNull());
@@ -169,4 +271,228 @@ void WebCryptoImpl::verifySignature(
   }
 }
 
+bool WebCryptoImpl::ImportKeyJwk(
+    const unsigned char* key_data,
+    unsigned key_data_size,
+    const WebKit::WebCryptoAlgorithm& input_algorithm,
+    bool extractable,
+    WebKit::WebCryptoKeyUsageMask usage_mask,
+    WebKit::WebCryptoKey* key) {
+
+  // JSON Web Key Format (JWK)
+  // http://tools.ietf.org/html/draft-ietf-jose-json-web-key-16
+  // TODO(padolph): Not all possible values are handled by this code right now
+  //
+  // A JWK is a simple JSON dictionary with the following entries
+  // - "kty" (Key Type) Parameter, REQUIRED
+  // - <kty-specific parameters, see below>, REQUIRED
+  // - "use" (Key Use) Parameter, OPTIONAL
+  // - "alg" (Algorithm) Parameter, OPTIONAL
+  // - "extractable" (Key Exportability), OPTIONAL [NOTE: not yet part of JOSE]

[eroman, 2013/11/01 20:59:58]

Is there a bug number that can be linked to here? Whatever we add should be on
the standards track.

[padolph, 2013/11/01 23:08:50]

Not that I know of. Do you know how we can follow up with the effort to add
'extractable' to the JWT spec?

+  // (all other entries are ignored)
+  //
+  // Input key_data contains the JWK. To build a Web Crypto Key, the JWK values
+  // are parsed out and used as follows:
+  // Web Crypto Key type            <-- (deduced)
+  // Web Crypto Key extractable     <-- extractable
+  // Web Crypto Key algorithm       <-- alg
+  // Web Crypto Key keyUsage        <-- usage
+  // Web Crypto Key keying material <-- kty-specific parameters
+  //
+  // Values for each entry are case-sensitive and defined in
+  // http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-16.
+  // Note that not all values specified by JOSE are handled by this code. Only
+  // handled values are listed.
+  // - kty (Key Type)
+  //   +-------+--------------------------------------------------------------+
+  //   | "RSA" | RSA [RFC3447]                                                |
+  //   | "oct" | Octet sequence (used to represent symmetric keys)            |
+  //   +-------+--------------------------------------------------------------+
+  // - use (Key Use)
+  //   +-------+--------------------------------------------------------------+
+  //   | "enc" | encrypt and decrypt operations                               |
+  //   | "sig" | sign and verify (MAC) operations                             |
+  //   | "wrap"| key wrap and unwrap [not yet part of JOSE]                   |
+  //   +-------+--------------------------------------------------------------+
+  // - extractable (Key Exportability)
+  //   +-------+--------------------------------------------------------------+
+  //   | true  | Key may be exported from the trusted environment             |
+  //   | false | Key cannot exit the trusted environment                      |
+  //   +-------+--------------------------------------------------------------+
+  // - alg (Algorithm)
+  //   See http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-16
+  //   +--------------+-------------------------------------------------------+
+  //   | Digital Signature or MAC Algorithm                                   |
+  //   +--------------+-------------------------------------------------------+
+  //   | "HS256"      | HMAC using SHA-256 hash algorithm                     |
+  //   | "HS384"      | HMAC using SHA-384 hash algorithm                     |
+  //   | "HS512"      | HMAC using SHA-512 hash algorithm                     |
+  //   | "RS256"      | RSASSA using SHA-256 hash algorithm                   |
+  //   | "RS384"      | RSASSA using SHA-384 hash algorithm                   |
+  //   | "RS512"      | RSASSA using SHA-512 hash algorithm                   |
+  //   +--------------+-------------------------------------------------------|
+  //   | Key Management Algorithm                                             |
+  //   +--------------+-------------------------------------------------------+
+  //   | "RSA1_5"     | RSAES-PKCS1-V1_5 [RFC3447]                            |
+  //   | "RSA-OAEP"   | RSAES using Optimal Asymmetric Encryption Padding     |
+  //   |              | (OAEP) [RFC3447], with the default parameters         |
+  //   |              | specified by RFC3447 in Section A.2.1                 |
+  //   | "A128KW"     | Advanced Encryption Standard (AES) Key Wrap Algorithm |
+  //   |              | [RFC3394] using 128 bit keys                          |
+  //   | "A256KW"     | AES Key Wrap Algorithm using 256 bit keys             |
+  //   | "A128GCM"    | AES in Galois/Counter Mode (GCM) [NIST.800-38D] using |
+  //   |              | 128 bit keys                                          |
+  //   | "A256GCM"    | AES GCM using 256 bit keys                            |
+  //   | "A128CBC"    | AES in Cipher Block Chaining Mode (CBC) with PKCS #5  |
+  //   |              | padding [NIST.800-38A] [not yet part of JOSE]         |
+  //   | "A256CBC"    | AES CBC using 256 bit keys [not yet part of JOSE]     |
+  //   | "A384CBC"    | AES CBC using 384 bit keys [not yet part of JOSE]     |
+  //   | "A512CBC"    | AES CBC using 512 bit keys [not yet part of JOSE]     |
+  //   +--------------+-------------------------------------------------------+
+  //
+  // kty-specific parameters
+  // The value of kty determines the type and content of the keying material
+  // carried in the JWK to be imported. Currently only two possibilities are
+  // supported: a raw key or an RSA public key. RSA private keys are not
+  // supported because typical applications seldom need to import a private key,
+  // and the large number of JWK parameters required to describe one.
+  // - kty == "oct" (symmetric or other raw key)
+  //   +-------+--------------------------------------------------------------+
+  //   | "k"   | Contains the value of the symmetric (or other single-valued) |
+  //   |       | key.  It is represented as the base64url encoding of the     |
+  //   |       | octet sequence containing the key value.                     |
+  //   +-------+--------------------------------------------------------------+
+  // - kty == "RSA" (RSA public key)
+  //   +-------+--------------------------------------------------------------+
+  //   | "n"   | Contains the modulus value for the RSA public key.  It is    |
+  //   |       | represented as the base64url encoding of the value's         |
+  //   |       | unsigned big endian representation as an octet sequence.     |
+  //   +-------+--------------------------------------------------------------+
+  //   | "e"   | Contains the exponent value for the RSA public key.  It is   |
+  //   |       | represented as the base64url encoding of the value's         |
+  //   |       | unsigned big endian representation as an octet sequence.     |
+  //   +-------+--------------------------------------------------------------+
+  //
+  // Conflict resolution

[eroman, 2013/11/01 20:59:58]

Just wanted to re-iterate, I found the comments for this giant comment block to
be very helpful!

[padolph, 2013/11/01 23:08:50]

:-) They did take a while to type in. I needed a central reference for the
implementation. Glad you find it useful.

+  // The input_algorithm, extractable, and usage_mask input parameters may be
+  // different from similar values inside the JWK. The Web Crypto spec says that
+  // if a JWK value is present but is inconsistent with the input value, it is
+  // an error and the operation must fail. Conversely, should they be missing
+  // from the JWK, the input values should be used to "fill-in" for the
+  // corresponding JWK-optional values (use, alg, extractable).
+
+  DCHECK(key);
+
+  // Parse the incoming JWK JSON.
+  if (!key_data || !key_data_size)
+    return false;
+  base::StringPiece json_string(reinterpret_cast<const char*>(key_data),
+                                key_data_size);
+  scoped_ptr<base::Value> value(base::JSONReader::Read(json_string));
+  // Note, bare pointer dict_value is ok since it points into scoped value.
+  base::DictionaryValue* dict_value = NULL;
+  if (!value.get() || !value->GetAsDictionary(&dict_value) || !dict_value)
+    return false;
+
+  // JWK "kty". Exit early if this required JWK parameter is missing.
+  std::string jwk_kty_value;
+  if (!dict_value->GetString("kty", &jwk_kty_value))
+    return false;
+
+  // JWK "extractable" (optional) --> extractable parameter
+  bool jwk_extractable_value;

[eroman, 2013/11/01 20:59:58]

Paranoia: I would hate for future code to reference jwk_extractable_value and
result in an access to uninitialized value.

Could you put this in its own block to reduce the scope of this variable:
   {
      bool jwk_extractable_value;
      if (dict_value->GetBoolean(..
   }

[padolph, 2013/11/01 23:08:50]

Done.

+  if (dict_value->GetBoolean("extractable", &jwk_extractable_value)) {

[eroman, 2013/11/01 20:59:58]

Should we be doing stronger error checking here?

What if a JWK contains:

    extractable: "false"
or
    extractable: 0

In this case there is a type mismatch from what we are expecting (boolean), but
by treating it as an optional parameter means we silently discard the
information. Which could be a security problem for the caller.

This same question applies to the other properties.

[Ryan Sleevi, 2013/11/01 22:45:14]

This, I think, highlights some of the issues with "JWK-in-the-web". It means
that JSON.parse() in ES will yield a different object than, say, this reader,
because of the interpretations / conversions made. Same for conceptually mapping
a JWK into WebIDL, which also has language bindings issues.

Definitely, the JWK/JWT spec needs to be unambiguously clear on this, and given
that I have seen no standardization efforts or discussions regarding
'extractable' (other than the JOSE group rejecting it), I do think we need to be
extremely careful for the open web platform.

[padolph, 2013/11/01 23:08:50]

What is the best way to move this forward?

+    if (jwk_extractable_value != extractable)
+      return false;
+  }
+
+  // JWK "alg" (optional) --> algorithm parameter
+  // Note: input algorithm is also optional, so we have six cases to handle.
+  // 1. JWK alg present but unrecognized: error
+  // 2. JWK alg valid AND input algorithm isNull: use JWK value
+  // 3. JWK alg valid AND input algorithm specified, but JWK value
+  //      inconsistent with input: error
+  // 4. JWK alg valid AND input algorithm specified, both consistent: use
+  //      input value (because it has potentially more details)
+  // 5. JWK alg missing AND input algorithm isNull: error
+  // 6. JWK alg missing AND input algorithm specified: use input value
+  WebKit::WebCryptoAlgorithm algorithm =
+      WebKit::WebCryptoAlgorithm::createNull();
+  std::string jwk_alg_value;
+  if (dict_value->GetString("alg", &jwk_alg_value)) {
+    // JWK alg present
+    const WebKit::WebCryptoAlgorithm jwk_algorithm =
+        jwk_alg_factory.Get().CreateAlgorithmFromName(jwk_alg_value);
+    if (jwk_algorithm.isNull()) {
+      // JWK alg unrecognized
+      return false;  // case 1
+    }
+    // JWK alg valid
+    if (input_algorithm.isNull()) {
+      // input algorithm not specified
+      algorithm = jwk_algorithm;  // case 2
+    } else {
+      // input algorithm specified
+      if (!WebCryptoAlgorithmsConsistent(jwk_algorithm, input_algorithm))
+        return false;               // case 3
+      algorithm = input_algorithm;  // case 4
+    }
+  } else {
+    // JWK alg missing
+    if (input_algorithm.isNull())
+      return false;               // case 5
+    algorithm = input_algorithm;  // case 6
+  }
+

[eroman, 2013/11/01 20:59:58]

Purely for readability, I suggest adding here:

  DCHECK(!algorithm.isNull());

[padolph, 2013/11/01 23:08:50]

Done.

+  // JWK "use" (optional) --> usage_mask parameter
+  std::string jwk_use_value;
+  if (dict_value->GetString("use", &jwk_use_value)) {
+    unsigned jwk_usage_mask;

[eroman, 2013/11/01 20:59:58]

Please use the type as WebKit::WebCryptoKeyUsageMask instead of "unsigned". Also
can you defensively initialize it to 0?

[padolph, 2013/11/01 23:08:50]

Done.

+    if (jwk_use_value == "enc") {
+      jwk_usage_mask =
+          WebKit::WebCryptoKeyUsageEncrypt | WebKit::WebCryptoKeyUsageDecrypt;
+    } else if (jwk_use_value == "sig") {
+      jwk_usage_mask =
+          WebKit::WebCryptoKeyUsageSign | WebKit::WebCryptoKeyUsageVerify;
+    } else if (jwk_use_value == "wrap") {
+      jwk_usage_mask =
+          WebKit::WebCryptoKeyUsageWrapKey | WebKit::WebCryptoKeyUsageUnwrapKey;
+    } else {
+      return false;
+    }
+    if ((jwk_usage_mask & usage_mask) != usage_mask) {

[eroman, 2013/11/01 20:59:58]

Side comment (on spec): having to duplicate specify things as the caller
certainly seems inconvenient. Perhaps the usage should be optional as well
allowing importKey to just use what JWK had. Or jwk import use a different
function.

[padolph, 2013/11/01 23:08:50]

I totally agree. Especially if this JWK comes from inside an opaque JWE received
from a remote peer, and the protocol in use does not assume the receiver has any
a. priori knowledge.
One thing we don't want to allow is for the key to be used for an operation
different for what it was intended for when the JWK was built.

For optional input, what about a special case for when usage_mask is 0, take the
jwk value? I think this might require a WC spec change.

+      // A usage_mask must be a subset of jwk_usage_mask.
+      return false;
+    }
+  }
+
+  // JWK keying material --> ImportKeyInternal()
+  if (jwk_kty_value == "oct") {
+    std::string jwk_k_value_url64;
+    if (!dict_value->GetString("k", &jwk_k_value_url64))
+      return false;
+    std::string jwk_k_value;
+    if (!Base64DecodeUrlSafe(jwk_k_value_url64, &jwk_k_value) ||
+        !jwk_k_value.size()) {
+      return false;
+    }
+    if (!ImportKeyInternal(WebKit::WebCryptoKeyFormatRaw,
+                           reinterpret_cast<const uint8*>(jwk_k_value.data()),
+                           jwk_k_value.size(),
+                           algorithm,
+                           extractable,
+                           usage_mask,
+                           key)) {
+      return false;

[eroman, 2013/11/01 20:59:58]

What do you think about making this:

return ImportKeyInternal(....);

[padolph, 2013/11/01 23:08:50]

Done.

+    }
+  } else if (jwk_kty_value == "RSA") {
+    // TODO(padolph): JWK import RSA public key
+    return false;
+  } else {
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace content