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/pbkdf2_openssl.cc b/content/child/webcrypto/openssl/pbkdf2_openssl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..040a22c87876e14ad309fcc1c8f81539c30184e9
--- /dev/null
+++ b/content/child/webcrypto/openssl/pbkdf2_openssl.cc
@@ -0,0 +1,109 @@
+// Copyright 2015 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 "base/logging.h"
+#include "base/numerics/safe_math.h"
+#include "base/stl_util.h"
+#include "content/child/webcrypto/algorithm_implementation.h"
+#include "content/child/webcrypto/crypto_data.h"
+#include "content/child/webcrypto/jwk.h"
+#include "content/child/webcrypto/openssl/key_openssl.h"
+#include "content/child/webcrypto/openssl/util_openssl.h"
+#include "content/child/webcrypto/status.h"
+#include "content/child/webcrypto/webcrypto_util.h"
+#include "crypto/openssl_util.h"
+#include "crypto/secure_util.h"
+#include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
+#include "third_party/WebKit/public/platform/WebCryptoKeyAlgorithm.h"
+
+namespace content {
+
+namespace webcrypto {
+
+namespace {
+
+const blink::WebCryptoKeyUsageMask kAllKeyUsages =
+    blink::WebCryptoKeyUsageDeriveKey | blink::WebCryptoKeyUsageDeriveBits;
+class Pbkdf2Implementation : public AlgorithmImplementation {

[eroman, 2015/01/09 02:26:21]

style: Add a newline before class definition

[xun.sun, 2015/01/09 16:08:41]

Done.

+ public:
+  Pbkdf2Implementation() {}
+
+  Status GenerateKey(const blink::WebCryptoAlgorithm& algorithm,
+                     bool extractable,
+                     blink::WebCryptoKeyUsageMask usages,
+                     GenerateKeyResult* result) const override {
+    return Status::ErrorUnsupported();
+  }
+
+  Status VerifyKeyUsagesBeforeImportKey(
+      blink::WebCryptoKeyFormat format,
+      blink::WebCryptoKeyUsageMask usages) const override {
+    switch (format) {
+      case blink::WebCryptoKeyFormatRaw:
+        return CheckKeyCreationUsages(kAllKeyUsages, usages, false);
+      default:
+        return Status::ErrorUnsupportedImportKeyFormat();
+    }
+  }
+
+  Status ImportKeyRaw(const CryptoData& key_data,
+                      const blink::WebCryptoAlgorithm& algorithm,
+                      bool extractable,
+                      blink::WebCryptoKeyUsageMask usages,
+                      blink::WebCryptoKey* key) const override {
+    const blink::WebCryptoKeyAlgorithm key_algorithm =
+        blink::WebCryptoKeyAlgorithm::createPbkdf2(algorithm.id());
+
+    return CreateWebCryptoSecretKey(key_data, key_algorithm, extractable,
+                                    usages, key);
+  }
+
+  Status DeriveBits(const blink::WebCryptoAlgorithm& algorithm,
+                    const blink::WebCryptoKey& base_key,
+                    bool has_optional_length_bits,
+                    unsigned int optional_length_bits,
+                    std::vector<uint8_t>* derived_bytes) const override {
+    if (!has_optional_length_bits || optional_length_bits % 8)
+      return Status::ErrorPbkdf2InvalidLength();
+
+    const blink::WebCryptoPbkdf2Params* params = algorithm.pbkdf2Params();
+    const blink::WebVector<unsigned char>& salt = params->salt();
+    const unsigned long iterations = params->iterations();

[eroman, 2015/01/09 02:26:21]

This should be an unsigned int, will comment on blink side changelist.

That said, it isn't necessary to create an alias to params->iterations(). I
think this code is more readable if the call to PKCS_PBKDF2_HMAC() just calls
params->iterations() directly. Same for params->salt() and hash.

[xun.sun, 2015/01/09 16:08:41]

Done.

+    const blink::WebCryptoAlgorithm& hash = params->hash();
+
+    const EVP_MD* digest_algorithm = GetDigest(hash.id());
+    if (!digest_algorithm)
+      return Status::ErrorUnsupported();
+
+    unsigned int dkLen = optional_length_bits / 8;

[eroman, 2015/01/09 02:26:21]

(1) dkLen is not chromium style naming.
(20 dk_len is not a descriptive variable name. length_bytes or keylen_bytes
would be more consistent with function parameters.

[xun.sun, 2015/01/09 16:08:41]

Done.

+    unsigned int hLen = EVP_MD_size(digest_algorithm);

[eroman, 2015/01/09 02:26:21]

style naming throughout here.

[xun.sun, 2015/01/09 16:08:41]

Done.

+
+    unsigned long maxLength = ((1L << 32) - 1) * hLen;

[eroman, 2015/01/09 02:26:21]

This doesn't make sense, what is it trying to enforce?

dkLen should be 32-bit unsigned quantity (of bytes, which was given as a 32-bit
quantity of bits).

The only way to hit the operationerror below is if hlen=0 which also doesn't
make sense.

[xun.sun, 2015/01/09 16:08:41]

Section 5.2 in https://www.ietf.org/rfc/rfc2898.txt. But I agree this is moot
since dkLen is 32-bit. So this check is removed.

+    if (dkLen > maxLength)
+      return Status::OperationError();
+
+    derived_bytes->resize(dkLen);
+
+    const std::vector<uint8_t>& password =
+        SymKeyOpenSsl::Cast(base_key)->raw_key_data();
+
+    int result = PKCS5_PBKDF2_HMAC(
+        (const char*)password.data(), password.size(), salt.data(), salt.size(),

[eroman, 2015/01/09 02:26:21]

(1) use C++ style casts, like static_cast<const char*> or reinterpret_cast<const
char*>

(2) There needs to be a test to prevent underflowing password.size().

password.size() is an unsigned quantity, however the crypto library expects a
signed value. If it were to get -1 it will do something very very bad (treat the
input as a null-terminated C-string, which it is not).

Use base::CheckedNumeric<> to test first.

[xun.sun, 2015/01/09 16:08:41]

Done.

+        iterations, digest_algorithm, dkLen, derived_bytes->data());
+    if (result == 1)
+      return Status::Success();
+    else

[eroman, 2015/01/09 02:26:21]

nit: we omit "else" clauses when the "if" above returns, so delete this.

Or alternately:

return result == 1 ? Status::Succes() : Status::OperationError();

[xun.sun, 2015/01/09 16:08:41]

Done.

+      return Status::OperationError();
+  }

[eroman, 2015/01/09 02:26:21]

The Serialize methods are needed here as well, since the KDFs support structured
cloning.

See the comments I left for Nick's change, which is implementing a very similar
algorithm (https://codereview.chromium.org/803173010/)

[xun.sun, 2015/01/09 16:08:41]

Done.

+};
+
+}  // namespace
+
+AlgorithmImplementation* CreatePlatformPbkdf2Implementation() {
+  return new Pbkdf2Implementation;
+}
+
+}  // namespace webcrypto
+
+}  // namespace content