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

content/renderer/webcrypto_impl_nss.cc

Index: content/renderer/webcrypto_impl_nss.cc
diff --git a/content/renderer/webcrypto_impl_nss.cc b/content/renderer/webcrypto_impl_nss.cc
index 61aa3a69dcb9ef8816003722a1d042ec0247b373..f035289fd7b3da68daa332268707d04555ddeb53 100644
--- a/content/renderer/webcrypto_impl_nss.cc
+++ b/content/renderer/webcrypto_impl_nss.cc
@@ -4,41 +4,91 @@
 
 #include "content/renderer/webcrypto_impl.h"
 
-#include <sechash.h>
+#include <nss/sechash.h>
+#include <nss/pk11pub.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(
-    const WebKit::WebCryptoAlgorithm& algorithm,
-    const unsigned char* data,
-    size_t data_size,
-    WebKit::WebArrayBuffer* buffer) {
-  HASH_HashType hash_type = HASH_AlgNULL;
+class WebCryptoSymKeyHandle : public WebKit::WebCryptoKeyHandle {
+ public:
+  WebCryptoSymKeyHandle(CK_MECHANISM_TYPE mechanism)

[eroman, 2013/09/06 23:03:16]

single-parameter ctors should be marked as "explicit"

[Bryan Eyler, 2013/09/09 22:32:10]

Done.

+      : mechanism_(mechanism),
+        slot_(PK11_GetInternalSlot()) {
+    DCHECK(slot_ != NULL);

[eroman, 2013/09/06 23:03:16]

It is customary in chrome to omit the != NULL. just:

DCHECK(slot_);

or if that doesn't compile then:

DCHECK(slot_.get());

Just to show my NSS ignorance for a moment, is failure possible in the API? (in
which case, this should be a handled error case rather than a DCHECK).

[Bryan Eyler, 2013/09/09 22:32:10]

Not much documentation on this, but crypto/ seems to handle the error case, so
changed from DCHECK to error handling.

+  }
+
+  void set_key(crypto::ScopedPK11SymKey key) {

[eroman, 2013/09/06 23:03:16]

Good.
I suggest adding a DCHECK(!key_) for good measure, as this is only supposed to
be called once.

[Bryan Eyler, 2013/09/09 22:32:10]

Done.

+    key_ = key.Pass();
+  }
+
+  const CK_MECHANISM_TYPE mechanism() const { return mechanism_; }
+  PK11SlotInfo* slot() { return slot_.get(); }
+  const PK11SymKey* key() const { return key_.get(); }
+
+ private:

[eroman, 2013/09/06 23:03:16]

Please also add DISALLOW_COPY_AND_ASSIGN (or whatever it is).

Not strictly necessary since the scoped slot makes this uncopiable, but still
good practice.

[Bryan Eyler, 2013/09/09 22:32:10]

Done.

+  CK_MECHANISM_TYPE mechanism_;
+  crypto::ScopedPK11Slot slot_;
+  crypto::ScopedPK11SymKey key_;
+};
+
+static CK_FLAGS WebCryptoKeyUsageMaskToNSSFlags(

[eroman, 2013/09/06 23:03:16]

I recommend moving all of this section into an unnamed namespace rather than
labeling them static.

Thanks for moving this to a standalone function!

[Bryan Eyler, 2013/09/09 22:32:10]

Done.

+    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);
+}
 
+static HASH_HashType WebCryptoAlgorithmToNSSHashType(
+    const WebKit::WebCryptoAlgorithm& algorithm) {
   switch (algorithm.id()) {
     case WebKit::WebCryptoAlgorithmIdSha1:
-      hash_type = HASH_AlgSHA1;
-      break;
+      return HASH_AlgSHA1;
     case WebKit::WebCryptoAlgorithmIdSha224:
-      hash_type = HASH_AlgSHA224;
-      break;
+      return HASH_AlgSHA224;
     case WebKit::WebCryptoAlgorithmIdSha256:
-      hash_type = HASH_AlgSHA256;
-      break;
+      return HASH_AlgSHA256;
     case WebKit::WebCryptoAlgorithmIdSha384:
-      hash_type = HASH_AlgSHA384;
-      break;
+      return HASH_AlgSHA384;
     case WebKit::WebCryptoAlgorithmIdSha512:
-      hash_type = HASH_AlgSHA512;
-      break;
+      return HASH_AlgSHA512;
     default:
       // Not a digest algorithm.
-      return false;
+      return HASH_AlgNULL;
+  }
+}
+
+static CK_MECHANISM_TYPE WebCryptoAlgorithmToNSSMechanism(
+    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;
+  }
+}
+
+bool WebCryptoImpl::DigestInternal(
+    const WebKit::WebCryptoAlgorithm& algorithm,
+    const unsigned char* data,
+    size_t data_size,
+    WebKit::WebArrayBuffer* buffer) {
+  HASH_HashType hash_type = WebCryptoAlgorithmToNSSHashType(algorithm);
+  if (hash_type == HASH_AlgNULL) {
+    return false;
   }
 
   crypto::EnsureNSSInit();
@@ -67,4 +117,142 @@ bool WebCryptoImpl::digestInternal(
   return result_length == hash_result_length;
 }
 
+bool WebCryptoImpl::ImportKeyInternal(
+    WebKit::WebCryptoKeyFormat format,
+    const unsigned char* key_data,
+    size_t 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));
+      break;
+    }
+    default:
+      return false;
+  }
+
+  SECItem key_item = {

[eroman, 2013/09/06 23:03:16]

nit: inconsistent style (later on you initialize a SecItem just like this all on
one line).

[Bryan Eyler, 2013/09/09 22:32:10]

Done.

+    siBuffer,
+    NULL,
+    0
+  };
+
+  switch (format) {
+    case WebKit::WebCryptoKeyFormatRaw:
+      key_item.data = const_cast<unsigned char*>(key_data);
+      key_item.len = key_data_size;

[eroman, 2013/09/06 23:03:16]

Hmm.

NSS expects "unsigned int" for lengths, however here we are setting it with a
size_t.

Under the LP64 datamodel, size_t will be 64-bits whereas unsigned int will be
32-bits, so that could result in truncation.

Unfortunately chromium doesn't treat integer narrowing warnings as an error, so
it isn't easy to notice.

In practice the blink side will not pass individual buffers exceeding 32-bit so
this isn't a problem.

I hadn't considered this issue when I designed the API. Probably the best thing
is for me to change the api from "size_t" to "unsigned" and then we don't have
to worry about this anymore :)

[Bryan Eyler, 2013/09/09 22:32:10]

Will re-based once https://codereview.chromium.org/23537027/ lands.

+      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,
+    size_t data_size,
+    WebKit::WebArrayBuffer* buffer) {
+  WebKit::WebArrayBuffer result;
+
+  switch (algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdHmac: {
+      const WebKit::WebCryptoHmacParams* params = algorithm.hmacParams();
+      if (!params) {
+        return false;
+      }
+
+      const WebCryptoSymKeyHandle* sym_key =
+          reinterpret_cast<const WebCryptoSymKeyHandle*>(key);
+
+      HASH_HashType hash_type = WebCryptoAlgorithmToNSSHashType(params->hash());
+      if (hash_type == HASH_AlgNULL) {
+        return false;
+      }
+
+      size_t digest_length = HASH_ResultLen(hash_type);

[eroman, 2013/09/06 23:03:16]

unsigned int for consistency

[Bryan Eyler, 2013/09/09 22:32:10]

This is something James requested I change in my previous CL.  In
DigestInternal, I do a similar thing with HASH_ResultLenContext(), and it was
requested to be made into size_t.

+
+      DCHECK_EQ(sym_key->mechanism(),
+                WebCryptoAlgorithmToNSSMechanism(params->hash()));
+
+      result = WebKit::WebArrayBuffer::create(digest_length, 1);
+
+      SECItem param_item = { siBuffer, NULL, 0 };
+      SECItem data_item = {
+        siBuffer,
+        const_cast<unsigned char*>(data),
+        data_size
+      };
+      SECItem signature_item = {
+        siBuffer,
+        reinterpret_cast<unsigned char*>(result.data()),
+        digest_length

[eroman, 2013/09/06 23:03:16]

I think this is more readable as result.byteLength().

[Bryan Eyler, 2013/09/09 22:32:10]

Done.

+      };
+
+      if (PK11_SignWithSymKey(const_cast<PK11SymKey*>(sym_key->key()),

[eroman, 2013/09/06 23:03:16]

Commented on other patchset

+                              sym_key->mechanism(),
+                              &param_item,
+                              &signature_item,
+                              &data_item) != SECSuccess) {
+        NOTREACHED();
+        return false;
+      }
+
+      break;
+    }
+    default:
+      return false;
+  }
+
+  *buffer = result;
+  return true;
+}
+
 }  // namespace content