[WebCrypto] Throw syntaxError if keyUsage is empty in ImportKey

content/child/webcrypto/webcrypto_util.cc

 // Copyright 2014 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/child/webcrypto/webcrypto_util.h"
 
 #include <set>
 
 #include "base/logging.h"
 #include "base/numerics/safe_math.h"
@@ skipping 253 matching lines @@
     return Status::Success();
 
   // BoringSSL does not support 192-bit AES.
   if (keylen_bytes == 24)
     return Status::ErrorAes192BitUnsupported();
 
   return Status::ErrorImportAesKeyLength();
 }
 
 Status CheckKeyCreationUsages(blink::WebCryptoKeyUsageMask all_possible_usages,
-                              blink::WebCryptoKeyUsageMask actual_usages) {
+                              blink::WebCryptoKeyUsageMask actual_usages,
+                              bool allow_empty_usages) {
+  if (allow_empty_usages && actual_usages == 0)

[eroman, 2014/12/16 01:26:45]

This naming is reversed. You are passing true for places which do NOT allow
empty usages

[Habib Virji, 2014/12/16 09:59:42]

Corrected now, it has now allow_empty_usages as true for scenarios it should not
check and false when it should check. I was trying to do reverse but naming was
not appropriate. Hopefully this is corrected now. 

+    return Status::ErrorKeyEmptyUsages();
+
   if (!ContainsKeyUsages(all_possible_usages, actual_usages))
     return Status::ErrorCreateKeyBadUsages();
   return Status::Success();
 }
 
 Status GetRsaKeyGenParameters(
     const blink::WebCryptoRsaHashedKeyGenParams* params,
     unsigned int* public_exponent,
     unsigned int* modulus_length_bits) {
   *modulus_length_bits = params->modulusLengthBits();
@@ skipping 19 matching lines @@
   return Status::Success();
 }
 
 Status VerifyUsagesBeforeImportAsymmetricKey(
     blink::WebCryptoKeyFormat format,
     blink::WebCryptoKeyUsageMask all_public_key_usages,
     blink::WebCryptoKeyUsageMask all_private_key_usages,
     blink::WebCryptoKeyUsageMask usages) {
   switch (format) {
     case blink::WebCryptoKeyFormatSpki:
-      return CheckKeyCreationUsages(all_public_key_usages, usages);
+      return CheckKeyCreationUsages(all_public_key_usages, usages, false);
     case blink::WebCryptoKeyFormatPkcs8:
-      return CheckKeyCreationUsages(all_private_key_usages, usages);
+      return CheckKeyCreationUsages(all_private_key_usages, usages, true);
     case blink::WebCryptoKeyFormatJwk: {
       // The JWK could represent either a public key or private key. The usages
       // must make sense for one of the two. The usages will be checked again by
       // ImportKeyJwk() once the key type has been determined.
-      if (CheckKeyCreationUsages(all_public_key_usages, usages).IsError() &&
-          CheckKeyCreationUsages(all_private_key_usages, usages).IsError()) {
+      if (CheckKeyCreationUsages(
+              all_public_key_usages, usages, false).IsError() &&
+          CheckKeyCreationUsages(
+              all_private_key_usages, usages, true).IsError()) {
         return Status::ErrorCreateKeyBadUsages();
       }
       return Status::Success();
     }
     default:
       return Status::ErrorUnsupportedImportKeyFormat();
   }
 }
 
 void TruncateToBitLength(size_t length_bits, std::vector<uint8_t>* bytes) {
@@ skipping 47 matching lines @@
   *has_length_bits = true;
   return GetShaBlockSizeBits(params->hash(), length_bits);
 }
 
 Status GetUsagesForGenerateAsymmetricKey(
     blink::WebCryptoKeyUsageMask combined_usages,
     blink::WebCryptoKeyUsageMask all_public_usages,
     blink::WebCryptoKeyUsageMask all_private_usages,
     blink::WebCryptoKeyUsageMask* public_usages,
     blink::WebCryptoKeyUsageMask* private_usages) {
+  bool allow_empty_usage =

[eroman, 2014/12/16 01:26:45]

I don't understand this, doesn't seem right to me.

It is also confusing because allow_empty_usage is in fact reversed inside of
CheckKeyCreationusages() per my other comment.

[Habib Virji, 2014/12/16 09:59:42]

Yes, i was passing true for scenario where it should check. But naming in the
function was making it mean different.

+      (combined_usages & all_private_usages) ? true : false;
   Status status = CheckKeyCreationUsages(all_public_usages | all_private_usages,
-                                         combined_usages);
+                                         combined_usages, allow_empty_usage);
   if (status.IsError())
     return status;
 
   *public_usages = combined_usages & all_public_usages;
   *private_usages = combined_usages & all_private_usages;
 
-  if (*private_usages == 0)
-    return Status::ErrorCreateKeyEmptyUsages();

[eroman, 2014/12/16 01:26:45]

The older mechanism was clearer, and correct. Just call the combined
CheckKeyCreationUsages() above with a static value (false, although depends if
you change it to actually meaning allow_empty_usage)

And then keep this line.

[Habib Virji, 2014/12/16 09:59:42]

I have updated above, please suggest if it is okay.

-
   return Status::Success();
 }
 
 }  // namespace webcrypto
 
 }  // namespace content