[webcrypto] Add RSAES-PKCS1-v1_5 encrypt and decrypt for NSS.

content/renderer/webcrypto/webcrypto_impl_nss.cc

Index: content/renderer/webcrypto/webcrypto_impl_nss.cc
diff --git a/content/renderer/webcrypto/webcrypto_impl_nss.cc b/content/renderer/webcrypto/webcrypto_impl_nss.cc
index 1590a09961558af8bc8d14fd7f28b86ea432e7aa..d0f1fc0bff33351eef0e730dda902a6bf2e74022 100644
--- a/content/renderer/webcrypto/webcrypto_impl_nss.cc
+++ b/content/renderer/webcrypto/webcrypto_impl_nss.cc
@@ -250,9 +250,47 @@ bool WebCryptoImpl::EncryptInternal(
     const unsigned char* data,
     unsigned data_size,
     blink::WebArrayBuffer* buffer) {
+
+  DCHECK_EQ(algorithm.id(), key.algorithm().id());
+  DCHECK(key.handle());
+  DCHECK(buffer);
+
   if (algorithm.id() == blink::WebCryptoAlgorithmIdAesCbc) {
     return AesCbcEncryptDecrypt(
         CKA_ENCRYPT, algorithm, key, data, data_size, buffer);
+  } else if (algorithm.id() == blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5) {
+
+    // RSAES encryption does not support empty input
+    if (!data_size)
+      return false;
+    DCHECK(data);
+
+    if (!key.type() == blink::WebCryptoKeyTypePublic)

[eroman, 2013/11/21 00:18:26]

I imagine you meant to use != (since as written this expression doesn't make
much sense).

This is an interesting candidate for a clang compiler warning; I will ask thakis
about it.

[eroman, 2013/11/21 01:02:15]

FYI: Nico showed me that clang already has a warning for this:
  -Wlogical-not-parentheses

[padolph, 2013/11/21 01:56:11]

Doh! Not sure how that happened.

[padolph, 2013/11/21 01:56:11]

Thanks. I only use clang sometimes. I find it _much_ slower than gcc on my
system. I will try to run it more often, especially on fresh code like this.

+      return false;  // TODO(padolph): should be DCHECK?

[eroman, 2013/11/21 00:18:26]

Can remove the TODO: should not be a DCHECK, since the blink side does not
validate this aspect of the keys before calling into here.

+
+    PublicKeyHandle* const public_key =
+        reinterpret_cast<PublicKeyHandle*>(key.handle());
+
+    const unsigned encrypted_length_bytes =
+        SECKEY_PublicKeyStrength(public_key->key());
+
+    // RSAES can operate on messages up to a length of k - 11, where k is the
+    // octet length of the RSA modulus.
+    if (encrypted_length_bytes < data_size + 11)

[eroman, 2013/11/21 00:18:26]

paranoia: what if data_size + 11 overflows? Could lead to PubEncryptPKCS1
writing beyond the buffer.

Could you express this in an overflow safe manner, perhaps:

if (encrypted_length_bytes < 11 || encrypted_length_bytes - 11 < data_size)
  return false;

[padolph, 2013/11/21 01:56:11]

Done.

[padolph, 2013/11/21 01:56:11]

Done.

+      return false;
+
+    *buffer = blink::WebArrayBuffer::create(encrypted_length_bytes, 1);
+    unsigned char* const buffer_data =
+        reinterpret_cast<unsigned char*>(buffer->data());
+
+    if (PK11_PubEncryptPKCS1(public_key->key(),
+                             buffer_data,
+                             const_cast<unsigned char*>(data),
+                             data_size,
+                             NULL) != SECSuccess) {
+      return false;
+    }
+    return true;
   }
 
   return false;
@@ -264,9 +302,50 @@ bool WebCryptoImpl::DecryptInternal(
     const unsigned char* data,
     unsigned data_size,
     blink::WebArrayBuffer* buffer) {
+
+  DCHECK_EQ(algorithm.id(), key.algorithm().id());
+  DCHECK(key.handle());
+  DCHECK(buffer);
+
   if (algorithm.id() == blink::WebCryptoAlgorithmIdAesCbc) {
     return AesCbcEncryptDecrypt(
         CKA_DECRYPT, algorithm, key, data, data_size, buffer);
+  } else if (algorithm.id() == blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5) {
+
+    // RSAES decryption does not support empty input
+    if (!data_size)
+      return false;
+    DCHECK(data);
+
+    if (!key.type() == blink::WebCryptoKeyTypePrivate)

[eroman, 2013/11/21 00:18:26]

Same comment as above. You surely meant either X != Y or !(X == Y)

[padolph, 2013/11/21 01:56:11]

Done.

[padolph, 2013/11/21 01:56:11]

Done.

+      return false;  // TODO(padolph): should be DCHECK instead?
+
+    PrivateKeyHandle* const private_key =
+        reinterpret_cast<PrivateKeyHandle*>(key.handle());
+
+    const int modulus_length_bytes =
+        PK11_GetPrivateModulusLen(private_key->key());
+    if (modulus_length_bytes < 0)  // TODO(padolph): should be a DCHECK only?

[eroman, 2013/11/21 00:18:26]

Should this check <= 0?

[padolph, 2013/11/21 01:56:11]

NSS returns -1 in case of error. Returning 0 is a non-error for
PK11_GetPrivateModulusLen() but would indicate a programming error around the
use of keys with this method.

However, I see your point, this distinction is not very important for this
method.

Changed.

+      return false;
+    DCHECK(modulus_length_bytes);
+    const unsigned& max_output_length_bytes = modulus_length_bytes;

[eroman, 2013/11/21 00:18:26]

Rather than a reference, either make a copy or remove this variable all
together. (No performance or readability improvement by using a reference that I
can tell).

[padolph, 2013/11/21 01:56:11]

Done.

+
+    *buffer = blink::WebArrayBuffer::create(max_output_length_bytes, 1);
+    unsigned char* const buffer_data =
+        reinterpret_cast<unsigned char*>(buffer->data());
+
+    unsigned output_length_bytes = 0;
+    if (PK11_PrivDecryptPKCS1(private_key->key(),
+                              buffer_data,
+                              &output_length_bytes,
+                              max_output_length_bytes,
+                              const_cast<unsigned char*>(data),
+                              data_size) != SECSuccess) {
+      return false;
+    }
+    DCHECK_LE(output_length_bytes, max_output_length_bytes);
+    WebCryptoImpl::ShrinkBuffer(buffer, output_length_bytes);
+    return true;
   }
 
   return false;