This patch does a lot of stuff. /me reminds self to break into smaller patches next time. The good news is that it's a net reduction in code of 673 lines! # IDL Compliance -- Updated WebIDL to match latest spec -- Removed old interfaces from SubtleCrypto.idl and Bindings.conf -- KeyAlgorithm, CryptoKeyPair, etc. -- Added new KeyAlgorithm dictionaries in KeyAlgorithm.idl -- Removed KeyAlgorithm interface implementation files: -- dom/crypto/AesKeyAlgorithm.cpp -- dom/crypto/AesKeyAlgorithm.h -- dom/crypto/BasicSymmetricKeyAlgorithm.h -- dom/crypto/CryptoKeyPair.cpp -- dom/crypto/CryptoKeyPair.h -- dom/crypto/EcKeyAlgorithm.cpp -- dom/crypto/EcKeyAlgorithm.h -- dom/crypto/HmacKeyAlgorithm.cpp -- dom/crypto/HmacKeyAlgorithm.h -- dom/crypto/KeyAlgorithm.cpp -- dom/crypto/KeyAlgorithm.h -- dom/crypto/RsaHashedKeyAlgorithm.cpp -- dom/crypto/RsaHashedKeyAlgorithm.h -- /dom/crypto/RsaKeyAlgorithm.cpp -- /dom/crypto/RsaKeyAlgorithm.h -- Added method CryptoBuffer::ToUint8Array (wrapping Uint8Array::Create) -- Added class KeyAlgorithmProxy to provide functionality beyond the dictionaries -- Structured clone -- Translation to JWK "alg" tokens and NSS mechanisms -- Simplified constructors -- Converted CryptoKey::mAlgorithm nsRefPtr<KeyAlgorithm> to KeyAlgorithmProxy # Processing compliance -- Removed support for RSAES-PKCS#1-v1_5 -- Moved algorithm name normalization to WebCryptoCommon.h (from WebCryptoTask.cpp) -- Each WebCrypto method now performs the specified checks -- There are a few divergences noted in comments; there are spec bugs for these -- Added several minor compliance checks -- JWKs no longer accepted for import if format is not "jwk" -- Import fails if no relevant key usages are provided -- Minor updates to tests -- Bring them in compliance with the new constraints -- Remove debugging code

Comment on attachment 8483145 [details] [diff] [review] bug-1037892.patch Review of attachment 8483145 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a great cleanup to me. And I really appreciate all the new checks you put in. My r+ doesn't mean too much here but I'll give it anyway :) ::: dom/crypto/WebCryptoTask.cpp @@ +2722,5 @@ > + > + // Verify that aKeyUsages does not contain an unrecognized value > + for (uint32_t i = 0; i < aKeyUsages.Length(); ++i) { > + if (!CryptoKey::RecognizedUsage(aKeyUsages[i])) { > + return new FailureTask(NS_ERROR_DOM_SYNTAX_ERR); Should we move this code to a helper function? Or make it a method on CryptoKey()? ::: dom/crypto/test/test_WebCrypto.html @@ +466,1 @@ > console.log(x); Removing all the other log() calls you added one here? @@ +1338,5 @@ > }; > var unwrapKey; > > function doWrap(keys) { > + console.log("entered doWrap"); Do we need those? @@ +1345,5 @@ > unwrapKey = keys[2]; > return crypto.subtle.wrapKey("raw", originalKey, wrapKey, oaep); > } > function doUnwrap(wrappedKey) { > + console.log("entered doUnwrap"); This? @@ +1350,5 @@ > return crypto.subtle.unwrapKey("raw", wrappedKey, unwrapKey, oaep, > gcm, false, ['encrypt']); > } > function doEncrypt(aesKey) { > + console.log("entered doEncrypt"); And this? @@ +1644,5 @@ > function () { > var that = this; > > function doWrap(keys) { > + console.log("entered doWrap"); Aaaand this?

Comment on attachment 8483145 [details] [diff] [review] bug-1037892.patch Sorry for the lag here; big patches that involve spec reading are hard to schedule in. :( Followup reviews of interdiffs will be faster. >+++ b/dom/crypto/CryptoKey.cpp >+CryptoKey::GetAlgorithm(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal) const >+ case KeyAlgorithmProxy::UNKNOWN: How can you end up with this type? KeyAlgorithmProxy doesn't have a constructor (should it?) and never sets mType to UNKNOWN as far as I can tell. >+ case KeyAlgorithmProxy::PLAIN: How can you end up with _this_ type? Again, I don't think you can. >+ ToJSValue(cx, mAlgorithm.mPlain, &val); You need to handle failures here and in the other ToJSValue callsites. ToJSValue has a return value for a reason! This getter should be marked as [Throws] in the IDL, and you should Throw() on the ErrorResult if ToJSValue returns false. >+++ b/dom/crypto/CryptoKey.h >+ static bool RecognizedUsage(const nsString& aUsage); Maybe IsRecognizedUsage? >+++ b/dom/crypto/KeyAlgorithmProxy.cpp >+KeyAlgorithmProxy::WriteStructuredClone(JSStructuredCloneWriter* aWriter) const >+ CryptoBuffer publicExponent; >+ publicExponent.Assign(mRsa.mPublicExponent.Value()); Why do you need to make this copy? I don't think you do; just write the thing you already have! If you _do_ have to copy, you have to check it for failure. >+KeyAlgorithmProxy::ReadStructuredClone(JSStructuredCloneReader* aReader) >+ !JS_ReadUint32Pair(aReader, (uint32_t*) &type, &zero)) { Nothing says KeyAlgorithmType is a 32-bit integer type, so this cast is not ok. Please read into an actual uint32_t. >+ CryptoBuffer publicExponent; Again, not sure why you can't just read directly into the right member. If you do have to copy like this, you need to check for failure. >+KeyAlgorithmProxy::GetMechanism(const HmacKeyAlgorithm& aAlgorithm) >+ if (aAlgorithm.mName.Value().EqualsLiteral(WEBCRYPTO_ALG_HMAC)) { Under what circumstances would this condition test false? Worth documenting, if such conditions actually exist. >+++ b/dom/crypto/KeyAlgorithmProxy.h >+struct RsaHashedKeyAlgorithmStorage { >+ Optional<nsString> mName; >+ KeyAlgorithm mHash; >+ Optional<uint16_t> mModulusLength; >+ Optional<CryptoBuffer> mPublicExponent; Why are those members Optional? They always get set in practice, no? >+struct KeyAlgorithmProxy >+ enum KeyAlgorithmType { >+ UNKNOWN, >+ PLAIN, Again, these two enum values seem to be unused. >+ // Plain is always populated with the algorithm name Why? Why is mPlain needed? Just so you can get the name easily without switching on mType? In that case, just storing the name would perhaps be simpler... but see below. >+ KeyAlgorithm mPlain; >+ AesKeyAlgorithm mAes; >+ HmacKeyAlgorithm mHmac; >+ RsaHashedKeyAlgorithmStorage mRsa; >+ EcKeyAlgorithm mEc; These are mutually exclusive, right? Could this be a union? In particular all of these except RsaHashedKeyAlgorithmStorage inherit from KeyAlgorithm, and you can make RsaHashedKeyAlgorithmStorage inherit from KeyAlgorithm as well and remove it mName member. Then you can do: union { KeyAlgorithm mPlain; AesKeyAlgorithm mAes; HmacKeyAlgorithm mHmac; RsaHashedKeyAlgorithmStorage mRsa; EcKeyAlgorithm mEc; }; and then just set up your actual algorithm type struct. Since they all inherit from KeyAlgorithm, they will have a binary layour that matches each other and mPlain for the part that has mName, so you can still use mPlain.mName as you do now, but without having to duplicate it in the other structs. I can understand not wanting to play games with unions if you don't want to, but in that case please just store the name as an nsString instead of obfuscating things with a KeyAlgorithm mPlain. >+ mPlain.mName.Construct(aName); >+ mAes.mName.Construct(aName); >+ mAes.mLength.Construct(aLength); So... all of these algorithm structs always have all their members set. Are there situations in which it's legit to have the members missing when these dictionaries are used as arguments? If not, the IDL should have the members marked as required, and then a bunch of complexity involving checking of WasPassed() and calls to .Construct() and .Value() can go away. This will obviously need a spec issue too, so I'm probably OK with it happening as a followup if it has to... but checking in all these curlicues just to remove them later seems a bit annoying. >+++ b/dom/crypto/WebCryptoCommon.h > inline bool >+NormalizeAlgorithmName(const nsString& aName, nsString& aDest) Does this really make sense to inline? Why is this returning bool? No one ever looks at the return value. >+ if (aName.EqualsIgnoreCase(WEBCRYPTO_ALG_AES_CBC)) { I assume you verified that EqualsIgnoreCase only ignores ASCII case instead of doing Unicode case stuff? And that this will never change? It might be better to do this with nsContentUtils::EqualsIgnoreASCIICase, which does what it says on the tin. Yes, I know you just moved the code... >+EqualsNameNormalized(const nsString& aName, const nsString& aReference) Just use nsContentUtils::EqualsIgnoreASCIICase, please. It does what you want. >+++ b/dom/crypto/WebCryptoTask.cpp >+ mKeyPair.mPrivateKey.Construct(); >+ mKeyPair.mPublicKey.Construct(); Are there valid uses of a keypair with one of the keys missing? If not, should they both be required in the IDL? Then you can drop these Construct() calls and all the Value() silliness below. >+ mKeyPair.mPublicKey.Value().get()->Algorithm().MakeRsa(algName, How about we add an operator-> to OwningNonNull? Having to do this .get()-> thing is silly. Even better would be if we could just have the "." operator work, but that's one thing C++ won't let you override. :( Followup is OK for this part if you prefer, though again it seems a bit silly to then go through and fix up all the noise. >+// * Each method handles it's slice of the supportedAlgorithms structure s/it's/its/ >+WebCryptoTask::CreateImportKeyTask(JSContext* aCx, >+ // SPEC-BUG: PBKDF2 is not supposed to be supported for this operation. >+ // However, the spec should be updated to allow it. Spec issue filed? > WebCryptoTask::CreateExportKeyTask(const nsAString& aFormat, >+ // SPEC-BUG: PBKDF2 is not supposed to be supported for this operation. >+ // However, the spec should be updated to allow it. Again, spec issue filed? > WebCryptoTask::CreateGenerateKeyTask(JSContext* aCx, >+ // SPEC-BUG: Spec says that this should be InvalidAccessError, but that >+ // is inconsistent with other analogous points in the spec And here? This one is different from the other two: there you seem to be implementing the (claimed buggy) spec, but here you're violating the spec, right? I see a bunch of behavior changes here in terms of validating input. Do the modifications to the tests exercise those changes? r=me with the above issues fixed.

Carrying forward r+ from ttaubert and bz. Interdiff to follow shortly Tim's comments I just implemented. Responses to bz are below. His may be the longest r+ I've ever gotten :) (In reply to Boris Zbarsky [:bz] from comment #4) > Comment on attachment 8483145 [details] [diff] [review] > bug-1037892.patch > > Sorry for the lag here; big patches that involve spec reading are hard to > schedule in. :( Followup reviews of interdiffs will be faster. > > >+++ b/dom/crypto/CryptoKey.cpp > >+CryptoKey::GetAlgorithm(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal) const > >+ case KeyAlgorithmProxy::UNKNOWN: > > How can you end up with this type? You can't... > KeyAlgorithmProxy doesn't have a constructor (should it?) and never sets > mType to UNKNOWN as far as I can tell. > > >+ case KeyAlgorithmProxy::PLAIN: > > How can you end up with _this_ type? Again, I don't think you can. ... or this one either. I just removed these types, since they weren't really adding any value. > >+KeyAlgorithmProxy::GetMechanism(const HmacKeyAlgorithm& aAlgorithm) > >+ if (aAlgorithm.mName.Value().EqualsLiteral(WEBCRYPTO_ALG_HMAC)) { > > Under what circumstances would this condition test false? Worth > documenting, if such conditions actually exist. I think this check is actually worth keeping, just as a safeguard against cases where an HmacKeyAlgorithm was initialized with a value that was not actually an HMAC key algorithm. I've changed it to an assertion, though, and added a comment. > >+++ b/dom/crypto/KeyAlgorithmProxy.h > >+struct RsaHashedKeyAlgorithmStorage { > >+ Optional<nsString> mName; > >+ KeyAlgorithm mHash; > >+ Optional<uint16_t> mModulusLength; > >+ Optional<CryptoBuffer> mPublicExponent; > > Why are those members Optional? They always get set in practice, no? Good point. Copypasta from RsaHashedKeyAlgorithm without thinking much about it. > union { > KeyAlgorithm mPlain; > AesKeyAlgorithm mAes; > HmacKeyAlgorithm mHmac; > RsaHashedKeyAlgorithmStorage mRsa; > EcKeyAlgorithm mEc; > }; Making a union was actually my first approach to this. I quickly learned a lot about unions in C++ :) I don't recall all the difficulties, but IIRC it had something to do with non-trivial constructors / destructors. If you think it's a big deal, I can take another look, but it seems simpler to me as it is now, and not too horrendously inefficient. > >+ mPlain.mName.Construct(aName); > >+ mAes.mName.Construct(aName); > >+ mAes.mLength.Construct(aLength); > > So... all of these algorithm structs always have all their members set. > > Are there situations in which it's legit to have the members missing when > these dictionaries are used as arguments? If not, the IDL should have the > members marked as required, and then a bunch of complexity involving > checking of WasPassed() and calls to .Construct() and .Value() can go away. > This will obviously need a spec issue too, so I'm probably OK with it > happening as a followup if it has to... but checking in all these curlicues > just to remove them later seems a bit annoying. These dictionaries are never passed as arguments. They're constructed by the browser and assigned as attributes of CryptoKey objects. If the "required" stuff has landed, I'm happy to use it. I was not enjoying the WasPassed() / Construct() / Value() stuff. I've added the required "required" tokens to the WebIDL, both for KeyAlgorithms in KeyAlgorithm.webidl and for other dictionaries in SubtleCrypto.webidl, and removed all of the unnecessary the Optional<> method calls. > >+++ b/dom/crypto/WebCryptoCommon.h > > inline bool > >+NormalizeAlgorithmName(const nsString& aName, nsString& aDest) > > Does this really make sense to inline? > > Why is this returning bool? No one ever looks at the return value. I updated GetAlgorithmName to check the return value and return an error if the name was unknown. > >+ if (aName.EqualsIgnoreCase(WEBCRYPTO_ALG_AES_CBC)) { > > I assume you verified that EqualsIgnoreCase only ignores ASCII case instead > of doing Unicode case stuff? And that this will never change? > > It might be better to do this with nsContentUtils::EqualsIgnoreASCIICase, > which does what it says on the tin. > > Yes, I know you just moved the code... Will do, but good lord, I hope we never have algorithm names with non-ASCII characters. Actually, while I was at it, I just merged the normalization stuff into one NormalizeToken method to call when you need to normalize some string that WebCrypto says should be case-insensitive. > >+++ b/dom/crypto/WebCryptoTask.cpp > >+ mKeyPair.mPrivateKey.Construct(); > >+ mKeyPair.mPublicKey.Construct(); > > Are there valid uses of a keypair with one of the keys missing? If not, > should they both be required in the IDL? Then you can drop these > Construct() calls and all the Value() silliness below. I don't think so. There's only two of them; if one is missing, just hold on to the other :) I've added "required" in our WebIDL. > >+ mKeyPair.mPublicKey.Value().get()->Algorithm().MakeRsa(algName, > > How about we add an operator-> to OwningNonNull? Having to do this .get()-> > thing is silly. Even better would be if we could just have the "." operator > work, but that's one thing C++ won't let you override. :( > > Followup is OK for this part if you prefer, though again it seems a bit > silly to then go through and fix up all the noise. SGTM, but let's punt to a follow-up. > > WebCryptoTask::CreateGenerateKeyTask(JSContext* aCx, > >+ // SPEC-BUG: Spec says that this should be InvalidAccessError, but that > >+ // is inconsistent with other analogous points in the spec > > And here? > > This one is different from the other two: there you seem to be implementing > the (claimed buggy) spec, but here you're violating the spec, right? No, in all three cases, we're presuming that the spec bugs have been resolved in the way we suggest. We allow PBKDF2 for import/export, even though it's forbidden by the spec. We return SyntaxError, even though the spec calls for InvalidAccessError. Here are the relevant spec bugs: Import/Export for PBKDF2: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26011 SyntaxError for unrecognized usage: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26413 Add "required", drop nullable: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26674 > I see a bunch of behavior changes here in terms of validating input. Do the > modifications to the tests exercise those changes? I don't think these are behavior changes so much as refactoring and bug fixes. We already have tests for most of these things; most of the modifications were to bring the tests into compliance with the more complete tests. > r=me with the above issues fixed. Thanks for the thorough review, as usual.

Comment on attachment 8495331 [details] bug-1037892.0-1.interdiff >+CryptoKey::GetAlgorithm(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal, > aRetVal.set(&val.toObject()); You don't want to do that in the !converted case, because in that case "val" is not actually isObject(). Please fix this! > KeyAlgorithmProxy::ReadStructuredClone(JSStructuredCloneReader* aReader) >- !JS_ReadUint32Pair(aReader, (uint32_t*) &type, &zero)) { You don't need the cast here anymore. > but IIRC it had something to do with non-trivial constructors / destructors Oh, good point. Yeah, you don't want the union thing here, then. > I hope we never have algorithm names with non-ASCII characters. If someone asks you to do case-insensitive matching of non-ASCII for a web API, tell them that's not a good idea, since there's like 10 different ways to do it and you'd have to pick one kinda randomly... Thank you for posting the interdiff!

(In reply to Boris Zbarsky [:bz] from comment #7) > If someone asks you to do case-insensitive matching of non-ASCII for a web > API, tell them that's not a good idea, since there's like 10 different ways > to do it and you'd have to pick one kinda randomly... HTML5 actually says to do a 'compatibility caseless' match for the name attribute of input elements of a radio button group [1]. As far as I know we don't actually do that, though. Irrelevant for this bug, but I was reminded of this odd wrinkle by your comment :) [1] http://www.w3.org/html/wg/drafts/html/master/forms.html#radio-button-group

(In reply to Boris Zbarsky [:bz] from comment #7) > Comment on attachment 8495331 [details] > bug-1037892.0-1.interdiff > > >+CryptoKey::GetAlgorithm(JSContext* cx, JS::MutableHandle<JSObject*> aRetVal, > > aRetVal.set(&val.toObject()); > > You don't want to do that in the !converted case, because in that case "val" > is not actually isObject(). Please fix this! Added "return;" for the !converted case. > > KeyAlgorithmProxy::ReadStructuredClone(JSStructuredCloneReader* aReader) > >- !JS_ReadUint32Pair(aReader, (uint32_t*) &type, &zero)) { > > You don't need the cast here anymore. Done. Two more things where I would like your concurrence: 1. I've removed CryptoKeyPair from test_interfaces.html, which I think might possibly mean I should think about getting review from a DOM peer. A table laying out the differences is below. 2. I should have realized before that this changes the format of KeyAlgorithm structures are serialized with structured clone. This breaks backward compatibility if people have stored keys. Two options: (a) Revert to the previous format. This would require changing the order in which fields are read/written, and result in some strange values for the KeyAlgorithmProxy::KeyAlgorithmType enum (1, 2, 3, 5). (b) Stick with the new format. This would break existing stored keys, and might require that we add some check to fail cleanly on the old format. Overall, I'm inclined to go with (b). The new format is cleaner, and the number of existing keys is negligible (telemetry shows single-digit numbers of reports). The backward-compatibility check is pretty straightforward -- we just need to check for a null in the first character of the name. I've attached patches for both solutions; the patch for (b) has more explanation on why that check works. Type Before After ------------------------------------------------------- AES KEYTAG(1) | 0 name(str) length | 0 type(0) | 0 name(str) length | 0 ------------------------------------------------------- HMAC KEYTAG(3) | 0 name(str) length | 0 type(1) | 0 hash(str) length | 0 name(str) hash(str) ------------------------------------------------------- EC KEYTAG(2) | 0 name(str) namedCurve(str) type(2) | 0 name(str) namedCurve(str) ------------------------------------------------------- RsaHashed KEYTAG(5) | 0 name(str) modulusLength | 0 type(3) | 0 publicExponent(buf) modulusLength | 0 hash(str) publicExponent(buf) name(str) hash(str) -------------------------------------------------------

Comment on attachment 8496007 [details] [diff] [review] bug-1037892.2.patch > I've removed CryptoKeyPair from test_interfaces.html Yep, r=me on that part. > This breaks backward compatibility if people have stored keys This applies only to people who were experimenting with webcrypto, right? And as you note there are not many of those. What is the failure mode they will observe with approach (a)? Will they be able to easily get themselves back to a working state? Can we at least log a console error to tell them what happened? I assume that the name can't be empty in the new format, which is why the first char is never null? Also, how come we're storing persistent data without some sort of version/schema marker so we have to resort to sniffing for coincidental stuff? :( Can we at least move towards fixing that?

(In reply to Boris Zbarsky [:bz] from comment #13) > Comment on attachment 8496007 [details] [diff] [review] > bug-1037892.2.patch > > > I've removed CryptoKeyPair from test_interfaces.html > > Yep, r=me on that part. > > > This breaks backward compatibility if people have stored keys > > This applies only to people who were experimenting with webcrypto, right? > And as you note there are not many of those. That's correct. They will have had to either be using a pre-release version, or have turned on dom.webcrypto.enabled. > What is the failure mode they will observe with approach (a)? Will they be > able to easily get themselves back to a working state? Can we at least log > a console error to tell them what happened? With (a), there is no failure. With (b), the structured clone read operation will report DataCloneError. As far as ease of getting back working: There is no way to recover the key, so it will be as if the object had been deleted. Since similar conditions can happen anyway (e.g., by a user clearing cookies), this should be a situation that applications are prepared to deal with. > I assume that the name can't be empty in the new format, which is why the > first char is never null? > > Also, how come we're storing persistent data without some sort of > version/schema marker so we have to resort to sniffing for coincidental > stuff? :( Can we at least move towards fixing that? That seems like a good reason for obsoleting the format (i.e., option (b)). And it actually simplifies the backward compatibility checking -- both the CryptoKey and KeyAlgorithm serializations have a uint32_t field that is always zero, since they needed to write a single number using JS_WriteUint32Pair(). So we can just declare that to be the version field, and the old format to be version zero. So now that we've got a version field we can switch on, the question is whether we support the old format. I'm still inclined not to, because (1) it adds cruft to the code base, (2) there's not much benefit, and (3) the errors it would cause should be recoverable. This patch implements the version check with no backward compatibility.

Comment on attachment 8496144 [details] [diff] [review] bug-1037892-format-check.patch > Since similar conditions can happen anyway Ah, great. Sounds fine to me, then.

This patch folds in the SC format patch, and just landed as: https://hg.mozilla.org/integration/mozilla-inbound/rev/398bdeea30b0

Backed out for build errors in https://hg.mozilla.org/integration/mozilla-inbound/rev/d1268000b5d0 https://tbpl.mozilla.org/php/getParsedLog.php?id=49004465&tree=Mozilla-Inbound

There was also this failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=49004464&tree=Mozilla-Inbound

Minor update addressing test failures. Re-try of failing tests is green: https://tbpl.mozilla.org/?tree=Try&rev=9c62d3a99f63 Re-landing: https://hg.mozilla.org/integration/mozilla-inbound/rev/62e3d5d264c2

Comment on attachment 8496453 [details] [diff] [review] bug-1037892.4.patch Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: This patch removes the RSAES-PKCS1-v1_5 algorithm from WebCrypto, which can expose a secret WebCrypto key if used improperly. The longer this algorithm is available in Firefox, the more likely it is that it will be used. More generally, this patch brings our WebCrypto implementation into compliance with the requirements of the WebCrypto spec, so if declined, web developers will have to continue to deal with incompatibilities. [Describe test coverage new/current, TBPL]: Existing WebCrypto tests were updated to bring them into compliance with new checks required by the spec. [Risks and why]: Low risk. Largely removing unneeded WebIDL implementations and RSAES-PKCS1-v1_5 code. [String/UUID change made/needed]: none

Comment on attachment 8496453 [details] [diff] [review] bug-1037892.4.patch I spoke with Richard about this change. As he noted, it is highly preferable to remove RSAES-PKCS1-v1_5 and bring the implementation into compliance with the spec earlier. Although this is a large patch, the changes are confined to WebCrypto and should have no impact on the rest of the product or the Web at large. As well, a good portion of the patch is the removal of RSAES-PKCS1-v1_5. Aurora+

Needs rebasing for Aurora uplift.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24) > Needs rebasing for Aurora uplift. Also depends on Bug 1070764 and Bug 1057541. (As I noticed when I tried to rebase.) I will post rebased patches for this bug and those soon. Should I set the uplift flags on the dependencies as well, or can they ride along with this?

Actually, good news: Bug 1070764 and Bug 1057541 both apply cleanly to mozilla-aurora. So no revised patches needed. This patch is rebased on top of mozilla-aurora, Bug 1064320, Bug 1057541, and Bug 1070764 (in that order). In other words, my mq patch queue now looks as follows: bug-1064320.patch required-dict-parser.patch // 1057541 dict-required-arg-default.patch // 1057541 required-dict-codegen.patch // 1057541 fix-required-dict-member.patch // 1070764 bug-1037892.aurora.patch The only modification needed was to make the auto-merge process deal with the changes to RsaesPkcs1Task in Bug 1064320.

(In reply to Richard Barnes [:rbarnes] from comment #25) > Should I set the uplift flags on the dependencies as well? Yes, please.

Done. https://bugzilla.mozilla.org/show_bug.cgi?id=1057541#c8 https://bugzilla.mozilla.org/show_bug.cgi?id=1070764#c4