Implement PBKDF2 (except for generateKey) using BoringSSL

content/child/webcrypto/openssl/pbkdf2_openssl.cc

Index: content/child/webcrypto/openssl/pbkdf2_openssl.cc
diff --git a/content/child/webcrypto/openssl/hkdf_openssl.cc b/content/child/webcrypto/openssl/pbkdf2_openssl.cc
similarity index 59%
copy from content/child/webcrypto/openssl/hkdf_openssl.cc
copy to content/child/webcrypto/openssl/pbkdf2_openssl.cc
index 7a33b96e892accc351a501e165b9c1b4100ce544..266ec628bf1264dcad7bb522171d05f296470187 100644
--- a/content/child/webcrypto/openssl/hkdf_openssl.cc
+++ b/content/child/webcrypto/openssl/pbkdf2_openssl.cc
@@ -2,11 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <openssl/err.h>
-#include <openssl/hkdf.h>
-
-#include "base/logging.h"
-#include "base/stl_util.h"
+#include "base/numerics/safe_math.h"
 #include "content/child/webcrypto/algorithm_implementation.h"
 #include "content/child/webcrypto/crypto_data.h"
 #include "content/child/webcrypto/openssl/key_openssl.h"
@@ -23,19 +19,22 @@ namespace webcrypto {
 
 namespace {
 
-const blink::WebCryptoKeyUsageMask kValidUsages =
+const blink::WebCryptoKeyUsageMask kAllKeyUsages =
     blink::WebCryptoKeyUsageDeriveKey | blink::WebCryptoKeyUsageDeriveBits;
 
-class HkdfImplementation : public AlgorithmImplementation {
+class Pbkdf2Implementation : public AlgorithmImplementation {
  public:
-  HkdfImplementation() {}
+  Pbkdf2Implementation() {}
 
   Status VerifyKeyUsagesBeforeImportKey(
       blink::WebCryptoKeyFormat format,
       blink::WebCryptoKeyUsageMask usages) const override {
-    if (format != blink::WebCryptoKeyFormatRaw)
-      return Status::ErrorUnsupportedImportKeyFormat();
-    return CheckKeyCreationUsages(kValidUsages, usages, false);
+    switch (format) {
+      case blink::WebCryptoKeyFormatRaw:
+        return CheckKeyCreationUsages(kAllKeyUsages, usages, false);
+      default:
+        return Status::ErrorUnsupportedImportKeyFormat();
+    }
   }
 
   Status ImportKeyRaw(const CryptoData& key_data,
@@ -43,10 +42,12 @@ class HkdfImplementation : public AlgorithmImplementation {
                       bool extractable,
                       blink::WebCryptoKeyUsageMask usages,
                       blink::WebCryptoKey* key) const override {
-    return CreateWebCryptoSecretKey(
-        key_data, blink::WebCryptoKeyAlgorithm::createWithoutParams(
-                      blink::WebCryptoAlgorithmIdHkdf),
-        extractable, usages, key);
+    const blink::WebCryptoKeyAlgorithm key_algorithm =
+        blink::WebCryptoKeyAlgorithm::createWithoutParams(
+            blink::WebCryptoAlgorithmIdPbkdf2);
+
+    return CreateWebCryptoSecretKey(key_data, key_algorithm, extractable,
+                                    usages, key);
   }
 
   Status DeriveBits(const blink::WebCryptoAlgorithm& algorithm,
@@ -55,39 +56,45 @@ class HkdfImplementation : public AlgorithmImplementation {
                     unsigned int optional_length_bits,
                     std::vector<uint8_t>* derived_bytes) const override {
     crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
     if (!has_optional_length_bits)
-      return Status::ErrorHkdfDeriveBitsLengthNotSpecified();
+      return Status::ErrorPbkdf2DeriveBitsLengthNotSpecified();
+
+    if (optional_length_bits % 8)
+      return Status::ErrorPbkdf2InvalidLength();
 
-    const blink::WebCryptoHkdfParams* params = algorithm.hkdfParams();
+    const blink::WebCryptoPbkdf2Params* params = algorithm.pbkdf2Params();
 
-    const EVP_MD* digest_algorithm = GetDigest(params->hash().id());
+    const blink::WebCryptoAlgorithm& hash = params->hash();
+    const EVP_MD* digest_algorithm = GetDigest(hash.id());
     if (!digest_algorithm)
       return Status::ErrorUnsupported();
 
-    // Size output to fit length
-    unsigned int derived_bytes_len = NumBitsToBytes(optional_length_bits);
-    derived_bytes->resize(derived_bytes_len);
+    unsigned int keylen_bytes = optional_length_bits / 8;
+    derived_bytes->resize(keylen_bytes);
 
-    // Algorithm dispatch checks that the algorithm in |base_key| matches
-    // |algorithm|.
-    const std::vector<uint8_t>& raw_key =
+    const std::vector<uint8_t>& password =
         SymKeyOpenSsl::Cast(base_key)->raw_key_data();
-    const uint8_t* raw_key_ptr = raw_key.empty() ? NULL : &raw_key.front();
-    uint8_t* derived_bytes_ptr =
-        derived_bytes->empty() ? NULL : &derived_bytes->front();
-    if (!HKDF(derived_bytes_ptr, derived_bytes_len, digest_algorithm,
-              raw_key_ptr, raw_key.size(), params->salt().data(),
-              params->salt().size(), params->info().data(),
-              params->info().size())) {
-      uint32_t error = ERR_get_error();
-      if (ERR_GET_LIB(error) == ERR_LIB_HKDF &&
-          ERR_GET_REASON(error) == HKDF_R_OUTPUT_TOO_LARGE) {
-        return Status::ErrorHkdfLengthTooLong();
-      }
-      return Status::OperationError();
-    }
 
-    TruncateToBitLength(optional_length_bits, derived_bytes);
+    base::CheckedNumeric<int> password_size = password.size();

[eroman, 2015/01/14 20:44:58]

Can you add a comment that BoringSSL expects the size as an int, and will
interpret the data as a C-String if it is -1.

[xun.sun, 2015/01/15 17:21:17]

Done.

+    if (!password_size.IsValid())
+      return Status::ErrorDataTooLarge();
+
+    int result = PKCS5_PBKDF2_HMAC(
+        reinterpret_cast<const char*>(password.data()),
+        password_size.ValueOrDie(), params->salt().data(),
+        params->salt().size(), params->iterations(), digest_algorithm,

[davidben, 2015/01/14 23:42:30]

Note: you'll probably end up spinning indefinitely in C++ if iterations is, say,
UINT_MAX. Probably more a question for Eric, but is something WebCrypto
considers okay?

[davidben, 2015/01/14 23:43:03]

(Sorry, not indefinitely but for a rather long time.)

[eroman, 2015/01/15 01:32:07]

I don't believe it is a problem that needs to be solved -- other parts of
WebCrypto can similarly DOS the renderer.

At a more basic level you can write a webpage that does a while (1) {alert();}
and hang the renderer, so meh, WorkingAsIntended :)

That said, WebCrypto is a bit different in the following regard: The crypto
operations are run on a dedicated worker thread, and the result communicated
asynchronously via Promises.

So what would happen in this case is all of your WebCrypto operations would
stall for the renderer. This scenario does deserve some extra thought for
developer debuggability however. I will file a bug and followup there.

+        keylen_bytes, derived_bytes->data());

[davidben, 2015/01/14 23:42:30]

Is std::vector::data something we can rely on in all our platforms? I haven't
been paying attention to whether WebCrypto has been fine with that. (Also
probably a question for Eric.)

It's a library feature new with C++11. (Used here and on password. salt is a
WebVector, so that one seems fine.)

[eroman, 2015/01/15 01:32:06]

Yeah I raised this earlier as well. Other code in WebCrypto does not use
std::vector::data() because as you say, it was unavailable.

I figure if it passes trybots then great the situation has changed (the holdout
IIRC was windows compilers, but the last time I observed this was months ago)

If you do need to remove use of data() you will need to be careful to preserve
the same semantics, since these vectors can be empty.

[xun.sun, 2015/01/15 17:21:17]

Acknowledged.

+
+    if (result == 1)

[davidben, 2015/01/14 23:42:30]

Style nit: PKCS5_PBKDF2_HMAC has been fixed to return only 0 and 1 in BoringSSL.
You can probably just write this as

if (!PKCS5_PBKDF2_HMAC(stuff)) {
  return Status::OperationError();
}
return Status::Success();

[xun.sun, 2015/01/15 17:21:17]

Done.

+      return Status::Success();
+    return Status::OperationError();
+  }
+
+  Status GetKeyLength(const blink::WebCryptoAlgorithm& key_length_algorithm,

[eroman, 2015/01/14 20:44:58]

nit: For consistency with hdkf_openssl.cc, can you move this definition below
DeserializeKeyForClone?

[xun.sun, 2015/01/15 17:21:17]

Done.

+                      bool* has_length_bits,
+                      unsigned int* length_bits) const override {
+    *has_length_bits = false;
     return Status::Success();
   }
 
@@ -107,19 +114,12 @@ class HkdfImplementation : public AlgorithmImplementation {
     return CreateWebCryptoSecretKey(key_data, algorithm, extractable, usages,
                                     key);
   }
-
-  Status GetKeyLength(const blink::WebCryptoAlgorithm& key_length_algorithm,
-                      bool* has_length_bits,
-                      unsigned int* length_bits) const override {
-    *has_length_bits = false;
-    return Status::Success();
-  }
 };
 
 }  // namespace
 
-AlgorithmImplementation* CreatePlatformHkdfImplementation() {
-  return new HkdfImplementation;
+AlgorithmImplementation* CreatePlatformPbkdf2Implementation() {
+  return new Pbkdf2Implementation;
 }
 
 }  // namespace webcrypto