Implement HKDF for webcrypto

content/child/webcrypto/openssl/hkdf_openssl.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 <openssl/err.h>
+#include <openssl/hkdf.h>
+
+// TODO: figure out which headers belong here and remove ones that were

[eroman, 2014/12/23 23:13:36]

(1) TODO(nharper)
(2) Address the TODO now :)

[nharper, 2015/01/06 22:53:42]

Done.

+// just copied over from hmac
+#include "base/logging.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/sym_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 kValidUsages =
+    blink::WebCryptoKeyUsageDeriveKey | blink::WebCryptoKeyUsageDeriveBits;
+
+class HkdfImplementation : public AlgorithmImplementation {
+ public:
+  HkdfImplementation() {}
+
+  // This is doing a check that is only valid for raw format, but since
+  // ImportKeyRaw is the the only ImportKey implemented here, there's no
+  // need to check the format here.

[eroman, 2014/12/23 23:13:36]

You should do the check, otherwise this is inconsistent with how other
algorithms are implemented.

In particular consider the case of unwrapping a key with a format other than
raw. The errors will differ.

[nharper, 2015/01/06 22:53:42]

I added a check here for this. It turns out in both cases, the correct thing to
do is throw a NotSupportedError, so there's no difference in the errors.

+  Status VerifyKeyUsagesBeforeImportKey(
+      blink::WebCryptoKeyFormat format,
+      blink::WebCryptoKeyUsageMask usages) const override {
+    return CheckKeyCreationUsages(kValidUsages, usages, false);
+  }
+

[eroman, 2014/12/23 23:13:36]

This also needs support for cloning/de-cloning.

[nharper, 2015/01/06 22:53:42]

Done.

+  Status ImportKeyRaw(const CryptoData& key_data,
+                      const blink::WebCryptoAlgorithm& algorithm,
+                      bool extractable,
+                      blink::WebCryptoKeyUsageMask usages,
+                      blink::WebCryptoKey* key) const override {
+    *key = blink::WebCryptoKey::create(
+        new SymKeyOpenSsl(key_data), blink::WebCryptoKeyTypeSecret, extractable,
+        blink::WebCryptoKeyAlgorithm::createHkdf(), usages);
+    return Status::Success();
+  }
+
+  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 {
+    // When called by crypto.subtle.deriveBits(), length will already be present
+    // and converted to a number. It won't be called as part of deriveKey,

[eroman, 2014/12/23 23:13:36]

I don't understand this comment. HKDF does support deriveKey().

Seems like this is reachable by calling deriveKey() for HDKF, and specifying the
derivedKeyType as also HKDF, right?

The spec says to throw a TypeError in this case.

[nharper, 2015/01/06 22:53:42]

Done.

+    // because that's not supported for this algorithm. Therefore, it's
+    // unexpected for this method to ever be called without a length.
+    if (!has_optional_length_bits)
+      return Status::ErrorUnexpected();
+
+    // "If the hash member of normalizedAlgorithm does not describe a recognized
+    // algorithm that supports the digest operation, then throw a
+    // NotSupportedError"
+    const EVP_MD* digest_algorithm =
+        GetDigest(algorithm.hkdfParams()->hash().id());

[eroman, 2014/12/23 23:13:36]

It is guaranteed to already be a digest, since the Blink Layer ensures it is a
HashAlgorithmIdentifier when parsing.

[nharper, 2015/01/06 22:53:42]

This same check is done in the HMAC code - any objection to leaving it here?

[eroman, 2015/01/07 00:23:42]

Nope. Although we should be consistent about when it is Unexpected vs
NotSupported. We should change the others to be NotSupported as well.

[nharper, 2015/01/08 01:31:23]

It looks like sha_openssl.cc is the only one (code that calls GetDigest() in
src/content/child/webcrypto/openssl) that returns Status::ErrorUnexpected()
instead of Status::ErrorUnsupported(). I'll change that in a separate cl.

+    if (!digest_algorithm)
+      return Status::ErrorUnsupported();
+
+    // Size output to fit length
+    unsigned int derived_bytes_len = (optional_length_bits + 7) / 8;

[eroman, 2014/12/23 23:13:36]

Use NumBitsToBytes() instead.

It is more readable, but more importantly the input is user controlled and we
should not allow integer overflowing.

[nharper, 2015/01/06 22:53:42]

Done.

+    derived_bytes->resize(derived_bytes_len);
+
+    // Algorithm dispatch checks that the algorithm in |base_key| matches
+    // |algorithm|.
+    std::vector<uint8_t> raw_key =

[eroman, 2014/12/23 23:13:36]

Don't make a copy of the key material.

[nharper, 2015/01/06 22:53:42]

Done.

+        SymKeyOpenSsl::Cast(base_key)->raw_key_data();
+    blink::WebVector<unsigned char> salt = algorithm.hkdfParams()->salt();

[eroman, 2014/12/23 23:13:36]

Don't make copies here.

[nharper, 2015/01/06 22:53:42]

Done.

+    blink::WebVector<unsigned char> info = algorithm.hkdfParams()->info();
+    if (!HKDF(derived_bytes->data(), derived_bytes_len, digest_algorithm,
+              raw_key.data(), raw_key.size(), salt.data(), salt.size(),
+              info.data(), 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();
+      } else {

[eroman, 2014/12/23 23:13:36]

nit: omit "else" when short-circuiting.

[nharper, 2015/01/06 22:53:42]

Done. Also removed the curly braces from the one-line if statement, even though
I disagree with that style.

+        return Status::OperationError();
+      }
+    }
+    return Status::Success();
+  }
+};
+
+}  // namespace
+
+AlgorithmImplementation* CreatePlatformHkdfImplementation() {
+  return new HkdfImplementation;
+}
+
+}  // namespace webcrypto
+
+}  // namespace content