[webcrypto] Validate JWK import of AES keys: key length must match algorithm.

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 4d69f0750d596f10941d99b9ccfac007c1d657cf..e446c5e9e3f71ab52665680928f4f78af1b12274 100644
--- a/content/renderer/webcrypto/webcrypto_impl.cc
+++ b/content/renderer/webcrypto/webcrypto_impl.cc
@@ -38,89 +38,130 @@ bool IsAlgorithmAsymmetric(const blink::WebCryptoAlgorithm& algorithm) {
           algorithm.id() == blink::WebCryptoAlgorithmIdRsaOaep);
 }
 
-// Binds a specific key length value to a compatible factory function.
-typedef blink::WebCryptoAlgorithm (*AlgFactoryFuncWithOneShortArg)(
-    unsigned short);
-template <AlgFactoryFuncWithOneShortArg func, unsigned short key_length>
-blink::WebCryptoAlgorithm BindAlgFactoryWithKeyLen() {
-  return func(key_length);
-}
+typedef blink::WebCryptoAlgorithm (*AlgorithmCreationFunc)();
 
-// Binds a WebCryptoAlgorithmId value to a compatible factory function.
-typedef blink::WebCryptoAlgorithm (*AlgFactoryFuncWithWebCryptoAlgIdArg)(
-    blink::WebCryptoAlgorithmId);
-template <AlgFactoryFuncWithWebCryptoAlgIdArg func,
-          blink::WebCryptoAlgorithmId algorithm_id>
-blink::WebCryptoAlgorithm BindAlgFactoryAlgorithmId() {
-  return func(algorithm_id);
-}
+class JwkAlgorithmInfo {

[Ryan Sleevi, 2014/01/31 20:55:59]

Can we not avoid having to create this class by making use of templated
indirection?

You're allowed to specify integer constants as part of template parameters, so
you should be able to bind "required_key_size_length"

[eroman, 2014/01/31 21:02:50]

Not sure I follow, can u explain further?

[Ryan Sleevi, 2014/01/31 21:15:09]

Well, I was thinking you could smuggle required_key_length_bytes_ as a template
parameter, and instead of returning a blink::WebCryptoAlgorithm, you return a
webcrypto::Status and fill in a blink::WebCryptoAlgorithm* iff the key length
matches the required key length.

That said, it's really unfortunate that the JWK requirements have to live in the
embedder side - it seems like the whole mapping of JWK algs to Blink algs is
something better done on the blink side? IDK

[eroman, 2014/01/31 21:51:24]

I am open to moving more JWK to the blink side, some of the issues are:
  - Export to JWK would likely mainly have to be on the chromium side, otherwise
would need to teach blink how to convert some other key representation to JWK.
If exports lives in Chromium, would be asymetric to have import on the blink
side since a different set of options might be understood.

  - Key wrap/unwrap for JWK formats would have to jump back over to blink
assuming that the core unwrap/wrap was happening on chromium side.

Not sure I see a blink-only JWK solution that would be cleaner, but will think
about some more.

+ public:
+  JwkAlgorithmInfo() :
+      required_key_length_bytes_(NO_KEY_SIZE_REQUIREMENT),
+      creation_func_(NULL) {
+  }
+
+  explicit JwkAlgorithmInfo(AlgorithmCreationFunc algorithm_creation_func)
+      : required_key_length_bytes_(NO_KEY_SIZE_REQUIREMENT),
+        creation_func_(algorithm_creation_func) {
+  }
+
+  JwkAlgorithmInfo(unsigned int required_key_length_bits,
+                   AlgorithmCreationFunc algorithm_creation_func)
+      : required_key_length_bytes_(required_key_length_bits / 8),
+        creation_func_(algorithm_creation_func) {
+    DCHECK((required_key_length_bits % 8) == 0);
+  }
+
+  bool CreateAlgorithm(blink::WebCryptoAlgorithm* algorithm) const {
+    *algorithm = creation_func_();
+    return !algorithm->isNull();
+  }
+
+  bool IsInvalidKeyByteLength(size_t byte_length) const {
+    if (required_key_length_bytes_ == NO_KEY_SIZE_REQUIREMENT)
+      return false;
+    return required_key_length_bytes_  != byte_length;

[Ryan Sleevi, 2014/01/31 21:15:09]

nit: delete extra " "

+  }
 
-// Defines a map between a JWK 'alg' string ID and a corresponding Web Crypto
-// factory function.
-typedef blink::WebCryptoAlgorithm (*AlgFactoryFuncNoArgs)();
-typedef std::map<std::string, AlgFactoryFuncNoArgs> JwkAlgFactoryMap;
+ private:
+  enum {NO_KEY_SIZE_REQUIREMENT = UINT_MAX};
+
+  // The expected key size for the algorithm or NO_KEY_SIZE_REQUIREMENT.
+  unsigned int required_key_length_bytes_;
+
+  AlgorithmCreationFunc creation_func_;

[Ryan Sleevi, 2014/01/31 21:15:09]

pedantic nit: It seems a little structurally cleaner to have creation_func_
first (since that's common for all) and then any *optional* parameters (such as
required_key_length_bytes_) follow afterwards.

+};
+
+typedef std::map<std::string, JwkAlgorithmInfo> JwkAlgorithmInfoMap;
 
-class JwkAlgorithmFactoryMap {
+class JwkAlgorithmRegistry {
  public:
-  JwkAlgorithmFactoryMap() {
-    map_["HS256"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateHmacAlgorithmByHashId,
-                                   blink::WebCryptoAlgorithmIdSha256>;
-    map_["HS384"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateHmacAlgorithmByHashId,
-                                   blink::WebCryptoAlgorithmIdSha384>;
-    map_["HS512"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateHmacAlgorithmByHashId,
-                                   blink::WebCryptoAlgorithmIdSha512>;
-    map_["RS256"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateRsaSsaAlgorithm,
-                                   blink::WebCryptoAlgorithmIdSha256>;
-    map_["RS384"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateRsaSsaAlgorithm,
-                                   blink::WebCryptoAlgorithmIdSha384>;
-    map_["RS512"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateRsaSsaAlgorithm,
-                                   blink::WebCryptoAlgorithmIdSha512>;
-    map_["RSA1_5"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateAlgorithm,
-                                   blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5>;
-    map_["RSA-OAEP"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateRsaOaepAlgorithm,
-                                   blink::WebCryptoAlgorithmIdSha1>;
+  JwkAlgorithmRegistry() {
+    // TODO(eroman):
+    // http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-20
+    // says HMAC with SHA-2 should have a key size at least as large as the
+    // hash output.
+    alg_to_info_["HS256"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateHmacAlgorithmByHashId,
+                        blink::WebCryptoAlgorithmIdSha256>);
+    alg_to_info_["HS384"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateHmacAlgorithmByHashId,
+                        blink::WebCryptoAlgorithmIdSha384>);
+    alg_to_info_["HS512"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateHmacAlgorithmByHashId,
+                         blink::WebCryptoAlgorithmIdSha512>);
+    alg_to_info_["RS256"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateRsaSsaAlgorithm,
+                         blink::WebCryptoAlgorithmIdSha256>);
+    alg_to_info_["RS384"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateRsaSsaAlgorithm,
+                         blink::WebCryptoAlgorithmIdSha384>);
+    alg_to_info_["RS512"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateRsaSsaAlgorithm,
+                         blink::WebCryptoAlgorithmIdSha512>);
+    alg_to_info_["RSA1_5"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateAlgorithm,
+                         blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5>);
+    alg_to_info_["RSA-OAEP"] = JwkAlgorithmInfo(
+        &BindAlgorithmId<webcrypto::CreateRsaOaepAlgorithm,
+                         blink::WebCryptoAlgorithmIdSha1>);
     // TODO(padolph): The Web Crypto spec does not enumerate AES-KW 128 yet
-    map_["A128KW"] = &blink::WebCryptoAlgorithm::createNull;
+    alg_to_info_["A128KW"] =
+        JwkAlgorithmInfo(128, &blink::WebCryptoAlgorithm::createNull);
     // TODO(padolph): The Web Crypto spec does not enumerate AES-KW 256 yet
-    map_["A256KW"] = &blink::WebCryptoAlgorithm::createNull;
-    map_["A128GCM"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateAlgorithm,
-                                   blink::WebCryptoAlgorithmIdAesGcm>;
-    map_["A256GCM"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateAlgorithm,
-                                   blink::WebCryptoAlgorithmIdAesGcm>;
-    map_["A128CBC"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateAlgorithm,
-                                   blink::WebCryptoAlgorithmIdAesCbc>;
-    map_["A192CBC"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateAlgorithm,
-                                   blink::WebCryptoAlgorithmIdAesCbc>;
-    map_["A256CBC"] =
-        &BindAlgFactoryAlgorithmId<webcrypto::CreateAlgorithm,
-                                   blink::WebCryptoAlgorithmIdAesCbc>;
+    alg_to_info_["A256KW"] =
+        JwkAlgorithmInfo(256, &blink::WebCryptoAlgorithm::createNull);
+    alg_to_info_["A128GCM"] = JwkAlgorithmInfo(
+        128,
+        &BindAlgorithmId<webcrypto::CreateAlgorithm,
+                         blink::WebCryptoAlgorithmIdAesGcm>);
+    alg_to_info_["A256GCM"] = JwkAlgorithmInfo(
+        256,
+        &BindAlgorithmId<webcrypto::CreateAlgorithm,
+                         blink::WebCryptoAlgorithmIdAesGcm>);
+    alg_to_info_["A128CBC"] = JwkAlgorithmInfo(
+        128,
+        &BindAlgorithmId<webcrypto::CreateAlgorithm,
+                         blink::WebCryptoAlgorithmIdAesCbc>);
+    alg_to_info_["A192CBC"] = JwkAlgorithmInfo(
+        192,
+        &BindAlgorithmId<webcrypto::CreateAlgorithm,
+                         blink::WebCryptoAlgorithmIdAesCbc>);
+    alg_to_info_["A256CBC"] = JwkAlgorithmInfo(
+        256,
+        &BindAlgorithmId<webcrypto::CreateAlgorithm,
+                         blink::WebCryptoAlgorithmIdAesCbc>);
   }
 
-  blink::WebCryptoAlgorithm CreateAlgorithmFromName(const std::string& alg_id)
-      const {
-    const JwkAlgFactoryMap::const_iterator pos = map_.find(alg_id);
-    if (pos == map_.end())
-      return blink::WebCryptoAlgorithm::createNull();
-    return pos->second();
+  // Returns NULL if the algorithm name was not registered.
+  const JwkAlgorithmInfo* GetAlgorithmInfo(const std::string& jwk_alg) const {
+    const JwkAlgorithmInfoMap::const_iterator pos = alg_to_info_.find(jwk_alg);
+    if (pos == alg_to_info_.end())
+      return NULL;
+    return &pos->second;
   }
 
  private:
-  JwkAlgFactoryMap map_;
+  // Binds a WebCryptoAlgorithmId value to a compatible factory function.
+  typedef blink::WebCryptoAlgorithm (*FuncWithWebCryptoAlgIdArg)(
+      blink::WebCryptoAlgorithmId);
+  template <FuncWithWebCryptoAlgIdArg func,
+            blink::WebCryptoAlgorithmId algorithm_id>
+  static blink::WebCryptoAlgorithm BindAlgorithmId() {
+    return func(algorithm_id);
+  }
+
+  JwkAlgorithmInfoMap alg_to_info_;
 };
 
-base::LazyInstance<JwkAlgorithmFactoryMap> jwk_alg_factory =
+base::LazyInstance<JwkAlgorithmRegistry> jwk_alg_registry =
     LAZY_INSTANCE_INITIALIZER;
 
 bool WebCryptoAlgorithmsConsistent(const blink::WebCryptoAlgorithm& alg1,
@@ -161,12 +202,8 @@ bool GetDecodedUrl64ValueByKey(
     const std::string& key,
     std::string* decoded) {
   std::string value_url64;
-  if (!dict.GetString(key, &value_url64) ||
-      !webcrypto::Base64DecodeUrlSafe(value_url64, decoded) ||
-      !decoded->size()) {
-    return false;
-  }
-  return true;
+  return dict.GetString(key, &value_url64) &&
+         webcrypto::Base64DecodeUrlSafe(value_url64, decoded);
 }
 
 }  // namespace
@@ -536,6 +573,7 @@ Status WebCryptoImpl::ImportKeyJwk(
   // 6. JWK alg missing AND input algorithm specified: use input value
   // TODO(eroman): Should error if "alg" was specified but not a string.
   blink::WebCryptoAlgorithm algorithm = blink::WebCryptoAlgorithm::createNull();
+  const JwkAlgorithmInfo* algorithm_info = NULL;
   std::string jwk_alg_value;
   if (dict_value->GetString("alg", &jwk_alg_value)) {
     // JWK alg present
@@ -543,12 +581,12 @@ Status WebCryptoImpl::ImportKeyJwk(
     // TODO(padolph): Validate alg vs kty. For example kty="RSA" implies alg can
     // only be from the RSA family.
 
-    const blink::WebCryptoAlgorithm jwk_algorithm =
-        jwk_alg_factory.Get().CreateAlgorithmFromName(jwk_alg_value);
-    if (jwk_algorithm.isNull()) {
-      // JWK alg unrecognized
+    blink::WebCryptoAlgorithm jwk_algorithm =
+        blink::WebCryptoAlgorithm::createNull();
+    algorithm_info = jwk_alg_registry.Get().GetAlgorithmInfo(jwk_alg_value);
+    if (!algorithm_info || !algorithm_info->CreateAlgorithm(&jwk_algorithm))
       return Status::ErrorJwkUnrecognizedAlgorithm();  // case 1
-    }
+
     // JWK alg valid
     if (algorithm_or_null.isNull()) {
       // input algorithm not specified
@@ -597,11 +635,16 @@ Status WebCryptoImpl::ImportKeyJwk(
     if (!GetDecodedUrl64ValueByKey(*dict_value, "k", &jwk_k_value))
       return Status::ErrorJwkDecodeK();
 
-    // TODO(padolph): Some JWK alg ID's embed information about the key length
-    // in the alg ID string. For example "A128" implies the JWK carries 128 bits
-    // of key material, and "HS512" implies the JWK carries _at least_ 512 bits
-    // of key material. For such keys validate the actual key length against the
-    // value in the ID.
+    // Some JWK alg ID's embed information about the key length in the alg ID
+    // string. For example "A128CBC" implies the JWK carries 128 bits
+    // of key material. For such keys validate that enough bytes were provided.
+    // If this validation is not done, then it would be possible to select a
+    // different algorithm by passing a different lengthed key, since that is
+    // how WebCrypto interprets things.
+    if (algorithm_info &&
+        algorithm_info->IsInvalidKeyByteLength(jwk_k_value.size())) {
+      return Status::ErrorJwkIncorrectKeyLength();
+    }
 
     return ImportKeyInternal(blink::WebCryptoKeyFormatRaw,
                              reinterpret_cast<const uint8*>(jwk_k_value.data()),