[webcryto] Validate key usages during key creation.

content/child/webcrypto/shared_crypto.cc

Index: content/child/webcrypto/shared_crypto.cc
diff --git a/content/child/webcrypto/shared_crypto.cc b/content/child/webcrypto/shared_crypto.cc
index 7d4704fc656ec7aca87fdefa3ffe22201bd70608..539617d323b563f02084adf6816cb49c4e213a35 100644
--- a/content/child/webcrypto/shared_crypto.cc
+++ b/content/child/webcrypto/shared_crypto.cc
@@ -474,12 +474,13 @@ Status UnwrapKeyDecryptAndImport(
       wrapping_algorithm, wrapping_key, wrapped_key_data, &buffer);
   if (status.IsError())
     return status;
-  status = ImportKey(
+  // NOTE that returning the details of ImportKey() failures may leak
+  // information about the plaintext of the encrypted key (for instance the JWK
+  // key_ops). As long as the ImportKey error messages don't describe actual
+  // key bytes however this should be OK. For more discussion see
+  // http://crubg.com/372040

[Ryan Sleevi, 2014/05/15 06:29:40]

crbug.com

+  return ImportKey(
       format, CryptoData(buffer), algorithm, extractable, usage_mask, key);
-  // NOTE! Returning the details of any ImportKey() failure here would leak
-  // information about the plaintext internals of the encrypted key. Instead,
-  // collapse any error into the generic Status::OperationError().
-  return status.IsError() ? Status::OperationError() : Status::Success();
 }
 
 Status WrapKeyExportAndEncrypt(
@@ -511,6 +512,150 @@ unsigned int ShaBlockSizeBytes(blink::WebCryptoAlgorithmId hash_id) {
   }
 }
 
+Status ImportKeyDontCheckKeyUsages(blink::WebCryptoKeyFormat format,
+                                   const CryptoData& key_data,
+                                   const blink::WebCryptoAlgorithm& algorithm,
+                                   bool extractable,
+                                   blink::WebCryptoKeyUsageMask usage_mask,
+                                   blink::WebCryptoKey* key) {
+  switch (format) {
+    case blink::WebCryptoKeyFormatRaw:
+      return ImportKeyRaw(key_data, algorithm, extractable, usage_mask, key);
+    case blink::WebCryptoKeyFormatSpki:
+      return platform::ImportKeySpki(
+          algorithm, key_data, extractable, usage_mask, key);
+    case blink::WebCryptoKeyFormatPkcs8:
+      return platform::ImportKeyPkcs8(
+          algorithm, key_data, extractable, usage_mask, key);
+    case blink::WebCryptoKeyFormatJwk:
+      return ImportKeyJwk(key_data, algorithm, extractable, usage_mask, key);
+    default:
+      return Status::ErrorUnsupported();
+  }
+}
+
+Status UnwrapKeyDontCheckKeyUsages(
+    blink::WebCryptoKeyFormat format,
+    const CryptoData& wrapped_key_data,
+    const blink::WebCryptoKey& wrapping_key,
+    const blink::WebCryptoAlgorithm& wrapping_algorithm,
+    const blink::WebCryptoAlgorithm& algorithm,
+    bool extractable,
+    blink::WebCryptoKeyUsageMask usage_mask,
+    blink::WebCryptoKey* key) {
+  if (!KeyUsageAllows(wrapping_key, blink::WebCryptoKeyUsageUnwrapKey))
+    return Status::ErrorUnexpected();
+  if (wrapping_algorithm.id() != wrapping_key.algorithm().id())
+    return Status::ErrorUnexpected();
+
+  switch (format) {
+    case blink::WebCryptoKeyFormatRaw:
+      return UnwrapKeyRaw(wrapped_key_data,
+                          wrapping_key,
+                          wrapping_algorithm,
+                          algorithm,
+                          extractable,
+                          usage_mask,
+                          key);
+    case blink::WebCryptoKeyFormatJwk:
+      return UnwrapKeyDecryptAndImport(format,
+                                       wrapped_key_data,
+                                       wrapping_key,
+                                       wrapping_algorithm,
+                                       algorithm,
+                                       extractable,
+                                       usage_mask,
+                                       key);
+    case blink::WebCryptoKeyFormatSpki:
+    case blink::WebCryptoKeyFormatPkcs8:
+      return Status::ErrorUnsupported();  // TODO(padolph)
+    default:
+      NOTREACHED();
+      return Status::ErrorUnsupported();
+  }
+}
+
+// Returns the mask of all key usages that are possible for |algorithm| and
+// |key_type|.
+blink::WebCryptoKeyUsageMask GetValidKeyUsagesForKeyType(
+    blink::WebCryptoAlgorithmId algorithm,
+    blink::WebCryptoKeyType key_type) {
+  switch (algorithm) {
+    case blink::WebCryptoAlgorithmIdAesCbc:
+    case blink::WebCryptoAlgorithmIdAesGcm:
+    case blink::WebCryptoAlgorithmIdAesCtr:
+      return blink::WebCryptoKeyUsageEncrypt | blink::WebCryptoKeyUsageDecrypt |
+             blink::WebCryptoKeyUsageWrapKey |
+             blink::WebCryptoKeyUsageUnwrapKey;
+    case blink::WebCryptoAlgorithmIdAesKw:
+      return blink::WebCryptoKeyUsageWrapKey |
+             blink::WebCryptoKeyUsageUnwrapKey;
+    case blink::WebCryptoAlgorithmIdHmac:
+      return blink::WebCryptoKeyUsageSign | blink::WebCryptoKeyUsageVerify;
+    case blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5:
+      switch (key_type) {
+        case blink::WebCryptoKeyTypePublic:
+          return blink::WebCryptoKeyUsageVerify;
+        case blink::WebCryptoKeyTypePrivate:
+          return blink::WebCryptoKeyUsageSign;
+        default:
+          return 0;
+      }
+    case blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5:
+    case blink::WebCryptoAlgorithmIdRsaOaep:
+      switch (key_type) {
+        case blink::WebCryptoKeyTypePublic:

[Ryan Sleevi, 2014/05/15 06:29:40]

Structurally, I'm wondering whether it makes more sense to divide this first by
key type, then by algorithm. Thoughts?

[eroman, 2014/05/16 00:04:37]

I can paint the bikeshed either colour, don't particularly care.

My arguments for doing algorithm first are:

  * Fewer default cases in switches. When chromium/blink are eventually merged I
want to get rid of all the "default" switches on "switch (algorithm)". Breaking
down on keytype first means there will be 3 incomplete switches.
  
  * Aesthetically I prefer grouping algorithms together.

[Ryan Sleevi, 2014/05/19 23:06:29]

Part of the comment was motivated by things like line 601-602 / 613-614, which
by default return 0 if the key_type is not consistent with algorithm - eg: if
'secret' is supplied as type.

However, for the symmetric keys, you don't have that equivalent behaviour - if
the type is public/private (which is invalid), it still returns 'valid' key
usages.

+          return blink::WebCryptoKeyUsageEncrypt |
+                 blink::WebCryptoKeyUsageWrapKey;
+        case blink::WebCryptoKeyTypePrivate:
+          return blink::WebCryptoKeyUsageDecrypt |
+                 blink::WebCryptoKeyUsageUnwrapKey;
+        default:
+          return 0;
+      }
+    default:
+      return 0;
+  }
+}
+
+// Returns Status::Success() if |usages| is a valid set of key usages for
+// |algorithm| and |key_type|. Otherwise returns an error.
+Status CheckKeyUsages(blink::WebCryptoAlgorithmId algorithm,
+                      blink::WebCryptoKeyType key_type,
+                      blink::WebCryptoKeyUsageMask usages) {
+  blink::WebCryptoKeyUsageMask all_usages =
+      GetValidKeyUsagesForKeyType(algorithm, key_type);
+
+  if (!ContainsKeyUsages(all_usages, usages))
+    return Status::ErrorCreateKeyBadUsages();
+
+  return Status::Success();
+}
+
+// Returns an error if |combined_usage_mask| is invalid for generating a key
+// pair for |algorithm|. Otherwise returns Status::Success(), and fills
+// |public_key_usages| with the usages for the public key, and
+// |private_key_usages| with those for the private key.
+Status CheckKeyUsagesForGenerateKeyPair(
+    blink::WebCryptoAlgorithmId algorithm,
+    blink::WebCryptoKeyUsageMask combined_usage_mask,
+    blink::WebCryptoKeyUsageMask* public_key_usages,
+    blink::WebCryptoKeyUsageMask* private_key_usages) {
+  blink::WebCryptoKeyUsageMask all_public_key_usages =
+      GetValidKeyUsagesForKeyType(algorithm, blink::WebCryptoKeyTypePublic);
+  blink::WebCryptoKeyUsageMask all_private_key_usages =
+      GetValidKeyUsagesForKeyType(algorithm, blink::WebCryptoKeyTypePrivate);
+
+  if (!ContainsKeyUsages(all_public_key_usages | all_private_key_usages,
+                         combined_usage_mask))
+    return Status::ErrorCreateKeyBadUsages();
+
+  *public_key_usages = combined_usage_mask & all_public_key_usages;
+  *private_key_usages = combined_usage_mask & all_private_key_usages;

[Ryan Sleevi, 2014/05/15 06:29:40]

Seems like it should be invalid to generate a keypair where the private key has
no usages.

Thoughts? Spec bug?

[eroman, 2014/05/16 00:04:37]

My interpretation of the spec is that keys with no usages is valid, and hence it
is allowed by Chromium.

I personally don't see a reason to disallow it. I am not sure how useful a key
without usages is, but it can still be used for exporting the key material so
not completely useless.

[Ryan Sleevi, 2014/05/19 23:06:29]

Fair point, though I wonder if we shouldn't correct the spec, since it's
possible you attempt to generate for usages that are not supported, end up
hitting the empty array - but still perform the expensive operation of
generating the key.

[eroman, 2014/05/20 00:32:19]

Not sure I follow.

GenerateKey() does a pre-check -- if any of the requested usages are not
applicable to either public keys or private keys, it fails.

Only then does it take the intersection. For instance this can be seen in the
RSA-ES part of the spec which starts with:

If usages contains an entry which is not "encrypt", "decrypt", "wrapKey" or
"unwrapKey", then return an error named DataError.



So you can't simply end up with a key with empty usages because it contained
something unexpected. If the requested usages contained something unexpected and
totally bogus you end up with a DataError.

Now as far as whether empty usages should be allowed at all, I raised that as
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25820, however I see this as an
orthogonal issue.

+
+  return Status::Success();
+}
+
 }  // namespace
 
 void Init() { platform::Init(); }
@@ -556,6 +701,11 @@ Status GenerateSecretKey(const blink::WebCryptoAlgorithm& algorithm,
                          bool extractable,
                          blink::WebCryptoKeyUsageMask usage_mask,
                          blink::WebCryptoKey* key) {
+  Status status =
+      CheckKeyUsages(algorithm.id(), blink::WebCryptoKeyTypeSecret, usage_mask);
+  if (status.IsError())
+    return status;
+
   unsigned int keylen_bytes = 0;
 
   // Get the secret key length in bytes from generation parameters.
@@ -600,9 +750,19 @@ Status GenerateSecretKey(const blink::WebCryptoAlgorithm& algorithm,
 
 Status GenerateKeyPair(const blink::WebCryptoAlgorithm& algorithm,
                        bool extractable,
-                       blink::WebCryptoKeyUsageMask usage_mask,
+                       blink::WebCryptoKeyUsageMask combined_usage_mask,
                        blink::WebCryptoKey* public_key,
                        blink::WebCryptoKey* private_key) {
+  blink::WebCryptoKeyUsageMask public_key_usage_mask = 0;
+  blink::WebCryptoKeyUsageMask private_key_usage_mask = 0;
+
+  Status status = CheckKeyUsagesForGenerateKeyPair(algorithm.id(),
+                                                   combined_usage_mask,
+                                                   &public_key_usage_mask,
+                                                   &private_key_usage_mask);
+  if (status.IsError())
+    return status;
+
   // TODO(padolph): Handle other asymmetric algorithm key generation.
   switch (algorithm.paramsType()) {
     case blink::WebCryptoAlgorithmParamsTypeRsaHashedKeyGenParams:
@@ -626,7 +786,8 @@ Status GenerateKeyPair(const blink::WebCryptoAlgorithm& algorithm,
 
       return platform::GenerateRsaKeyPair(algorithm,
                                           extractable,
-                                          usage_mask,
+                                          public_key_usage_mask,
+                                          private_key_usage_mask,
                                           params->modulusLengthBits(),
                                           publicExponent,
                                           hash_or_null,
@@ -645,20 +806,13 @@ Status ImportKey(blink::WebCryptoKeyFormat format,
                  bool extractable,
                  blink::WebCryptoKeyUsageMask usage_mask,
                  blink::WebCryptoKey* key) {
-  switch (format) {
-    case blink::WebCryptoKeyFormatRaw:
-      return ImportKeyRaw(key_data, algorithm, extractable, usage_mask, key);
-    case blink::WebCryptoKeyFormatSpki:
-      return platform::ImportKeySpki(
-          algorithm, key_data, extractable, usage_mask, key);
-    case blink::WebCryptoKeyFormatPkcs8:
-      return platform::ImportKeyPkcs8(
-          algorithm, key_data, extractable, usage_mask, key);
-    case blink::WebCryptoKeyFormatJwk:
-      return ImportKeyJwk(key_data, algorithm, extractable, usage_mask, key);
-    default:
-      return Status::ErrorUnsupported();
-  }
+  Status status = ImportKeyDontCheckKeyUsages(
+      format, key_data, algorithm, extractable, usage_mask, key);

[Ryan Sleevi, 2014/05/15 06:29:40]

Shouldn't we be able to fast-fail if |algorithm| and |usage_mask| are invalid?
The only time we might reduce the set of usages is for JWK import, and we can do
that check when considering the intersection.

Ditto unwrap

[eroman, 2014/05/16 00:04:37]

I opted to do the key usages test *after* key creation to avoid unnecessary code
complexity, and protect the code from breaking in the future.

Deciding exactly what to fail-fast on is unclear. For instance when unwrapping
JWK for HMAC we could fail fast on key usages (since key type is known), however
when unwrapping JWK for RSA-SSA we cannot (since key type is not known yet).

To illustrate how this can get complicated if you want consistent fail-fast here
are some of the cases that need care:

-------------
Implementation cases
-------------

Import:

 * In ImportKey(), for usage of raw,spki,pkcs8 do usage check upfront.
 * If usage == JWK, do the usage check as part of ImportKeyJwk(), once the key
type has been determined.

Unwrapping:
 * If unwrapping a raw/spki/pkcs8 format, do the usage check upfront. There are
landmines here: unwrapping of raw keys is currently special cased and doesn't go
through ImportKey() at all. So doing the pre-check is critical. However for the
other formats ImportKey() will subsequently be called and do the check as well
(used for JWK next)

 * If unwrapping a jwk format you _could_ do a fail-fast for symmetric
algorithms since the key type is already known. Otherwise the check can be left
to ImportKey() as part of importing JWK.

Hooking multiple places with key checks is just asking for things to break.
Ideally I would only have 2 places to hook: ImportKey() and GenerateKey(), but
UnwrapKey() doesn't consistently call ImportKey() so I added it there too.

-------------
Performance
-------------

The operation we are optimizing with a fail-fast is mainly developer error. With
the exception of JWK formats (import/unwrap) giving the wrong key usages is
quite simply an invalid usage, and whether we fail it fast or slow the end
result is likely still failure.

For these reasons I think code simplicity are the more important metric. WDYT?

[Ryan Sleevi, 2014/05/19 23:06:29]

For the similar reasons as the generate case above - where you intend to use a
feature that is not yet in the implementation - it seems vastly more beneficial
to fail as quickly as possible.

I'm still inclined to suggest a double check, and may try to put it in the spec
as a pre-condition, as part of the overall feature-detection/fast-failure.

[eroman, 2014/05/20 01:27:57]

FYI: I am updating the CL to do fail-fast wherever possible, will ping when
ready for re-review.

+
+  if (status.IsError())
+    return status;
+
+  return CheckKeyUsages(algorithm.id(), key->type(), usage_mask);
 }
 
 // TODO(eroman): Move this to anonymous namespace.
@@ -783,36 +937,18 @@ Status UnwrapKey(blink::WebCryptoKeyFormat format,
                  bool extractable,
                  blink::WebCryptoKeyUsageMask usage_mask,
                  blink::WebCryptoKey* key) {
-  if (!KeyUsageAllows(wrapping_key, blink::WebCryptoKeyUsageUnwrapKey))
-    return Status::ErrorUnexpected();
-  if (wrapping_algorithm.id() != wrapping_key.algorithm().id())
-    return Status::ErrorUnexpected();
+  Status status = UnwrapKeyDontCheckKeyUsages(format,
+                                              wrapped_key_data,
+                                              wrapping_key,
+                                              wrapping_algorithm,
+                                              algorithm,
+                                              extractable,
+                                              usage_mask,
+                                              key);
+  if (status.IsError())
+    return status;
 
-  switch (format) {
-    case blink::WebCryptoKeyFormatRaw:
-      return UnwrapKeyRaw(wrapped_key_data,
-                          wrapping_key,
-                          wrapping_algorithm,
-                          algorithm,
-                          extractable,
-                          usage_mask,
-                          key);
-    case blink::WebCryptoKeyFormatJwk:
-      return UnwrapKeyDecryptAndImport(format,
-                                       wrapped_key_data,
-                                       wrapping_key,
-                                       wrapping_algorithm,
-                                       algorithm,
-                                       extractable,
-                                       usage_mask,
-                                       key);
-    case blink::WebCryptoKeyFormatSpki:
-    case blink::WebCryptoKeyFormatPkcs8:
-      return Status::ErrorUnsupported();  // TODO(padolph)
-    default:
-      NOTREACHED();
-      return Status::ErrorUnsupported();
-  }
+  return CheckKeyUsages(algorithm.id(), key->type(), usage_mask);
 }
 
 // Note that this function is called from the target Blink thread.