WebCrypto: Implement digest() using NSS

content/renderer/webcrypto_impl.cc

Index: content/renderer/webcrypto_impl.cc
diff --git a/content/renderer/webcrypto_impl.cc b/content/renderer/webcrypto_impl.cc
index 80c551240eeb141a4c5b305f9485e5df2c241b6e..c27647b8a6929bc6865abd3651fcf00f8ea2020f 100644
--- a/content/renderer/webcrypto_impl.cc
+++ b/content/renderer/webcrypto_impl.cc
@@ -4,24 +4,85 @@
 
 #include "content/renderer/webcrypto_impl.h"
 
+#if defined(USE_NSS)
+#include <sechash.h>
+#endif
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"

[eroman, 2013/08/22 02:25:05]

unused; remove

[Bryan Eyler, 2013/08/22 18:51:03]

Done.

+#include "crypto/nss_util.h"
+#include "third_party/WebKit/public/platform/WebArrayBuffer.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
 
 namespace content {
 
-WebKit::WebCryptoOperation* WebCryptoImpl::digest(
-    const WebKit::WebCryptoAlgorithm& algorithm) {
+void WebCryptoImpl::digest(
+    const WebKit::WebCryptoAlgorithm& algorithm,
+    const unsigned char* data,
+    size_t data_size,
+    WebKit::WebCryptoResult result) {
+// TODO(bryaneyler): Also include these in OpenSSL build.  Issue 267888.
+#if defined(USE_NSS)

[eroman, 2013/08/22 02:25:05]

Because this is quickly going to turn into a lot of defines, I suggest splitting
it into a separate file from the get-go

* webcrypto_impl.h defines the class
* webcrypto_impl_nss.cc has the method definitions (for NSS)
* webcrypto_impl_openssl.cc has the method definitions (for OpenSSL)
* webcrypto_impl_unittest.cc has the tests which apply to WebCryptoImpl (shared
by both NSS and openssl).

[Bryan Eyler, 2013/08/22 18:51:03]

Done.

+  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:
-      // TODO(eroman): Implement.
-      return NULL;
+      hash_type = HASH_AlgSHA512;
+      break;
     default:
       // Not a digest algorithm.
-      return NULL;
+      result.completeWithError();
+      return;
   }
+
+  crypto::EnsureNSSInit();
+
+  HASHContext* context = HASH_Create(hash_type);
+  if (!context) {
+    LOG(ERROR) << "Could not create digest context for hash algorithm: "

[eroman, 2013/08/22 02:25:05]

I don't think there is value to having these LOG(ERROR) and suggest deleting
them.

(In a future update of the API I will add a string parameter to
completeWithError() so any relevant information can be stuffed into the
javascript's exception message.)

[Bryan Eyler, 2013/08/22 18:51:03]

Done.

+               << hash_type;
+    result.completeWithError();
+    return;
+  }
+
+  HASH_Begin(context);
+
+  HASH_Update(context, data, data_size);
+
+  unsigned int hash_result_length = HASH_ResultLenContext(context);

[jamesr, 2013/08/22 02:31:01]

i think size_t is a better type, but you don't want that at least use an
explicitly sized type

[Bryan Eyler, 2013/08/22 18:51:03]

Changed to size_t.  FWIW, I was trying to match the return type of
HASH_ResultLenContext().

+  WebKit::WebArrayBuffer buffer(
+      WebKit::WebArrayBuffer::create(hash_result_length, 1));
+
+  unsigned char* digest = reinterpret_cast<unsigned char*>(buffer.data());
+  DCHECK(digest);

[eroman, 2013/08/22 02:25:05]

I think the only situation when buffer.data() can be NULL is if (length * size)
in the call to WebArrayBuffer::create() above causes 32-bit integer overflow.

Probably safest to handle it explicitly instead of the DCHECK:

if (!digest) {
  result.completeWithError();
  return;
}

[jamesr, 2013/08/22 02:31:01]

I'm not sure this DCHECK makes sense. WebArrayBuffer::create() will only return
a null buffer if you request it to allocate > 2^32 bytes of data, which seems
crazy for the length of a hash result. I think it'd be better to DCHECK that
your inputs are sane if you aren't sure

[Bryan Eyler, 2013/08/22 18:51:03]

DCHECK changed to sanity check hash_result_length.

+
+  unsigned int result_length = 0;

[jamesr, 2013/08/22 02:31:01]

please use a better type

[Bryan Eyler, 2013/08/22 18:51:03]

I've made this uint32 for lack of a better type.  size_t won't typecast properly
to be passed into HASH_End().

+  HASH_End(context, digest, &result_length, hash_result_length);
+  if (result_length != hash_result_length) {
+    LOG(ERROR) << "Result length invalid; expected " << hash_result_length

[eroman, 2013/08/22 02:25:05]

LOG(ERROR) have very little usefulness in my experience, I suggest deleting (and
we will pass the details to completeWithError in the future).

[Bryan Eyler, 2013/08/22 18:51:03]

Done.

+               << ", got " << result_length;
+    result.completeWithError();
+    return;
+  }
+
+  result.completeWithBuffer(buffer);
+
+#else
+  // No way to process.
+  result.completeWithError();
+#endif
 }
 
 }  // namespace content