[webcrypto] JWK: Reject keys with non-minimal bigintegers.

RSA properties such as n, e, d, p, q, dp, dq, qi are big integers encoded as base64 url-safe, big-endian octet strings.

 * Reject big integers that contain leading zeros, since by the JWA rules they must be minimal.
 * Reject big integers that are the empty octet string (since 0 is the minimal representation of the big-endian number 0, not empty string).

This also changes the exception message and type when one of the optional parameters p, q, dp, dq, qi are missing. Before it would give an OperationError, because NSS was unable to infer the missing parameters. Now it gives a DataError explaining that the parameter is required.

BUG=374927,383998

Review URL: https://codereview.chromium.org/416993009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285798 0039d316-1c4b-4281-b951-d872f2087c98

content/child/webcrypto/jwk.cc

@@ -322,27 +322,23 @@ Status GetJwkBytes(base::DictionaryValue* dict,
   return Status::Success();
 }
 
-// Extracts the optional string property with key |path| from |dict| and saves
-// the base64url-decoded bytes to |*result|. If the property exist and is not a
-// string, or could not be base64url-decoded, returns an error. In the case
-// where the property does not exist, |result| is guaranteed to be empty.
-Status GetOptionalJwkBytes(base::DictionaryValue* dict,
-                           const std::string& path,
-                           std::string* result,
-                           bool* property_exists) {
-  std::string base64_string;
-  Status status =
-      GetOptionalJwkString(dict, path, &base64_string, property_exists);
+// Extracts the required base64url property, which is interpreted as being a
+// big-endian unsigned integer.
+Status GetJwkBigInteger(base::DictionaryValue* dict,
+                        const std::string& path,
+                        std::string* result) {
+  Status status = GetJwkBytes(dict, path, result);
   if (status.IsError())
     return status;
 
-  if (!*property_exists) {
-    result->clear();
-    return Status::Success();
-  }
+  if (result->empty())
+    return Status::ErrorJwkEmptyBigInteger(path);
 
-  if (!Base64DecodeUrlSafe(base64_string, result))
-    return Status::ErrorJwkBase64Decode(path);
+  // The JWA spec says that "The octet sequence MUST utilize the minimum number
+  // of octets to represent the value." This means there shouldn't be any
+  // leading zeros.
+  if (result->size() > 1 && (*result)[0] == 0)
+    return Status::ErrorJwkBigIntegerHasLeadingZero(path);
 
   return Status::Success();
 }
@@ -650,10 +646,10 @@ Status ReadRsaKeyJwk(const CryptoData& key_data,
   // (private exponent) entry.
   // See http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-18,
   // section 6.3.
-  status = GetJwkBytes(dict.get(), "n", &result->n);
+  status = GetJwkBigInteger(dict.get(), "n", &result->n);
   if (status.IsError())
     return status;
-  status = GetJwkBytes(dict.get(), "e", &result->e);
+  status = GetJwkBigInteger(dict.get(), "e", &result->e);
   if (status.IsError())
     return status;
 
@@ -661,43 +657,33 @@ Status ReadRsaKeyJwk(const CryptoData& key_data,
   if (!result->is_private_key)
     return Status::Success();
 
-  status = GetJwkBytes(dict.get(), "d", &result->d);
+  status = GetJwkBigInteger(dict.get(), "d", &result->d);
   if (status.IsError())
     return status;
 
-  // The "p", "q", "dp", "dq", and "qi" properties are optional. Treat these
-  // properties the same if they are unspecified, as if they were specified-but
-  // empty, since ImportRsaPrivateKey() doesn't do validation checks anyway.
+  // The "p", "q", "dp", "dq", and "qi" properties are optional in the JWA
+  // spec. However they are required by Chromium's WebCrypto implementation.
 
-  bool has_p;
-  status = GetOptionalJwkBytes(dict.get(), "p", &result->p, &has_p);
+  status = GetJwkBigInteger(dict.get(), "p", &result->p);
   if (status.IsError())
     return status;
 
-  bool has_q;
-  status = GetOptionalJwkBytes(dict.get(), "q", &result->q, &has_q);
+  status = GetJwkBigInteger(dict.get(), "q", &result->q);
   if (status.IsError())
     return status;
 
-  bool has_dp;
-  status = GetOptionalJwkBytes(dict.get(), "dp", &result->dp, &has_dp);
+  status = GetJwkBigInteger(dict.get(), "dp", &result->dp);
   if (status.IsError())
     return status;
 
-  bool has_dq;
-  status = GetOptionalJwkBytes(dict.get(), "dq", &result->dq, &has_dq);
+  status = GetJwkBigInteger(dict.get(), "dq", &result->dq);
   if (status.IsError())
     return status;
 
-  bool has_qi;
-  status = GetOptionalJwkBytes(dict.get(), "qi", &result->qi, &has_qi);
+  status = GetJwkBigInteger(dict.get(), "qi", &result->qi);
   if (status.IsError())
     return status;
 
-  int num_optional_properties = has_p + has_q + has_dp + has_dq + has_qi;
-  if (num_optional_properties != 0 && num_optional_properties != 5)
-    return Status::ErrorJwkIncompleteOptionalRsaPrivateKey();
-
   return Status::Success();
 }
 

content/child/webcrypto/nss/rsa_key_nss.cc

@@ -271,22 +271,18 @@ void AddAttribute(CK_ATTRIBUTE_TYPE type,
   templ->push_back(attribute);
 }
 
-// Helper to optionally add an attribute to a template, if the provided data is
-// non-empty.
-void AddOptionalAttribute(CK_ATTRIBUTE_TYPE type,
-                          const CryptoData& data,
-                          std::vector<CK_ATTRIBUTE>* templ) {
-  if (!data.byte_length())
-    return;
+void AddAttribute(CK_ATTRIBUTE_TYPE type,
+                  const CryptoData& data,
+                  std::vector<CK_ATTRIBUTE>* templ) {
   CK_ATTRIBUTE attribute = {type, const_cast<unsigned char*>(data.bytes()),
                             data.byte_length()};
   templ->push_back(attribute);
 }
 
-void AddOptionalAttribute(CK_ATTRIBUTE_TYPE type,
-                          const std::string& data,
-                          std::vector<CK_ATTRIBUTE>* templ) {
-  AddOptionalAttribute(type, CryptoData(data), templ);
+void AddAttribute(CK_ATTRIBUTE_TYPE type,
+                  const std::string& data,
+                  std::vector<CK_ATTRIBUTE>* templ) {
+  AddAttribute(type, CryptoData(data), templ);
 }
 
 Status ExportKeyPkcs8Nss(SECKEYPrivateKey* key, std::vector<uint8_t>* buffer) {
@@ -366,10 +362,10 @@ Status ImportRsaPrivateKey(const blink::WebCryptoAlgorithm& algorithm,
   AddAttribute(CKA_SENSITIVE, &ck_false, sizeof(ck_false), &key_template);
   AddAttribute(CKA_PRIVATE, &ck_false, sizeof(ck_false), &key_template);
 
-  // Required properties.
-  AddOptionalAttribute(CKA_MODULUS, params.n, &key_template);
-  AddOptionalAttribute(CKA_PUBLIC_EXPONENT, params.e, &key_template);
-  AddOptionalAttribute(CKA_PRIVATE_EXPONENT, params.d, &key_template);
+  // Required properties by JWA.
+  AddAttribute(CKA_MODULUS, params.n, &key_template);
+  AddAttribute(CKA_PUBLIC_EXPONENT, params.e, &key_template);
+  AddAttribute(CKA_PRIVATE_EXPONENT, params.d, &key_template);
 
   // Manufacture a CKA_ID so the created key can be retrieved later as a
   // SECKEYPrivateKey using FindKeyByKeyID(). Unfortunately there isn't a more
@@ -398,15 +394,16 @@ Status ImportRsaPrivateKey(const blink::WebCryptoAlgorithm& algorithm,
   //      marked sensitive) then this will break things.
   SECItem modulus_item = MakeSECItemForBuffer(CryptoData(params.n));
   crypto::ScopedSECItem object_id(PK11_MakeIDFromPubKey(&modulus_item));
-  AddOptionalAttribute(
+  AddAttribute(
       CKA_ID, CryptoData(object_id->data, object_id->len), &key_template);
 
-  // Optional properties (all of these will have been specified or none).
-  AddOptionalAttribute(CKA_PRIME_1, params.p, &key_template);
-  AddOptionalAttribute(CKA_PRIME_2, params.q, &key_template);
-  AddOptionalAttribute(CKA_EXPONENT_1, params.dp, &key_template);
-  AddOptionalAttribute(CKA_EXPONENT_2, params.dq, &key_template);
-  AddOptionalAttribute(CKA_COEFFICIENT, params.qi, &key_template);
+  // Optional properties by JWA, however guaranteed to be present by Chromium's
+  // implementation.
+  AddAttribute(CKA_PRIME_1, params.p, &key_template);
+  AddAttribute(CKA_PRIME_2, params.q, &key_template);
+  AddAttribute(CKA_EXPONENT_1, params.dp, &key_template);
+  AddAttribute(CKA_EXPONENT_2, params.dq, &key_template);
+  AddAttribute(CKA_COEFFICIENT, params.qi, &key_template);
 
   crypto::ScopedPK11Slot slot(PK11_GetInternalSlot());
 

content/child/webcrypto/shared_crypto_unittest.cc

@@ -1674,7 +1674,7 @@ TEST_F(SharedCryptoTest, MAYBE(ImportJwkRsaFailures)) {
 
     // Fail on empty parameter.
     dict.SetString(kKtyParmName[idx], "");
-    EXPECT_NE(Status::Success(),
+    EXPECT_EQ(Status::ErrorJwkEmptyBigInteger(kKtyParmName[idx]),
               ImportKeyJwkFromDict(dict, algorithm, false, usage_mask, &key));
     RestoreJwkRsaDictionary(&dict);
   }
@@ -2390,7 +2390,7 @@ TEST_F(SharedCryptoTest, MAYBE(ImportRsaPrivateKeyJwkMissingOptionalParams)) {
                  "iUJyCod1Fyc6NWBT6iobwMlKpy1VxuhilrLfyWeUjApyy8zKfqyzVwbgmh31W"
                  "hU1vZs8w0Fgs7bc0-2o5kQw");
 
-  ASSERT_EQ(Status::ErrorJwkIncompleteOptionalRsaPrivateKey(),
+  ASSERT_EQ(Status::ErrorJwkPropertyMissing("q"),
             ImportKeyJwkFromDict(dict,
                                  CreateRsaHashedImportAlgorithm(
                                      blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5,
@@ -2402,10 +2402,10 @@ TEST_F(SharedCryptoTest, MAYBE(ImportRsaPrivateKeyJwkMissingOptionalParams)) {
 
 // Import a JWK RSA private key, without any of the optional parameters.
 //
-// This is expected to work, however based on the current NSS implementation it
-// does not.
-//
-// TODO(eroman): http://crbug/com/374927
+// According to JWA, such keys are valid, but applications SHOULD
+// include all the parameters when sending, and recipients MAY
+// accept them, but are not required to. Chromium's WebCrypto does
+// not allow such degenerate keys.
 TEST_F(SharedCryptoTest, MAYBE(ImportRsaPrivateKeyJwkIncorrectOptionalEmpty)) {
   if (!SupportsRsaKeyImport())
     return;
@@ -2428,11 +2428,7 @@ TEST_F(SharedCryptoTest, MAYBE(ImportRsaPrivateKeyJwkIncorrectOptionalEmpty)) {
       "kuiUpySsPFaMj5eFOtB8AmbIxqPKCSnx6PESMYhEKfxNmuVf7olqEM5wfD7X5zTkRyejlXRQ"
       "GlMmgxCcKrrKuig8MbS9L1PD7jfjUs7jT55QO9gMBiKtecbc7og1R8ajsyU");
 
-  // TODO(eroman): This should pass, see: http://crbug/com/374927
-  //
-  // Technically it is OK to fail since JWA says that consumer are not required
-  // to support lack of the optional parameters.
-  ASSERT_EQ(Status::OperationError(),
+  ASSERT_EQ(Status::ErrorJwkPropertyMissing("p"),
             ImportKeyJwkFromDict(dict,
                                  CreateRsaHashedImportAlgorithm(
                                      blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5,
@@ -2442,6 +2438,30 @@ TEST_F(SharedCryptoTest, MAYBE(ImportRsaPrivateKeyJwkIncorrectOptionalEmpty)) {
                                  &key));
 }
 
+// Tries importing a public RSA key whose exponent contains leading zeros.
+TEST_F(SharedCryptoTest, MAYBE(ImportJwkRsaNonMinimalExponent)) {
+  base::DictionaryValue dict;
+
+  dict.SetString("kty", "RSA");
+  dict.SetString("e", "AAEAAQ");  // 00 01 00 01
+  dict.SetString(
+      "n",
+      "qLOyhK-OtQs4cDSoYPFGxJGfMYdjzWxVmMiuSBGh4KvEx-CwgtaTpef87Wdc9GaFEncsDLxk"
+      "p0LGxjD1M8jMcvYq6DPEC_JYQumEu3i9v5fAEH1VvbZi9cTg-rmEXLUUjvc5LdOq_5OuHmtm"
+      "e7PUJHYW1PW6ENTP0ibeiNOfFvs");
+
+  blink::WebCryptoKey key = blink::WebCryptoKey::createNull();
+
+  EXPECT_EQ(Status::ErrorJwkBigIntegerHasLeadingZero("e"),
+            ImportKeyJwkFromDict(dict,
+                                 CreateRsaHashedImportAlgorithm(
+                                     blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5,
+                                     blink::WebCryptoAlgorithmIdSha256),
+                                 false,
+                                 blink::WebCryptoKeyUsageVerify,
+                                 &key));
+}
+
 TEST_F(SharedCryptoTest, GenerateKeyPairRsa) {
   // Note: using unrealistic short key lengths here to avoid bogging down tests.
 

content/child/webcrypto/status.cc

@@ -105,10 +105,15 @@ Status Status::ErrorJwkIncorrectKeyLength() {
                 "of key data for the given algorithm.");
 }
 
-Status Status::ErrorJwkIncompleteOptionalRsaPrivateKey() {
+Status Status::ErrorJwkEmptyBigInteger(const std::string& property) {
   return Status(blink::WebCryptoErrorTypeData,
-                "The optional JWK properties p, q, dp, dq, qi must either all "
-                "be provided, or none provided");
+                "The JWK \"" + property + "\" property was empty.");
+}
+
+Status Status::ErrorJwkBigIntegerHasLeadingZero(const std::string& property) {
+  return Status(
+      blink::WebCryptoErrorTypeData,
+      "The JWK \"" + property + "\" property contained a leading zero.");
 }
 
 Status Status::ErrorImportEmptyKeyData() {

content/child/webcrypto/status.h

@@ -103,9 +103,13 @@ class CONTENT_EXPORT Status {
   // given that is an error.
   static Status ErrorJwkIncorrectKeyLength();
 
-  // The JWK was for an RSA private key but only partially provided the optional
-  // parameters (p, q, dq, dq, qi).
-  static Status ErrorJwkIncompleteOptionalRsaPrivateKey();
+  // The JWK property |property| is supposed to represent a big-endian unsigned
+  // integer, however was the empty string.
+  static Status ErrorJwkEmptyBigInteger(const std::string& property);
+
+  // The big-endian unsigned integer |property| contained leading zeros. This
+  // violates the JWA requirement that such octet strings be minimal.
+  static Status ErrorJwkBigIntegerHasLeadingZero(const std::string& property);
 
   // ------------------------------------
   // Other errors