998803 - Add support for RSA encryption and signing to WebCrypto API

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998803.patch

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8409964 [details] [diff] [review]
webcrypto-998803.patch

Review of attachment 8409964 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/WebCryptoTask.cpp
@@ +244,5 @@
>      return rv;
>    }
>  };
>  
> +class RsaesPkcs1Task : public ReturnArrayBufferViewTask

this name is kind of useless (RSAEncryptDescritPKCS1Task / RSACryptPKCS1Task?)

@@ +263,5 @@
> +      }
> +      mStrength = SECKEY_PublicKeyStrength(mPubKey);
> +
> +      // TODO Verify that the data input is not too big
> +      if (mData.Length() > mStrength-11) {

11? No magic numbers. If it is a constant, name it so that future readers know why.

@@ +406,5 @@
>      mResultPromise.forget();
>    }
>  };
>  
> +class RsassaPkcs1Task : public WebCryptoTask

Ditto: class name is kind of not useful:
RSAAuthenticationPKCS1Task?

@@ +494,5 @@
> +      rv = MapSECStatus(VFY_Update(ctx, mData.Elements(), mData.Length()));
> +      if (NS_FAILED(rv)) { return NS_ERROR_FAILURE; }
> +      rv = MapSECStatus(VFY_End(ctx));
> +      mVerified = NS_SUCCEEDED(rv);
> +      rv = NS_OK;

why not failuire?

::: dom/crypto/WebCryptoTask.h
@@ -33,5 @@
>  5. Resolve() or FailWithError()
>  
>  If any of these steps produces an error (setting mEarlyRv), then
>  subsequent steps will not proceed.  If the constructor or BeforeCrypto
> -sets mEarlyComplete to true, then we will skip step 3, saving the 

please move cleanup to the bug where this is inserted.

Comment 3

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998803.1.patch

Updated patch, rebasing after landing of initial WebCrypto patches, and (hopefully) incorporating lessons learned from those.

Comment 4

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8426481 [details] [diff] [review]
webcrypto-998803.1.patch

Review of attachment 8426481 [details] [diff] [review]:
-----------------------------------------------------------------

The NSS usages look good to me, except for one comment on allocating a SECItem (see below). I also noted a few nits and some areas where it would be good to document a bit more. r=me with those addressed.

::: dom/crypto/WebCryptoTask.cpp
@@ +271,5 @@
> +{
> +public:
> +  RsaesPkcs1Task(JSContext* aCx, const ObjectOrString& aAlgorithm,
> +             mozilla::dom::Key& aKey, const CryptoOperationData& aData,
> +             bool aEncrypt)

nit: indentation on these two lines

@@ +285,5 @@
> +      }
> +      mStrength = SECKEY_PublicKeyStrength(mPubKey);
> +
> +      // Verify that the data input is not too big
> +      if (mData.Length() > mStrength-11) {

a). Spaces around operators: "mStrength - 11"
b). This should be documented (why 11? (is there more reasoning than "it's in the rfc"?))

@@ +312,5 @@
> +    nsresult rv;
> +    mResult.SetLength(mStrength);
> +    if (mEncrypt) {
> +      rv = MapSECStatus(PK11_PubEncryptPKCS1(
> +              mPubKey.get(), mResult.Elements(),

It would be good to document why mStrength will be enough to hold the resulting encrypted data.

@@ +324,5 @@
> +              mData.Elements(), mData.Length()));
> +      mResult.SetLength(outLen);
> +    }
> +
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_OPERATION_ERR);

bz can probably speak more definitively about this, but my understanding is NS_ENSURE_SUCCESS is deprecated. It's longer, but this might be preferred in general:

if (NS_FAILED(rv) {
  return NS_ERROR_DOM_OPERATION_ERR;
}

(in this case, you could probably just do "return NS_FAILED(rv) ? NS_ERROR_DOM_OPERATION_ERR : NS_OK;")

@@ +422,5 @@
> +  RsassaPkcs1Task(JSContext* aCx, const ObjectOrString& aAlgorithm,
> +           mozilla::dom::Key& aKey,
> +           const CryptoOperationData& aSignature,
> +           const CryptoOperationData& aData,
> +           bool aSign)

nit: indentation on these four lines

@@ +427,5 @@
> +    : mPrivKey(aKey.GetPrivateKey())
> +    , mPubKey(aKey.GetPublicKey())
> +    , mSignature(aSignature)
> +    , mData(aData)
> +    , mSign(aSign)

Its probably a good idea to explicitly initialize all of the other members (e.g. mVerified).

@@ +430,5 @@
> +    , mData(aData)
> +    , mSign(aSign)
> +  {
> +    // Look up the SECOidTag based on the KeyAlgorithm
> +    nsString hash;

What is "hash" used for, here?

@@ +432,5 @@
> +  {
> +    // Look up the SECOidTag based on the KeyAlgorithm
> +    nsString hash;
> +    nsRefPtr<KeyAlgorithm> keyAlg = aKey.Algorithm();
> +    nsRefPtr<RsaHashedKeyAlgorithm> rsaAlg = static_cast<RsaHashedKeyAlgorithm*>(keyAlg.get());

Are we guaranteed to get an RSA key here? Should we check the type first?

@@ +454,5 @@
> +      }
> +    }
> +
> +    // Check that we have the appropriate key
> +    if ((mSign && !mPrivKey)||

nit: space before "||" (in fact, this can probably all be one line)

@@ +474,5 @@
> +  virtual nsresult DoCrypto() MOZ_OVERRIDE
> +  {
> +    nsresult rv;
> +    if (mSign) {
> +      ScopedSECItem signature(new SECItem());

This should be PORT_Alloc(sizeof(SECItem)) instead of new (since the ScopedSECItem will free it with PORT_Free).

@@ +486,5 @@
> +
> +      rv = MapSECStatus(SGN_Update(ctx, mData.Elements(), mData.Length()));
> +      NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_OPERATION_ERR);
> +
> +      rv = MapSECStatus(SGN_End(ctx.get(), signature.get()));

Why does SGN_End need "ctx.get()" but SGN_Begin and SGN_Update are happy with just "ctx"?

@@ +494,5 @@
> +    } else {
> +      ScopedSECItem signature(mSignature.ToSECItem());
> +      ScopedVFYContext ctx(VFY_CreateContext(mPubKey, signature.get(),
> +                                             mOidTag, nullptr));
> +      if (!ctx) {

Above, we have "if (!ctx.get())" - let's be consistent if either works.

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ +121,5 @@
>    PK11_DestroyContext(ctx, true);
>  }
>  
> +inline void
> +SGN_DestroyContext_true(SGNContext * ctx) {

nit: * next to type name (so "SGNContext*", "VFYContext*")
I know it's inconsistent with what's there, but let's try and go with what we want the style to be going forward.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8426481 [details] [diff] [review]
webcrypto-998803.1.patch

>+++ b/dom/crypto/WebCryptoTask.cpp
>+  RsaesPkcs1Task(JSContext* aCx, const ObjectOrString& aAlgorithm,
>+    , mData(aData)

OOM?

>+      if (mData.Length() > mStrength-11) {

Where is the magic 11 coming from?  At the very least, document.

>+    } else {
>+      mPrivKey = aKey.GetPrivateKey();

You did that already, no?

>+  virtual nsresult DoCrypto() MOZ_OVERRIDE
>+    mResult.SetLength(mStrength);

OOM?

I'm going to assume that this is a sufficient length... but I have no idea how tell that from looking at this code.

>+    if (mEncrypt) {
>+      rv = MapSECStatus(PK11_PubEncryptPKCS1(
>+              mPubKey.get(), mResult.Elements(),
>+              mData.Elements(), mData.Length(),
>+              nullptr));

So the encrypted data is the same length no matter what data we passed in?  Seems a bit weird...

>+class RsassaPkcs1Task : public WebCryptoTask
>+    , mSignature(aSignature)
>+    , mData(aData)

OOM for both of those?

>+    nsString hash;

This is written to but never read.  Why is it needed?

>+    if ((mSign && !mPrivKey)||

Missing space before "||".

>+  virtual nsresult DoCrypto() MOZ_OVERRIDE
>+      mSignature.Assign(signature.get());

OOM?

>+      ScopedSECItem signature(mSignature.ToSECItem());

OOM?

>+  virtual void Resolve() MOZ_OVERRIDE
>+      AutoJSContext cx;
>+      JS::RootedValue ret(cx, JS::BooleanValue(mVerified));
>+      mResultPromise->MaybeResolve(cx, ret);

  mResultPromise->MaybeResolve(mVerified);

r=me with those issues fixed

Comment 6

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998803.2.patch r=bz,dkeeler

Updated patch addressing review comments.

Happy try run covering latest patches for 998802, 998803, 998471 here:
https://tbpl.mozilla.org/?tree=Try&rev=e607e1f42a20

Comment 7

[Carsten Book [:Tomcat]]

In the try run you mentioned is a bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=40295974&tree=Try - not sure which one of the 3 cause this but i guess this has to be fixed first before we can do the checkin.

Comment 8

[Richard Barnes [:rbarnes]]

Missed an #include, and it was masked by UNIFIED_SOURCES.  Looking better now:
https://tbpl.mozilla.org/?tree=Try&rev=de75a4759611

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce017a1024b2

Comment 10

[Boris Zbarsky [:bzbarsky]]

>+      // (as required by PKCS#1 / RFC 3447)

As long as you're citing, why not cite the relevant section?

There is still no OOM handling for the mSignature set in RsassaPkcs1Task:::DoCrypto.  If it's not needed, please document why.

There is still no OOM handling for mSignature.ToSECItem() in the same function (in the other if branch).

Comment 11

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/ce017a1024b2

Comment 12

[Richard Barnes [:rbarnes]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> >+      // (as required by PKCS#1 / RFC 3447)
> 
> As long as you're citing, why not cite the relevant section?
> 
> There is still no OOM handling for the mSignature set in
> RsassaPkcs1Task:::DoCrypto.  If it's not needed, please document why.
> 
> There is still no OOM handling for mSignature.ToSECItem() in the same
> function (in the other if branch).

Fixed in Bug 1018467.