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..f9116a0d604cbe9d82e1c253530191eb301773b9 100644
--- a/content/renderer/webcrypto_impl_nss.cc
+++ b/content/renderer/webcrypto_impl_nss.cc
@@ -4,15 +4,40 @@
 
 #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 {
 
+class WebCryptoSymKeyHandle : public WebKit::WebCryptoKeyHandle {
+ public:
+  void set_mechanism(CK_MECHANISM_TYPE mechanism) {
+    mechanism_ = mechanism;

[eroman, 2013/09/05 01:57:51]

What is the initial value? I would rather this be an attribute of the
constructor so it has to be specified.

[Bryan Eyler, 2013/09/06 01:27:51]

Yeah, initial value appears to be not clear.  Moved to constructor.

+  }
+  void set_slot(PK11SlotInfo* slot) {

[eroman, 2013/09/05 01:57:51]

I don't see any current need to specify a slot other than that from
GetInternalSlot(). To that end, can the slot be set automatically by the
constructor?

[Bryan Eyler, 2013/09/06 01:27:51]

Sure, makes sense.

+    slot_.reset(slot);
+  }
+  void set_key(PK11SymKey* key) {

[eroman, 2013/09/05 01:57:51]

This effectively passes ownership of the key. I think it would be more
expressive to have it take a ScopedPK11SymKey key here, and do a "move".

Just thinking out loud right now, but to avoid "moving" stuff around while
building the key, another code organization would be to have a factory method on
WebCryptoSymKeyHandle, like WebCryptoSymKeyHandle* CreateByImporting(...). Just
throwing it out as an idea, not sure which design is cleaner :) 

[Bryan Eyler, 2013/09/06 01:27:51]

It gets complicated as the slot is needed for creating the key, so if we want
the GetInternalSlot() to be part of the constructor, we can't have a factory
whose arguments depend on that.

We could have a factory that takes in the slot?

In the meantime, I've made this a ScopedPK11SymKey to make the tracking better.

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

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

Don't make the pointer const. Then there is no need to const-cast later!

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

Done.

+
+ private:
+  CK_MECHANISM_TYPE mechanism_;
+  crypto::ScopedPK11Slot slot_;
+  crypto::ScopedPK11SymKey key_;
+};
+
 bool WebCryptoImpl::digestInternal(
     const WebKit::WebCryptoAlgorithm& algorithm,
     const unsigned char* data,
@@ -67,4 +92,162 @@ 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,
+    bool extractable,
+    WebKit::WebCryptoKeyUsageMask usage_mask,
+    WebKit::WebCryptoKeyHandle** handle) {
+  // TODO(bryaneyler): Need to split handling for symmetric and asymmetric keys.
+  // Currently only supporting symmetric.
+  scoped_ptr<WebCryptoSymKeyHandle> sym_key(new WebCryptoSymKeyHandle);
+
+  switch(algorithm.id()) {
+    case WebKit::WebCryptoAlgorithmIdHmac: {
+      WebKit::WebCryptoHmacKeyParams* params = algorithm.hmacKeyParams();

[eroman, 2013/09/05 01:57:51]

Ah! I should have defined these as "const" pointers in the API. Please assume
they are "const", I will update the public API to fix this :)

[Bryan Eyler, 2013/09/06 01:27:51]

Done.

+      if (!params) {
+        return false;
+      }
+
+      switch (params->hash().id()) {

[eroman, 2013/09/05 01:57:51]

You will have to coordinate with Elly's changelist here (she is added an
algorithm information table which enumerates these properties). You can merge
that once it ends up landing (tough timing!)

[Bryan Eyler, 2013/09/06 01:27:51]

Yeah, I realized there would be a bit of overlap/merge issues.

+        case WebKit::WebCryptoAlgorithmIdSha1:
+          sym_key->set_mechanism(CKM_SHA_1_HMAC);
+          break;
+        case WebKit::WebCryptoAlgorithmIdSha256:
+          sym_key->set_mechanism(CKM_SHA256_HMAC);
+          break;
+        default:
+          return false;
+      }
+      break;
+    }
+    default:
+      return false;
+  }
+
+  sym_key->set_slot(PK11_GetInternalSlot());
+  if (!sym_key->slot()) {
+    NOTREACHED();
+    return false;
+  }
+
+  CK_FLAGS flags =
+      ((usage_mask & WebKit::WebCryptoKeyUsageEncrypt) ? CKF_ENCRYPT : 0) |

[eroman, 2013/09/05 01:57:51]

sleevi: My understanding is the NSS usage flags are not really respected.

It certainly seems prudent to set them, in case it has any effect on some
internals for how the key is provisioned.

But curious if this has an effect.

bryaneyler: I suggest extracting this to a helper function since it will surely
be used elsewhere. Also as an aside, the Blink layer enforces the key usage
before delegating operations to the embedding layer, so it will not be necessary
to verify key usage mask in each operation (other than asserting if you wanted
to).

[Bryan Eyler, 2013/09/06 01:27:51]

I tried not setting the appropriate flags and it failed, so I think this is
necessary to set them.  Alternatively, we could just set them all if you think
we should just rely on Blink?

[Ryan Sleevi, 2013/09/06 19:39:02]

Not for softoken (effectively), but definitely for other token types. The
correct masks should be set.

+      ((usage_mask & WebKit::WebCryptoKeyUsageDecrypt) ? CKF_DECRYPT : 0) |
+      ((usage_mask & WebKit::WebCryptoKeyUsageSign) ? CKF_SIGN : 0) |
+      ((usage_mask & WebKit::WebCryptoKeyUsageVerify) ? CKF_VERIFY : 0) |
+      ((usage_mask & WebKit::WebCryptoKeyUsageDeriveKey) ? CKF_DERIVE : 0) |
+      ((usage_mask & WebKit::WebCryptoKeyUsageWrapKey) ? CKF_WRAP : 0) |
+      ((usage_mask & WebKit::WebCryptoKeyUsageUnwrapKey) ? CKF_UNWRAP : 0);
+
+  unsigned char* raw_key_data = NULL;
+  size_t raw_key_data_size = 0;
+
+  switch (format) {
+    case WebKit::WebCryptoKeyFormatRaw:
+      raw_key_data_size = key_data_size;
+      raw_key_data = new unsigned char[raw_key_data_size];
+      memcpy(raw_key_data, key_data, raw_key_data_size);

[eroman, 2013/09/05 01:57:51]

This is unnecessary. You can just pass a SECItem which references the data
directly, no copying necessary. 

sleevi: Right?

[Bryan Eyler, 2013/09/06 01:27:51]

Yes, true.  Done.

[Ryan Sleevi, 2013/09/06 19:39:02]

Right

+      break;
+    // TODO(bryaneyler): Handle additional formats.
+    default:
+      return false;
+  }
+
+  SECItem key_item = {
+    siBuffer,
+    raw_key_data,
+    raw_key_data_size
+  };
+
+  sym_key->set_key(
+      PK11_ImportSymKeyWithFlags(sym_key->slot(),
+                                 sym_key->mechanism(),
+                                 PK11_OriginUnwrap,
+                                 CKA_FLAGS_ONLY,
+                                 &key_item,
+                                 flags,
+                                 false,
+                                 NULL));
+  if (!sym_key->key()) {
+    NOTREACHED();
+    delete[] raw_key_data;

[eroman, 2013/09/05 01:57:51]

See my comment above. We don't want to duplicate the key data. Moreover if we
did, you would want to use a scoped container to handle the subsequent deletion.

[Bryan Eyler, 2013/09/06 01:27:51]

Done.

+    return false;
+  }
+
+  *handle = sym_key.release();
+
+  delete[] raw_key_data;

[eroman, 2013/09/05 01:57:51]

See above. We try hard in Chromium not to do manual memory management of this
sort, as it is very easy to get wrong.

[Bryan Eyler, 2013/09/06 01:27:51]

Done.

+  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: {
+      WebKit::WebCryptoHmacParams* params = algorithm.hmacParams();

[eroman, 2013/09/05 01:57:51]

const-ify please. I will change the API to disallow non-const accesses :)

[Bryan Eyler, 2013/09/06 01:27:51]

Done.

+      if (!params) {
+        return false;
+      }
+
+      const WebCryptoSymKeyHandle* sym_key =
+          reinterpret_cast<const WebCryptoSymKeyHandle*>(key);
+
+      size_t digest_length = 0;
+      switch (params->hash().id()) {
+        case WebKit::WebCryptoAlgorithmIdSha1:
+          digest_length = 20;

[eroman, 2013/09/05 01:57:51]

Rather than hardcode this, can you use a function to retrieve it?

For instance,  HASH_ResultLen(). Note that the digest() code above already has a
switch statement to convert WebCryptoAlgorithmId --> HASH_HashType, so you can
probably just extract that to a helper function.

sleevi: Or is there a better way that you recommend?

[Bryan Eyler, 2013/09/06 01:27:51]

Done.  In order to do the DCHECK, I also needed to extract out the mechanism,
which may also come in handy later on.

[Ryan Sleevi, 2013/09/06 19:39:02]

+1 to your recommended way.

+          DCHECK_EQ(sym_key->mechanism(),
+                    static_cast<CK_MECHANISM_TYPE>(CKM_SHA_1_HMAC));
+          break;
+        case WebKit::WebCryptoAlgorithmIdSha256:
+          digest_length = 32;
+          DCHECK_EQ(sym_key->mechanism(),
+                    static_cast<CK_MECHANISM_TYPE>(CKM_SHA256_HMAC));
+          break;
+        default:
+          return false;
+      }
+
+      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
+      };
+
+      if (PK11_SignWithSymKey(const_cast<PK11SymKey*>(sym_key->key()),

[eroman, 2013/09/05 01:57:51]

Please make key() return a non-const PK11SymKey rather than const-casting.

[Bryan Eyler, 2013/09/06 01:27:51]

I need to const_cast at some point because the key passed in is a const.  Should
I const_cast when casting to sym_key?

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

Yep, make sym_key non-const, and then change the method on SymKeyHandle to
return a non-const pointer. Shouldn't need a const-cast here.

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

Done.

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