WebCrypto: Implement importKey() and sign() for HMAC in NSS

content/renderer/webcrypto_impl_nss.cc

 // Copyright 2013 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 "content/renderer/webcrypto_impl.h"
 
+#include <pk11pub.h>
 #include <sechash.h>
 
 #include "base/logging.h"
 #include "crypto/nss_util.h"
+#include "crypto/scoped_nss_types.h"
 #include "third_party/WebKit/public/platform/WebArrayBuffer.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
+#include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 
 namespace content {
 
-bool WebCryptoImpl::digestInternal(
+namespace {
+
+class WebCryptoSymKeyHandle : public WebKit::WebCryptoKeyHandle {
+ public:
+  explicit WebCryptoSymKeyHandle(CK_MECHANISM_TYPE mechanism)
+      : mechanism_(mechanism) {
+  }
+
+  bool Initialize() {
+    slot_.reset(PK11_GetInternalSlot());
+    return slot_.get();
+  }
+
+  void set_key(crypto::ScopedPK11SymKey key) {
+    DCHECK(!key_.get());
+    key_ = key.Pass();
+  }
+
+  const CK_MECHANISM_TYPE mechanism() const { return mechanism_; }
+  PK11SlotInfo* slot() { return slot_.get(); }
+  PK11SymKey* key() { return key_.get(); }
+
+ private:
+  CK_MECHANISM_TYPE mechanism_;
+  crypto::ScopedPK11Slot slot_;
+  crypto::ScopedPK11SymKey key_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebCryptoSymKeyHandle);
+};
+
+CK_FLAGS WebCryptoKeyUsageMaskToNSSFlags(
+    WebKit::WebCryptoKeyUsageMask mask) {
+  return ((mask & WebKit::WebCryptoKeyUsageEncrypt) ? CKF_ENCRYPT : 0) |
+      ((mask & WebKit::WebCryptoKeyUsageDecrypt) ? CKF_DECRYPT : 0) |
+      ((mask & WebKit::WebCryptoKeyUsageSign) ? CKF_SIGN : 0) |
+      ((mask & WebKit::WebCryptoKeyUsageVerify) ? CKF_VERIFY : 0) |
+      ((mask & WebKit::WebCryptoKeyUsageDeriveKey) ? CKF_DERIVE : 0) |
+      ((mask & WebKit::WebCryptoKeyUsageWrapKey) ? CKF_WRAP : 0) |
+      ((mask & WebKit::WebCryptoKeyUsageUnwrapKey) ? CKF_UNWRAP : 0);

[Ryan Sleevi, 2013/09/10 19:25:54]

DESIGN: On second thought, we should be tracking these usages independent of the
PKCS#11 usages.

This is especially important for supporting the JWK and wrapped key use cases.
Because PKCS#11 does not support unwrapping JWK, we'll need to decrypt
(CKF_DECRYPT) and then import by hand in the UA.

My suggestion is that the usages should be stored on the SymKeyHandle /
KeyHandle, so that they can then be persisted (eg: as part of the Structured
Clone, if/when its implemented)

[Bryan Eyler, 2013/09/10 21:21:56]

Added a base class for the usages, as I believe this will be necessary for
asymmetric keys as well.  Also moved the mechanism and slot to this base class,
as I believe those will also be needed.

+}
+
+HASH_HashType WebCryptoAlgorithmToNSSHashType(
+    const WebKit::WebCryptoAlgorithm& algorithm) {
+  switch (algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdSha1:
+      return HASH_AlgSHA1;
+    case WebKit::WebCryptoAlgorithmIdSha224:
+      return HASH_AlgSHA224;
+    case WebKit::WebCryptoAlgorithmIdSha256:
+      return HASH_AlgSHA256;
+    case WebKit::WebCryptoAlgorithmIdSha384:
+      return HASH_AlgSHA384;
+    case WebKit::WebCryptoAlgorithmIdSha512:
+      return HASH_AlgSHA512;
+    default:
+      // Not a digest algorithm.
+      return HASH_AlgNULL;
+  }
+}
+
+CK_MECHANISM_TYPE WebCryptoAlgorithmToNSSMechanism(

[Ryan Sleevi, 2013/09/10 19:25:54]

STYLE: I feel like this method should be renamed, since it's for mapping HMAC
mechanisms. For example, you're not mapping to CKM_SHA_1, which is what this
method name would suggest.

[Bryan Eyler, 2013/09/10 21:21:56]

Renamed to WebCryptoAlgorithmToHMACMechanism.

+    const WebKit::WebCryptoAlgorithm& algorithm) {
+  switch (algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdSha1:
+      return CKM_SHA_1_HMAC;
+    case WebKit::WebCryptoAlgorithmIdSha256:
+      return CKM_SHA256_HMAC;
+    default:
+      // Not a supported algorithm.
+      return CKM_INVALID_MECHANISM;
+  }
+}
+
+}  // namespace
+
+bool WebCryptoImpl::DigestInternal(
     const WebKit::WebCryptoAlgorithm& algorithm,
     const unsigned char* data,
-    size_t data_size,
+    unsigned int data_size,
     WebKit::WebArrayBuffer* buffer) {
-  HASH_HashType hash_type = HASH_AlgNULL;
-
-  switch (algorithm.id()) {
-    case WebKit::WebCryptoAlgorithmIdSha1:
-      hash_type = HASH_AlgSHA1;
-      break;
-    case WebKit::WebCryptoAlgorithmIdSha224:
-      hash_type = HASH_AlgSHA224;
-      break;
-    case WebKit::WebCryptoAlgorithmIdSha256:
-      hash_type = HASH_AlgSHA256;
-      break;
-    case WebKit::WebCryptoAlgorithmIdSha384:
-      hash_type = HASH_AlgSHA384;
-      break;
-    case WebKit::WebCryptoAlgorithmIdSha512:
-      hash_type = HASH_AlgSHA512;
-      break;
-    default:
-      // Not a digest algorithm.
-      return false;
+  HASH_HashType hash_type = WebCryptoAlgorithmToNSSHashType(algorithm);
+  if (hash_type == HASH_AlgNULL) {
+    return false;
   }
 
   crypto::EnsureNSSInit();
 
   HASHContext* context = HASH_Create(hash_type);
   if (!context) {
     return false;
   }
 
   HASH_Begin(context);
 
   HASH_Update(context, data, data_size);
 
   size_t hash_result_length = HASH_ResultLenContext(context);
   DCHECK_LE(hash_result_length, static_cast<size_t>(HASH_LENGTH_MAX));
 
   *buffer = WebKit::WebArrayBuffer::create(hash_result_length, 1);
 
   unsigned char* digest = reinterpret_cast<unsigned char*>(buffer->data());
 
   uint32 result_length = 0;
   HASH_End(context, digest, &result_length, hash_result_length);

[Ryan Sleevi, 2013/09/10 19:25:54]

BUG: 1) result_length should be unsigned int, not uint32. What about platforms
where sizeof(unsigned int) != sizeof(uint32)
BUG: 2) hash_result_length is a size_t, but HASH_End expects an unsigned int.

[Bryan Eyler, 2013/09/10 21:21:56]

These were both requests from James; I have made them back into unsigned ints,
but I think there may be some debate about this.

[Ryan Sleevi, 2013/09/10 21:25:41]

This is absolutely a security bug when the sizeof's don't match, because then
you can have stack smashing.

Whenever talking to NSS, the API contract MUST be preserved. Any uses of size_t
(which I think are wrong anyways, since it'll eventually need to cross IPC
boundaries) should be done in the Chrome-controlled interfaces.

 
   HASH_Destroy(context);
 
   return result_length == hash_result_length;
 }
 
+bool WebCryptoImpl::ImportKeyInternal(
+    WebKit::WebCryptoKeyFormat format,
+    const unsigned char* key_data,
+    unsigned int key_data_size,
+    const WebKit::WebCryptoAlgorithm& algorithm,
+    WebKit::WebCryptoKeyUsageMask usage_mask,
+    scoped_ptr<WebKit::WebCryptoKeyHandle>* handle,
+    WebKit::WebCryptoKeyType* type) {
+  switch (algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdHmac:
+      *type = WebKit::WebCryptoKeyTypeSecret;
+      break;
+    // TODO(bryaneyler): Support more key types.
+    default:
+      return false;
+  }
+
+  // TODO(bryaneyler): Need to split handling for symmetric and asymmetric keys.
+  // Currently only supporting symmetric.
+  scoped_ptr<WebCryptoSymKeyHandle> sym_key;
+
+  switch(algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdHmac: {
+      const WebKit::WebCryptoHmacParams* params = algorithm.hmacParams();
+      if (!params) {
+        return false;
+      }
+
+      CK_MECHANISM_TYPE mechanism =
+          WebCryptoAlgorithmToNSSMechanism(params->hash());
+      if (mechanism == CKM_INVALID_MECHANISM) {
+        return false;
+      }
+
+      sym_key.reset(new WebCryptoSymKeyHandle(mechanism));
+
+      if (!sym_key->Initialize()) {
+        return false;
+      }
+
+      break;
+    }
+    default:
+      return false;
+  }
+
+  SECItem key_item = { siBuffer, NULL, 0 };
+
+  switch (format) {
+    case WebKit::WebCryptoKeyFormatRaw:
+      key_item.data = const_cast<unsigned char*>(key_data);
+      key_item.len = key_data_size;
+      break;
+    // TODO(bryaneyler): Handle additional formats.
+    default:
+      return false;
+  }
+
+  crypto::ScopedPK11SymKey pk11_sym_key(
+      PK11_ImportSymKeyWithFlags(sym_key->slot(),
+                                 sym_key->mechanism(),
+                                 PK11_OriginUnwrap,
+                                 CKA_FLAGS_ONLY,
+                                 &key_item,
+                                 WebCryptoKeyUsageMaskToNSSFlags(usage_mask),
+                                 false,
+                                 NULL));
+  sym_key->set_key(pk11_sym_key.Pass());
+  if (!sym_key->key()) {
+    NOTREACHED();
+    return false;
+  }
+
+  *handle = sym_key.Pass();
+
+  return true;
+}
+
+bool WebCryptoImpl::SignInternal(
+    const WebKit::WebCryptoAlgorithm& algorithm,
+    const WebKit::WebCryptoKeyHandle* key,
+    const unsigned char* data,
+    unsigned int data_size,
+    WebKit::WebArrayBuffer* buffer) {
+  WebKit::WebArrayBuffer result;
+
+  switch (algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdHmac: {
+      const WebKit::WebCryptoHmacParams* params = algorithm.hmacParams();
+      if (!params) {
+        return false;
+      }
+
+      WebCryptoSymKeyHandle* sym_key =
+          const_cast<WebCryptoSymKeyHandle*>(
+              reinterpret_cast<const WebCryptoSymKeyHandle*>(key));
+
+      DCHECK_EQ(sym_key->mechanism(),
+                WebCryptoAlgorithmToNSSMechanism(params->hash()));

[Ryan Sleevi, 2013/09/10 19:25:54]

Why is this DCHECK still needed? You're no longer constructing the output based
on ->hash(), so this seems entirely unnecessary.

[Bryan Eyler, 2013/09/10 21:21:56]

Sign is provided with the hash algorithm, so I figured it might be useful to
validate that the algorithm matches.  Does this make sense?  Or should I just
ignore that algorithm value and trust the Blink layer to catch an invalid use?

+
+      SECItem param_item = { siBuffer, NULL, 0 };
+      SECItem data_item = {
+        siBuffer,
+        const_cast<unsigned char*>(data),
+        data_size
+      };
+      // First call is to figure out the length.
+      SECItem signature_item = { siBuffer, NULL, 0 };
+
+      if (PK11_SignWithSymKey(sym_key->key(),
+                              sym_key->mechanism(),
+                              &param_item,
+                              &signature_item,
+                              &data_item) != SECSuccess) {
+        NOTREACHED();
+        return false;
+      }
+
+      DCHECK_LT(0u, signature_item.len);
+
+      result = WebKit::WebArrayBuffer::create(signature_item.len, 1);
+      signature_item.data = reinterpret_cast<unsigned char*>(result.data());
+
+      if (PK11_SignWithSymKey(sym_key->key(),
+                              sym_key->mechanism(),
+                              &param_item,
+                              &signature_item,
+                              &data_item) != SECSuccess) {
+        NOTREACHED();
+        return false;
+      }
+

[Ryan Sleevi, 2013/09/10 01:22:36]

BUG: You need to update the length of |result| to be the returned length.

This is more of a general sanity thing - in the case of HMAC, it will always be
the length, but as you move to add stuff like AES-CBC etc, you may have variable
block sizes.

[Bryan Eyler, 2013/09/10 21:21:56]

Sorry, I'm not exactly sure what you're looking for here.  Do you want me to
make sure the resulting length matches the data length with a DCHECK?

[Ryan Sleevi, 2013/09/10 21:25:41]

PK11_SignWithSymKey will update signature_item.len to match the length of the
actual signature item, which may be less than the signature_item.len at
creation.

WebArrayBuffer's length then needs to be updated to reflect that returned size.

[Bryan Eyler, 2013/09/10 22:45:34]

It doesn't look like there's a way to update the length of the WebArrayBuffer,
but I can create a new one.  This probably makes the most sense to have this
logic when the length is expected to be different from the computed length, but
I could add logic to check if the length differs and create a new WebArrayBuffer
if it does?

+      break;
+    }
+    default:
+      return false;
+  }
+
+  *buffer = result;
+  return true;
+}
+
 }  // namespace content