WebCrypto: Implement crypto.subtle.deriveKey (chromium-side).

content/child/webcrypto/test/ecdh_unittest.cc

Index: content/child/webcrypto/test/ecdh_unittest.cc
diff --git a/content/child/webcrypto/test/ecdh_unittest.cc b/content/child/webcrypto/test/ecdh_unittest.cc
index 56af8b4106f11a1c167ede7cce903651dc01bfe2..e092c3159a76838344eb2ea0faf8b442a0f6b2de 100644
--- a/content/child/webcrypto/test/ecdh_unittest.cc
+++ b/content/child/webcrypto/test/ecdh_unittest.cc
@@ -42,7 +42,41 @@ blink::WebCryptoAlgorithm CreateEcdhDeriveParams(
       new blink::WebCryptoEcdhKeyDeriveParams(public_key));
 }
 
-TEST(WebCryptoEcdhTest, VerifyKnownAnswer) {
+blink::WebCryptoAlgorithm CreateAesGcmDerivedKeyParams(
+    unsigned short length_bits) {
+  return blink::WebCryptoAlgorithm::adoptParamsAndCreate(
+      blink::WebCryptoAlgorithmIdAesGcm,
+      new blink::WebCryptoAesDerivedKeyParams(length_bits));
+}
+
+// Helper that loads a  "public_key" and "private_key" from the test data.
+void ImportKeysFromTest(const base::DictionaryValue* test,
+                        blink::WebCryptoKey* public_key,
+                        blink::WebCryptoKey* private_key) {
+  // Import the public key.
+  const base::DictionaryValue* public_key_json = NULL;
+  EXPECT_TRUE(test->GetDictionary("public_key", &public_key_json));
+  blink::WebCryptoNamedCurve curve =
+      GetCurveNameFromDictionary(public_key_json, "crv");
+  ASSERT_EQ(Status::Success(),
+            ImportKey(blink::WebCryptoKeyFormatJwk,
+                      CryptoData(MakeJsonVector(*public_key_json)),
+                      CreateEcdhImportAlgorithm(curve), true, 0, public_key));
+
+  // Import the private key.
+  const base::DictionaryValue* private_key_json = NULL;
+  EXPECT_TRUE(test->GetDictionary("private_key", &private_key_json));
+  curve = GetCurveNameFromDictionary(private_key_json, "crv");
+  ASSERT_EQ(Status::Success(),
+            ImportKey(blink::WebCryptoKeyFormatJwk,
+                      CryptoData(MakeJsonVector(*private_key_json)),
+                      CreateEcdhImportAlgorithm(curve), true,
+                      blink::WebCryptoKeyUsageDeriveBits |
+                          blink::WebCryptoKeyUsageDeriveKey,
+                      private_key));
+}
+
+TEST(WebCryptoEcdhTest, DeriveBitsKnownAnswer) {
   if (!SupportsEcdh())
     return;
 
@@ -55,28 +89,10 @@ TEST(WebCryptoEcdhTest, VerifyKnownAnswer) {
     const base::DictionaryValue* test;
     ASSERT_TRUE(tests->GetDictionary(test_index, &test));
 
-    // Import the public key.
-    const base::DictionaryValue* public_key_json = NULL;
-    EXPECT_TRUE(test->GetDictionary("public_key", &public_key_json));
-    blink::WebCryptoNamedCurve curve =
-        GetCurveNameFromDictionary(public_key_json, "crv");
+    // Import the keys.
     blink::WebCryptoKey public_key;
-    ASSERT_EQ(
-        Status::Success(),
-        ImportKey(blink::WebCryptoKeyFormatJwk,
-                  CryptoData(MakeJsonVector(*public_key_json)),
-                  CreateEcdhImportAlgorithm(curve), true, 0, &public_key));
-
-    // Import the private key.
-    const base::DictionaryValue* private_key_json = NULL;
-    EXPECT_TRUE(test->GetDictionary("private_key", &private_key_json));
-    curve = GetCurveNameFromDictionary(private_key_json, "crv");
     blink::WebCryptoKey private_key;
-    ASSERT_EQ(Status::Success(),
-              ImportKey(blink::WebCryptoKeyFormatJwk,
-                        CryptoData(MakeJsonVector(*private_key_json)),
-                        CreateEcdhImportAlgorithm(curve), true,
-                        blink::WebCryptoKeyUsageDeriveBits, &private_key));
+    ImportKeysFromTest(test, &public_key, &private_key);
 
     // Now try to derive bytes.
     std::vector<uint8_t> derived_bytes;
@@ -100,6 +116,197 @@ TEST(WebCryptoEcdhTest, VerifyKnownAnswer) {
   }
 }
 
+// Loads up a test ECDH public and private key for P-521. The keys
+// come from different key pairs, and can be used for key derivation of up to
+// 528 bits.
+void LoadTestKeys(blink::WebCryptoKey* public_key,
+                  blink::WebCryptoKey* private_key) {
+  // Assume that the 7th key in the test data is for P-521.

[Ryan Sleevi, 2014/12/10 03:21:26]

I'm not terribly keen on assumptions like this baked into the files.

[eroman, 2014/12/12 01:25:22]

I have not addressed this comment yet.

I don't much care for this either, but it seems simpler than duplicating the key
data elsewhere.

Some ways to improve this would be:

  (1) Give explicit names to the test keys, rather than relying on its position
in the array.

  (2) Use a separate test file containing a P-521 key

  (3) Generate a key directly in the test (although will slow down the test)

  (4) Inline the key data into the C++ test file.


I will address this as a follow-up changelist based on your response to these
options.

+  scoped_ptr<base::ListValue> tests;
+  ASSERT_TRUE(ReadJsonTestFileToList("ecdh.json", &tests));
+
+  const base::DictionaryValue* test;
+  ASSERT_TRUE(tests->GetDictionary(6, &test));
+
+  ImportKeysFromTest(test, public_key, private_key);
+
+  ASSERT_EQ(blink::WebCryptoNamedCurveP521,
+            public_key->algorithm().ecParams()->namedCurve());
+}
+
+// Try deriving an AES key of length 129 bits.
+TEST(WebCryptoEcdhTest, DeriveKeyBadAesLength) {
+  if (!SupportsEcdh())
+    return;
+
+  blink::WebCryptoKey public_key;
+  blink::WebCryptoKey base_key;
+  LoadTestKeys(&public_key, &base_key);
+
+  blink::WebCryptoKey derived_key;
+
+  ASSERT_EQ(Status::ErrorGetAesKeyLength(),
+            DeriveKey(CreateEcdhDeriveParams(public_key), base_key,
+                      CreateAlgorithm(blink::WebCryptoAlgorithmIdAesGcm),
+                      CreateAesGcmDerivedKeyParams(129), true,
+                      blink::WebCryptoKeyUsageEncrypt, &derived_key));
+}
+
+// Try deriving an AES key of length 192 bits.
+TEST(WebCryptoEcdhTest, DeriveKeyUnsupportedAesLength) {
+  if (!SupportsEcdh())
+    return;
+
+  blink::WebCryptoKey public_key;
+  blink::WebCryptoKey base_key;
+  LoadTestKeys(&public_key, &base_key);
+
+  blink::WebCryptoKey derived_key;
+
+  ASSERT_EQ(Status::ErrorAes192BitUnsupported(),
+            DeriveKey(CreateEcdhDeriveParams(public_key), base_key,
+                      CreateAlgorithm(blink::WebCryptoAlgorithmIdAesGcm),
+                      CreateAesGcmDerivedKeyParams(192), true,
+                      blink::WebCryptoKeyUsageEncrypt, &derived_key));
+}
+
+// Try deriving an HMAC key of length 0 bits.
+TEST(WebCryptoEcdhTest, DeriveKeyZeroLengthHmac) {
+  if (!SupportsEcdh())
+    return;
+
+  blink::WebCryptoKey public_key;
+  blink::WebCryptoKey base_key;
+  LoadTestKeys(&public_key, &base_key);
+
+  blink::WebCryptoKey derived_key;
+
+  const blink::WebCryptoAlgorithm import_algorithm =
+      CreateHmacImportAlgorithm(blink::WebCryptoAlgorithmIdSha1, 0);
+
+  ASSERT_EQ(Status::ErrorGetHmacKeyLengthZero(),
+            DeriveKey(CreateEcdhDeriveParams(public_key), base_key,
+                      import_algorithm, import_algorithm, true,
+                      blink::WebCryptoKeyUsageSign, &derived_key));
+}
+
+// Derive an HMAC key of length 19 bits.
+TEST(WebCryptoEcdhTest, DeriveKeyHmac19Bits) {
+  if (!SupportsEcdh())
+    return;
+
+  blink::WebCryptoKey public_key;
+  blink::WebCryptoKey base_key;
+  LoadTestKeys(&public_key, &base_key);
+
+  blink::WebCryptoKey derived_key;
+
+  const blink::WebCryptoAlgorithm import_algorithm =
+      CreateHmacImportAlgorithm(blink::WebCryptoAlgorithmIdSha1, 19);
+
+  ASSERT_EQ(Status::Success(),
+            DeriveKey(CreateEcdhDeriveParams(public_key), base_key,
+                      import_algorithm, import_algorithm, true,
+                      blink::WebCryptoKeyUsageSign, &derived_key));
+
+  ASSERT_EQ(blink::WebCryptoAlgorithmIdHmac, derived_key.algorithm().id());
+  ASSERT_EQ(blink::WebCryptoAlgorithmIdSha1,
+            derived_key.algorithm().hmacParams()->hash().id());
+  ASSERT_EQ(19u, derived_key.algorithm().hmacParams()->lengthBits());
+
+  // Export the key and verify its contents.
+  std::vector<uint8_t> raw_key;
+  EXPECT_EQ(Status::Success(),
+            ExportKey(blink::WebCryptoKeyFormatRaw, derived_key, &raw_key));
+  EXPECT_EQ(3u, raw_key.size());
+  // The last 7 bits of the key should be zero.
+  EXPECT_EQ(0, raw_key[raw_key.size() - 1] & 0x1f);
+}
+
+// Derive an HMAC key with no specified length (just the hash of SHA-256).
+TEST(WebCryptoEcdhTest, DeriveKeyHmacSha256NoLength) {
+  if (!SupportsEcdh())
+    return;
+
+  blink::WebCryptoKey public_key;
+  blink::WebCryptoKey base_key;
+  LoadTestKeys(&public_key, &base_key);
+
+  blink::WebCryptoKey derived_key;
+
+  const blink::WebCryptoAlgorithm import_algorithm =
+      CreateHmacImportAlgorithmNoLength(blink::WebCryptoAlgorithmIdSha256);
+
+  ASSERT_EQ(Status::Success(),
+            DeriveKey(CreateEcdhDeriveParams(public_key), base_key,
+                      import_algorithm, import_algorithm, true,
+                      blink::WebCryptoKeyUsageSign, &derived_key));
+
+  ASSERT_EQ(blink::WebCryptoAlgorithmIdHmac, derived_key.algorithm().id());
+  ASSERT_EQ(blink::WebCryptoAlgorithmIdSha256,
+            derived_key.algorithm().hmacParams()->hash().id());
+  ASSERT_EQ(512u, derived_key.algorithm().hmacParams()->lengthBits());
+
+  // Export the key and verify its contents.
+  std::vector<uint8_t> raw_key;
+  EXPECT_EQ(Status::Success(),
+            ExportKey(blink::WebCryptoKeyFormatRaw, derived_key, &raw_key));
+  EXPECT_EQ(64u, raw_key.size());
+}
+
+// Derive an HMAC key with no specified length (just the hash of SHA-512).
+//
+// This fails, because ECDH using P-521 can only generate 528 bits, however HMAC
+// SHA-512 requires 1024 bits.
+//
+// (Really shouldn't be generating HMAC keys using ECDH to begin with, since the
+// output of ECDH is not that random...).

[Ryan Sleevi, 2014/12/10 03:21:26]

Wait, what's up with this comment?

[eroman, 2014/12/10 17:58:55]

The first 7 bits of ECDH P-521's output is zero (field size is 521 bits, however
output is 528 bits)

Therefore using ECDH P-521 to generate an AES-128-bit key (as an example) is
just asking for trouble. Your  derived key has 7 of the 128 bits directly known
(first bits will be zero). And traits of the key can be determined further
reducing entropy.

I am not a crypto expert, but it seems that truncating the ECDH output
(especially from the high order bytes) to directly make a symmetric key is a bad
idea.

Surely you should instead be sending the un-truncated output of ECDH through a
KDF to get a more randomly distributed 128-bit key?

That said I am happy to remove the comment.

[Ryan Sleevi, 2014/12/11 23:23:48]

Do we do leading padding? I thought the result was trailing padding?
I'm not sure how to interpret this last sentence.
Oh, yes, it's absolutely, positively terribad. 
Well, I'm not sure I'd go "more randomly distributed", since you're not going to
make entropy appear out of nowhere. But it is going to let you do key expansion,
as well as generate distinct keys, and those are both good things.
I think maybe the wording leaves a lot of nuance out

"(In practice, authors won't be directly generating keys from key agreement
schemes, as that is frequently insecure, and instead be using KDFs to expand and
generate keys. For simplicity of testing, however, test using an HMAC key)"

Or something.

[eroman, 2014/12/12 01:21:17]

The leading padding comes from using a big integer that is in big-ending order.
This is most pronounced in P-521 where the rounded up degree adds 7 unused bits.

From a conversation I had with Wan-Teh this is correct, and specified by SEC 1
section 2.3.7.
Not sure I know what I was writing here either...

I was trying to regurgitate what I learned from smarter people than myself on
the matter, who summarized it to me as:

"the result of an ECDH key agreement is not uniformly distributed in its range,
which is one reason it must be fed into a KDF"

The specific reasons why being, "math".
Sounds good, done

+TEST(WebCryptoEcdhTest, DeriveKeyHmacSha512NoLength) {
+  if (!SupportsEcdh())
+    return;
+
+  blink::WebCryptoKey public_key;
+  blink::WebCryptoKey base_key;
+  LoadTestKeys(&public_key, &base_key);
+
+  blink::WebCryptoKey derived_key;
+
+  const blink::WebCryptoAlgorithm import_algorithm =
+      CreateHmacImportAlgorithmNoLength(blink::WebCryptoAlgorithmIdSha512);
+
+  ASSERT_EQ(Status::ErrorEcdhLengthTooBig(528),
+            DeriveKey(CreateEcdhDeriveParams(public_key), base_key,
+                      import_algorithm, import_algorithm, true,
+                      blink::WebCryptoKeyUsageSign, &derived_key));
+}
+
+// Try deriving an AES key of length 128 bits.
+TEST(WebCryptoEcdhTest, DeriveKeyAes128) {
+  if (!SupportsEcdh())
+    return;
+
+  blink::WebCryptoKey public_key;
+  blink::WebCryptoKey base_key;
+  LoadTestKeys(&public_key, &base_key);
+
+  blink::WebCryptoKey derived_key;
+
+  ASSERT_EQ(Status::Success(),
+            DeriveKey(CreateEcdhDeriveParams(public_key), base_key,
+                      CreateAlgorithm(blink::WebCryptoAlgorithmIdAesGcm),
+                      CreateAesGcmDerivedKeyParams(128), true,
+                      blink::WebCryptoKeyUsageEncrypt, &derived_key));
+
+  ASSERT_EQ(blink::WebCryptoAlgorithmIdAesGcm, derived_key.algorithm().id());
+  ASSERT_EQ(128, derived_key.algorithm().aesParams()->lengthBits());
+
+  // Export the key and verify its contents.
+  std::vector<uint8_t> raw_key;
+  EXPECT_EQ(Status::Success(),
+            ExportKey(blink::WebCryptoKeyFormatRaw, derived_key, &raw_key));
+  EXPECT_EQ(16u, raw_key.size());
+}
+
 }  // namespace
 
 }  // namespace webcrypto