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..afd883b909ed81641eec6d2bb05a16983027485b 100644
--- a/content/renderer/webcrypto_impl_nss.cc
+++ b/content/renderer/webcrypto_impl_nss.cc
@@ -4,41 +4,101 @@
 
 #include "content/renderer/webcrypto_impl.h"
 
-#include <sechash.h>
+#include <nss/sechash.h>
+#include <nss/pk11pub.h>

[eroman, 2013/09/09 23:00:48]

keep these in sorted order (unless there is a header ordering dependency)

[Bryan Eyler, 2013/09/10 01:15:54]

Done.

 
 #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:
+  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);
+};
+
+namespace {
+
+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);
+}
+
+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;
+  }
+}
+
+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;

[Ryan Sleevi, 2013/09/10 00:42:49]

FYI: In order to verify truncated HMACs, you'll need to use the
CK_MAC_GENERAL_PARAMS with CKM_SHA*_HMAC_GENERAL

See 6.19.3 of PKCS#11 v 2.30 draft 7 - namely, it behaves exactly the same as
CKM_SHA256_HMAC, but is more general.

[Bryan Eyler, 2013/09/10 01:15:54]

If I rely on the signing to get the length (as described in the comments later
in the file), then I should keep this as the non-general versions, correct?

+    default:
+      // Not a supported algorithm.
+      return CKM_INVALID_MECHANISM;
+  }
+}
+
+}

[eroman, 2013/09/09 23:00:48]

nit: please add the comment:
}  // namespace

[Bryan Eyler, 2013/09/10 01:15:54]

Done.

+
+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 +127,144 @@ 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));
+
+      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,
+    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;
+      }
+
+      WebCryptoSymKeyHandle* sym_key =
+          const_cast<WebCryptoSymKeyHandle*>(
+              reinterpret_cast<const WebCryptoSymKeyHandle*>(key));
+
+      HASH_HashType hash_type = WebCryptoAlgorithmToNSSHashType(params->hash());

[Ryan Sleevi, 2013/09/10 00:42:49]

BUG: You have a lack of consistency between WebCryptoAlgorithmToNSSHashType and
WebCryptoAlgorithmToNSSMechanism

In particular, NSSMechanism only allows CKM_SHA1/256_HMAC, but NSSHashType
allows the SHA-224/384/512 types.

[eroman, 2013/09/10 00:57:56]

In practice this isn't a bug, since the Blink side verifies that
key.algorithm.hash matches algorithm.hash for HMAC before passing it to the
embedder (hence it will only be called for the subset of hashes that are
supported).

That being said, I do like your alternative to not calling HASH_ResultLen() in
the first place.

+1

[Bryan Eyler, 2013/09/10 01:15:54]

Done as per next comment.

+      if (hash_type == HASH_AlgNULL) {
+        return false;
+      }
+
+      size_t digest_length = HASH_ResultLen(hash_type);

[Ryan Sleevi, 2013/09/10 00:42:49]

Rather than computing the expected length via digest_length, if you supply a
NULL buffer for the output (namely, sig->data), then what will happen is
sig->len will be updated with the appropriate length needed for the signature.

This is because PK11_SignWithSymKey is a wrapper for C_Sign, and C_Sign (and
C_SignFinal) uses the convention described in Section 11.2 for producing output.

This will be the most flexible solution for working with any form of symmetric
key, and avoids the whole hash concerns raised in the BUG above.

[Bryan Eyler, 2013/09/10 01:15:54]

Done.

+
+      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()),
+        result.byteLength()
+      };
+
+      if (PK11_SignWithSymKey(sym_key->key(),
+                              sym_key->mechanism(),
+                              &param_item,
+                              &signature_item,
+                              &data_item) != SECSuccess) {
+        NOTREACHED();
+        return false;
+      }
+
+      break;
+    }
+    default:
+      return false;
+  }
+
+  *buffer = result;
+  return true;
+}
+
 }  // namespace content