WebCrypto: Implement digest() using NSS

content/renderer/webcrypto_sha_digest_nss.cc

+// Copyright (c) 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_sha_digest.h"
+
+#include <pk11pub.h>
+
+#include "base/logging.h"
+#include "crypto/nss_util.h"
+#include "third_party/WebKit/public/platform/WebArrayBuffer.h"
+#include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
+
+namespace content {
+

[Ryan Sleevi, 2013/07/31 18:19:19]

I'm a bit mixed on using the PK11 functions directly.

NSS offers a high-level hash interface that hides the PKCS#11 details - namely
http://mxr.mozilla.org/nss/source/lib/cryptohi/sechash.h - that more closely
matches the OpenSSL interface.

However, it means an extra level of function pointer indirection during the call
sequencing, so it may not be worth it.

[Bryan Eyler, 2013/08/02 00:49:05]

I didn't know about this interface.  Looks a bit cleaner to me.

Changed to this interface.

+WebCryptoSHADigest::WebCryptoSHADigest(
+    const WebKit::WebCryptoAlgorithmId algorithm_id,
+    WebKit::WebCryptoOperationResult& result)
+    : result_(result),
+      context_(NULL),
+      hash_algorithm_(SEC_OID_UNKNOWN),
+      hash_result_length_(0) {
+  switch (algorithm_id) {
+    case WebKit::WebCryptoAlgorithmIdSha1:
+      hash_algorithm_ = SEC_OID_SHA1;
+      hash_result_length_ = 20;

[Ryan Sleevi, 2013/07/31 18:19:19]

If you keep with the OID formation, all of these constants should be replaced
with their NSS header equivalents

SHA1_LENGTH
SHA224_LENGTH
SHA256_LENGTH
SHA384_LENGTH
SHA512_LENGTH

These all come from http://mxr.mozilla.org/nss/source/lib/util/hasht.h#35

[Bryan Eyler, 2013/08/02 00:49:05]

Done.

+      break;
+    case WebKit::WebCryptoAlgorithmIdSha224:
+      hash_algorithm_ = SEC_OID_SHA224;
+      hash_result_length_ = 28;
+      break;
+    case WebKit::WebCryptoAlgorithmIdSha256:
+      hash_algorithm_ = SEC_OID_SHA256;
+      hash_result_length_ = 32;
+      break;
+    case WebKit::WebCryptoAlgorithmIdSha384:
+      hash_algorithm_ = SEC_OID_SHA384;
+      hash_result_length_ = 48;
+      break;
+    case WebKit::WebCryptoAlgorithmIdSha512:
+      hash_algorithm_ = SEC_OID_SHA512;
+      hash_result_length_ = 64;
+      break;
+    default:
+      NOTREACHED();
+      hash_algorithm_ = SEC_OID_UNKNOWN;
+      hash_result_length_ = 0;
+  }
+}
+
+WebCryptoSHADigest::~WebCryptoSHADigest() {
+  if (context_) {
+    PK11_DestroyContext(context_, PR_TRUE);
+  }
+}
+
+bool WebCryptoSHADigest::Initialize() {
+  crypto::EnsureNSSInit();
+
+  context_ = PK11_CreateDigestContext(hash_algorithm_);
+  if (!context_) {
+    LOG(ERROR) << "Could not create digest context for hash algorithm: "
+               << hash_algorithm_;

[Ryan Sleevi, 2013/07/31 18:19:19]

Seems that this is unused beyond the constructor - there's no need to store this
in a member field (thus there's no reason to need the NSS header in this files
header)

[Bryan Eyler, 2013/08/02 00:49:05]

Make sense.  I want to keep the creation of the context within the Initialize()
so that the error can be propagated on failure to create, so I've made the
WebCryptoAlgorithm a parameter to Initialize() instead of the constructor.

+    return false;
+  }
+
+  if (PK11_DigestBegin(context_) != SECSuccess) {
+    LOG(ERROR) << "Could not initialize digest context.";
+    return false;
+  }
+
+  return true;
+}
+
+void WebCryptoSHADigest::process(const unsigned char* bytes, size_t size) {
+  DCHECK(context_);
+
+  if (PK11_DigestOp(context_, bytes, size) != SECSuccess) {

[eroman, 2013/07/31 02:23:50]

Is it safe to call this when |bytes| is garbage, but size == 0?

[Ryan Sleevi, 2013/07/31 18:19:19]

It should be.

[Bryan Eyler, 2013/08/02 00:49:05]

Test added for this.

+    LOG(ERROR) << "Could not process digest contents of size: " << size;
+    result_.completeWithError();

[eroman, 2013/07/31 02:23:50]

delete this;

(Any time completing the operation, it must be deleted).

I am playing with some other models to maybe simplify this in the future, but
the current API leaves memory management of the operation in the hands of the
embedder.

[Bryan Eyler, 2013/08/02 00:49:05]

Obsolete now.

+  }
+}
+
+void WebCryptoSHADigest::abort() {
+  delete this;
+}
+
+void WebCryptoSHADigest::finish() {
+  DCHECK(context_);
+
+  unsigned char* digest = NULL;

[Ryan Sleevi, 2013/07/31 18:19:19]

Move this to line 96 (keep declaration and definition close)

[Bryan Eyler, 2013/08/02 00:49:05]

Done.

+
+  WebKit::WebArrayBuffer buffer(
+      WebKit::WebArrayBuffer::create(hash_result_length_, 1));

[Ryan Sleevi, 2013/07/31 18:19:19]

Same here - hash_result_length_ is unused outside of this function. It would be
better to just use a TU-local function (eg: unnamed namespace) to look up the
result length.

[eroman, 2013/07/31 21:10:35]

This was based on my recommendation, so that he have just 1 switch statement
during initialization, rather than 2. Initially Bryan had a separate switch to
lookup the result length.

I am OK with either approach, just feel bad if Bryan is being pulled in opposing
directions :)

+
+  digest = reinterpret_cast<unsigned char*>(buffer.data());
+  DCHECK(digest);
+
+  unsigned int result_length = 0;
+  if (PK11_DigestFinal(context_, digest, &result_length, hash_result_length_)
+      != SECSuccess || result_length != hash_result_length_) {

[Ryan Sleevi, 2013/07/31 18:19:19]

Seems like this should match Chromium style (the != would be on the previous
line)

clang-format to the rescue?

[Bryan Eyler, 2013/08/02 00:49:05]

Obsolete after moving to new interface.  Will look into the clang-format tool.

+    LOG(ERROR) << "Could not finalize digest data.";
+    result_.completeWithError();

[eroman, 2013/07/31 02:23:50]

delete this;

[Bryan Eyler, 2013/08/02 00:49:05]

Done.

+    return;
+  }
+
+  result_.completeWithArrayBuffer(buffer);
+
+  delete this;
+}
+
+}  // namespace content