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

content/renderer/webcrypto/webcrypto_impl.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/webcrypto/webcrypto_impl.h"
 
 #include <algorithm>
 #include <functional>
 #include <map>
 #include "base/json/json_reader.h"
@@ skipping 20 matching lines @@
 }
 
 bool IsAlgorithmAsymmetric(const blink::WebCryptoAlgorithm& algorithm) {
   // TODO(padolph): include all other asymmetric algorithms once they are
   // defined, e.g. EC and DH.
   return (algorithm.id() == blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5 ||
           algorithm.id() == blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 ||
           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);
-}
-
-// 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;
-
-class JwkAlgorithmFactoryMap {
+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:
-  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>;
-    // TODO(padolph): The Web Crypto spec does not enumerate AES-KW 128 yet
-    map_["A128KW"] = &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>;
+  JwkAlgorithmInfo() :
+      required_key_length_bytes_(NO_KEY_SIZE_REQUIREMENT),
+      creation_func_(NULL) {
   }
 
-  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();
+  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 " "

   }
 
  private:
-  JwkAlgFactoryMap map_;
+  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.

 };
 
-base::LazyInstance<JwkAlgorithmFactoryMap> jwk_alg_factory =
+typedef std::map<std::string, JwkAlgorithmInfo> JwkAlgorithmInfoMap;
+
+class JwkAlgorithmRegistry {
+ public:
+  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
+    alg_to_info_["A128KW"] =
+        JwkAlgorithmInfo(128, &blink::WebCryptoAlgorithm::createNull);
+    // TODO(padolph): The Web Crypto spec does not enumerate AES-KW 256 yet
+    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>);
+  }
+
+  // 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:
+  // 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<JwkAlgorithmRegistry> jwk_alg_registry =
     LAZY_INSTANCE_INITIALIZER;
 
 bool WebCryptoAlgorithmsConsistent(const blink::WebCryptoAlgorithm& alg1,
                                    const blink::WebCryptoAlgorithm& alg2) {
   DCHECK(!alg1.isNull());
   DCHECK(!alg2.isNull());
   if (alg1.id() != alg2.id())
     return false;
   switch (alg1.id()) {
     case blink::WebCryptoAlgorithmIdHmac:
@@ skipping 20 matching lines @@
       break;
   }
   return false;
 }
 
 bool GetDecodedUrl64ValueByKey(
     const base::DictionaryValue& dict,
     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
 
 WebCryptoImpl::WebCryptoImpl() {
   Init();
 }
 
 void WebCryptoImpl::encrypt(
     const blink::WebCryptoAlgorithm& algorithm,
@@ skipping 349 matching lines @@
   // 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
   // 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
 
     // 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
       algorithm = jwk_algorithm;  // case 2
     } else {
       // input algorithm specified
       if (!WebCryptoAlgorithmsConsistent(jwk_algorithm, algorithm_or_null))
         return Status::ErrorJwkAlgorithmInconsistent();  // case 3
       algorithm = algorithm_or_null;  // case 4
     }
@@ skipping 28 matching lines @@
     }
   }
 
   // JWK keying material --> ImportKeyInternal()
   if (jwk_kty_value == "oct") {
 
     std::string jwk_k_value;
     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()),
                              jwk_k_value.size(),
                              algorithm,
                              extractable,
                              usage_mask,
                              key);
   } else if (jwk_kty_value == "RSA") {
 
@@ skipping 27 matching lines @@
         key);
 
   } else {
     return Status::ErrorJwkUnrecognizedKty();
   }
 
   return Status::Success();
 }
 
 }  // namespace content