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

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

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 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.

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.

> I've added comments after the definition of these parameters to clarify. That works, thanks.

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

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.

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

>+ 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?

(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.