998802 - Add support for symmetric-key encryption and MAC to WebCrypto API

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998802.patch

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8409963 [details] [diff] [review]
webcrypto-998802.patch

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

::: dom/crypto/WebCryptoTask.cpp
@@ +152,5 @@
> +      if (params.mAdditionalData.WasPassed()) {
> +        mAad = params.mAdditionalData.Value();
> +      }
> +
> +      // 32, 64, 96, 104, 112, 120 or 128

what happens for 192 for example?

@@ +192,5 @@
> +        param = mIv.AsSECItem();
> +        break;
> +      case CKM_AES_CTR:
> +        ctrParams.ulCounterBits = mCounterLength;
> +        MOZ_ASSERT(mIv.Length() == 16);

make this magic value a const and use the named value

@@ +220,5 @@
> +    }
> +
> +    // Initialize the input and output buffers (enough space for a full tag)
> +    uint32_t dataLen = mData.Length();
> +    uint32_t maxLen = dataLen + 64;

same here magic value, plase make a named const

Comment 3

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998802.1.patch

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

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8426480 [details] [diff] [review]
webcrypto-998802.1.patch

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

Awesome. Just some minor nits. r=me for NSS usages.

::: dom/crypto/WebCryptoTask.cpp
@@ +124,5 @@
> +    }
> +
> +    // Check that we got a reasonable key
> +    if ((mSymKey.Length() != 16)&&
> +        (mSymKey.Length() != 24)&&

nit: spaces around operators (so have a space before "&&")

@@ +179,5 @@
> +      mTagLength = 128;
> +      if (params.mTagLength.WasPassed()) {
> +        mTagLength = params.mTagLength.Value();
> +        if (!((mTagLength == 32) || (mTagLength == 64) ||
> +             ((mTagLength >= 96) && (mTagLength % 8 == 0)))) {

Is 128 the maximum? Should we check it's not bigger than that?

@@ +218,5 @@
> +      case CKM_AES_CTR:
> +        ctrParams.ulCounterBits = mCounterLength;
> +        MOZ_ASSERT(mIv.Length() == 16);
> +        memcpy(&ctrParams.cb, mIv.Elements(), 16);
> +        param = {siBuffer, (unsigned char*) &ctrParams, sizeof(ctrParams)};

I think some compilers (e.g. the one used on b2g) won't like this.
Also, I think the recommended style is to have a space after/before {/}

@@ +226,5 @@
> +        gcmParams.ulIvLen = mIv.Length();
> +        gcmParams.pAAD = mAad.Elements();
> +        gcmParams.ulAADLen = mAad.Length();
> +        gcmParams.ulTagBits = mTagLength;
> +        param = {siBuffer, (unsigned char*) &gcmParams, sizeof(gcmParams)};

same as above

@@ +258,5 @@
> +      rv = MapSECStatus(PK11_Decrypt(symKey.get(), mMechanism, &param,
> +                                     mResult.Elements(), &outLen, maxLen,
> +                                     mData.Elements(), mData.Length()));
> +    }
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_OPERATION_ERR);

Again, go with what bz says, but I believe NS_ENSURE_SUCCESS is deprecated.

@@ +311,5 @@
> +      return NS_ERROR_DOM_INVALID_ACCESS_ERR;
> +    }
> +
> +    // Compute the MAC
> +    SECItem param = { siBuffer, nullptr, 0 };

(note that this initialization will most likely be accepted by all compilers we use because it's completely static)

@@ +772,5 @@
> +
> +  if (algName.EqualsLiteral(WEBCRYPTO_ALG_AES_CBC) ||
> +      algName.EqualsLiteral(WEBCRYPTO_ALG_AES_CTR) ||
> +      algName.EqualsLiteral(WEBCRYPTO_ALG_AES_GCM))
> +  {

nit: { on previous line

@@ +774,5 @@
> +      algName.EqualsLiteral(WEBCRYPTO_ALG_AES_CTR) ||
> +      algName.EqualsLiteral(WEBCRYPTO_ALG_AES_GCM))
> +  {
> +    return new AesTask(aCx, aAlgorithm, aKey, aData, aEncrypt);
> +  } else {

nit: no else after return

@@ +802,5 @@
> +
> +
> +  if (algName.EqualsLiteral(WEBCRYPTO_ALG_HMAC)) {
> +    return new HmacTask(aCx, aAlgorithm, aKey, aSignature, aData, aSign);
> +  } else {

nit: no else after return

::: dom/crypto/test/tests.js
@@ +332,5 @@
>      );
>    }
>  );
> +
> +// -----------------------------------------------------------------------------

For some modes, decryption will fail if the ciphertext isn't a multiple of the block size, right? We should have a test for that. (And maybe for an empty ciphertext, and any other invalid inputs you can think of.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8426480 [details] [diff] [review]
webcrypto-998802.1.patch

>+++ b/dom/crypto/WebCryptoTask.cpp
>+  AesTask(JSContext* aCx, const ObjectOrString& aAlgorithm,
>+             mozilla::dom::Key& aKey, const CryptoOperationData& aData,
>+             bool aEncrypt)

Fix the indent, please.

>+    , mData(aData)

Do we need to do something if that fails to copy the data due to OOM?

>+      mIv = params.mIv.Value();

And here.

>+      mIv = params.mCounter.Value();
>+      if (mIv.Length() != 16) {

Yeah, like that.

>+      mIv = params.mIv.Value();

And here.

>+        mAad = params.mAdditionalData.Value();

This could really use a better member name than "mAad".  I assume mIv is named that for some reason, but it's not, a better name there would be good too.

>+        if (!((mTagLength == 32) || (mTagLength == 64) ||
>+             ((mTagLength >= 96) && (mTagLength % 8 == 0)))) {

This seems overparenthesized on the compares.

Is mTagLength == 136 valid?  Doesn't seem to be per spec; do we need to check mTagLength <= 128 in addition to >=96?

>+  virtual nsresult DoCrypto() MOZ_OVERRIDE
>+        cbcParam = mIv.ToSECItem();

That can return null on OOM, right?  Need to handle that.

>+    ScopedSECItem keyItem(mSymKey.ToSECItem());

Again, can return null, right?

>+    mResult.SetLength(maxLen);

And if that fails, we'll write out of bounds, no?  Really need to watch out for that OOM stuff.

>+class HmacTask : public WebCryptoTask
>+  HmacTask(JSContext* aCx, const ObjectOrString& aAlgorithm,
>+    , mSymKey(aKey.GetSymKey())
>+    , mData(aData)
>+    , mSignature(aSignature)

You're handling OOM for mSymKey, but not for mData or mSignature, afaict.

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

And if that fails, we need to deal.

>+    ScopedSECItem keyItem(mSymKey.ToSECItem());

Can return null on OOM.

>+  virtual void Resolve() MOZ_OVERRIDE
>+      int cmp = NSS_SecureMemcmp(mSignature.Elements(),
>+                                 mResult.Elements(),
>+                                 mSignature.Length());

Are we guaranteed that mSignature.Length() <= mResult.Length()?  What guarantees that?

>+      AutoJSContext cx;
>+      JS::RootedValue ret(cx, JS::BooleanValue(equal));
>+      mResultPromise->MaybeResolve(cx, ret);

  mResultPromise->MaybeResolve(equal);

should work fine.

r=me with those issues addressed.

Comment 6

[Richard Barnes [:rbarnes]]

Forthcoming patch should address the OOM issues.  One comment below.

(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8426480 [details] [diff] [review]
> webcrypto-998802.1.patch
> 
> >+        mAad = params.mAdditionalData.Value();
> 
> This could really use a better member name than "mAad".  I assume mIv is
> named that for some reason, but it's not, a better name there would be good
> too.

In the crypto parlance, "AAD" stands for "Additional Authenticated Data" and "IV" for "Initialization Vector".  These acronyms are widespread, and the expansions make for cumbersome variable names, so I went for the acronyms.  (For comparison, the W3C API uses "iv" and "additionalData".)  I've added comments after the definition of these parameters to clarify.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I've added comments after the definition of these parameters to clarify.

That works, thanks.

Comment 8

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998802.2.patch r=bz,keeler

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 9

[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 10

[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 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7bf0779446

Comment 12

[Boris Zbarsky [:bzbarsky]]

>+      equal = equal && (cmp == 0);

If !equal before this line, then the SecureMemcmp read bogus memory.  That's a bad idea in general (e.g. can crash the process)...  If the lengths really can be different, this code needs to skip the memcmp or do it to a length that's the min of the two lengths or something when they don't match, no?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c7bf0779446

Comment 14

[Richard Barnes [:rbarnes]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> >+      equal = equal && (cmp == 0);
> 
> If !equal before this line, then the SecureMemcmp read bogus memory.  That's
> a bad idea in general (e.g. can crash the process)...  If the lengths really
> can be different, this code needs to skip the memcmp or do it to a length
> that's the min of the two lengths or something when they don't match, no?

Fixed in Bug 1018467.