If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

WebCrypto algorithm names should be normalized as they are defined in the spec

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bz, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

unspecified
mozilla34
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=26311
Blocks: 865789
(Assignee)

Comment 1

3 years ago
Created attachment 8456502 [details] [diff] [review]
0001-Bug-1037501-WebCrypto-algorithm-names-should-be-norm.patch
Assignee: rlb → ttaubert
Status: NEW → ASSIGNED
Attachment #8456502 - Flags: review?(rlb)
Note that you probably need to merge with the patches of Richard's I was reviewing today, since they add more EqualsLiteral/EqualsAscii bits.
Comment on attachment 8456502 [details] [diff] [review]
0001-Bug-1037501-WebCrypto-algorithm-names-should-be-norm.patch

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

I think this patch goes about things a little backwards.  

For the normalization, rather than using LowerCaseEqualsLiteral all the time, I would think that you would normalize the input to lower case (ToLowerCase in appropriate places), then just use EqualsLiteral.

For the tests, there's no need to convert all the old tests to lower case, only to add a test with a lower-case or mixed-case identifier.
Attachment #8456502 - Flags: review?(rlb) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8462603 [details] [diff] [review]
0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v2

Do we need a separate test for this given that I had to convert some of the checks to use .toLowerCase()?
Attachment #8456502 - Attachment is obsolete: true
Attachment #8462603 - Flags: review?(rlb)
You at least want tests that check that uppercase, lowercase, and mixed-case input are all accepted, right?
(Assignee)

Comment 6

3 years ago
Yeah, lowercase and mixed-case input are missing. Should add that indeed.
(Assignee)

Comment 7

3 years ago
Created attachment 8462617 [details] [diff] [review]
0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v3

Added some tests for lowercase and mixed-case algorithm names. In theory we had to check all the places (every crypto task) that take algorithm names but that's a little excessive. We're fine if we use GetAlgorithmName() everywhere - which this patch also corrects for HMAC's hash param.
Attachment #8462603 - Attachment is obsolete: true
Attachment #8462603 - Flags: review?(rlb)
Attachment #8462617 - Flags: review?(rlb)
Comment on attachment 8462617 [details] [diff] [review]
0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v3

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

::: dom/crypto/test/tests.js
@@ +114,5 @@
>      function doExport(x) {
>        if (!hasKeyFields(x)) {
>          window.result = x;
>          throw "Invalid key; missing field(s)";
> +      } else if ((x.algorithm.name != alg.toLowerCase()) ||

Sigh.  Every time we go through this, it makes me sad in a different way.  MAYBE WE SHOULD GO BACK TO ONLY HAVING ONE CASE, THE WAY THE ROMANS DID IT.

The sadness here is that the algorithm names in the spec are all upper-case, so it seems like it would surprise developers to have lower-case things showing up here.  I would propose that we: 
1. Add ToUpperCase(mName) to the KeyAlgorithm ctor
2. Revert these tests to use ==

r=me with that change
Attachment #8462617 - Flags: review?(rlb) → review+
> 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor

And file a spec issue, right?
(Assignee)

Updated

3 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 10

3 years ago
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #9)
> > 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor
> 
> And file a spec issue, right?

Should we rather morph the issue you filed already?

https://www.w3.org/Bugs/Public/show_bug.cgi?id=26311
(Assignee)

Comment 11

3 years ago
(In reply to Richard Barnes [:rbarnes] from comment #8)
> I would propose that we: 
> 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor
> 2. Revert these tests to use ==

Yeah, I like that a lot more. One thing though is "RSAES-PKCS1-v1_5" which we would need to turn into "RSAES-PKCS1-V1_5" for equality testing. The spec does of course list the algorithm with a lowercase "v"... That's the only algorithm where we run into this :|

How should we handle this?
Flags: needinfo?(rlb)
Oh, that's not addressed yet?  Yeah, in that case just mention the issue in there.
Also, I assume we'll be contributing tests to the spec's test suite, right?
(Assignee)

Comment 14

3 years ago
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #12)
> Oh, that's not addressed yet?  Yeah, in that case just mention the issue in
> there.

Ok.

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #13)
> Also, I assume we'll be contributing tests to the spec's test suite, right?

Yeah (not sure how that works though).
(In reply to Tim Taubert [:ttaubert] from comment #11)
> (In reply to Richard Barnes [:rbarnes] from comment #8)
> > I would propose that we: 
> > 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor
> > 2. Revert these tests to use ==
> 
> Yeah, I like that a lot more. One thing though is "RSAES-PKCS1-v1_5" which
> we would need to turn into "RSAES-PKCS1-V1_5" for equality testing. The spec
> does of course list the algorithm with a lowercase "v"... That's the only
> algorithm where we run into this :|
> 
> How should we handle this?

Well, I'm not too bothered about RSAES-PKCS1-v1_5 in particular, since that's been removed from the spec.  (We just haven't removed it from the code yet.  One component of Bug 1037892.)  But it's still a problem for RSASSA-PKCS1-v1_5.

Let's review requirements/goals here:
1. Avoid the need for lots of LowerCaseEqualsLiteral
2. Have the #define'd constants be the ones in the spec (including mixed case)
3. Have the KeyAlgorithm name values be the ones in the spec
3. Accept algorithm names per the spec

(Of course, (4) is hard to evaluate, because this requirement hasn't yet been re-added to the spec.)

How about if we add some normalization to GetAlgorithmName?  Something like the following:

> nsString tempName;
> // Extract name to tempName from aAlgorithm or SYNTAX_ERR
> if (tempName.LowerCaseEqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) {
>   aName.Assign(WEBCRYPTO_ALG_RSASSA_PKCS1);
> } else if (/* ... */) {
>   // etc.
> } else {
>   return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
> }
> return NS_OK;
Flags: needinfo?(rlb)
(Assignee)

Comment 16

3 years ago
Created attachment 8465432 [details] [diff] [review]
0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v4

Ok, so how about this?
Attachment #8462617 - Attachment is obsolete: true
Attachment #8465432 - Flags: review?(rlb)
(Assignee)

Updated

3 years ago
Summary: WebCrypto algorithm names should be normalized to ASCII lowercase → WebCrypto algorithm names should be normalized as they are defined in the spec
Comment on attachment 8465432 [details] [diff] [review]
0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v4

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

::: dom/crypto/WebCryptoTask.cpp
@@ +139,5 @@
> +  if (aName.EqualsLiteral("RSAES-PKCS1-V1_5")) {
> +    aName.AssignLiteral(WEBCRYPTO_ALG_RSAES_PKCS1);
> +  } else if (aName.EqualsLiteral("RSASSA-PKCS1-V1_5")) {
> +    aName.AssignLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1);
> +  }

Rather than special-casing the mixed-case ones (since then we have to do something special for each algorithm), I would rather we just delete the ToUpperCase() and have the if-statement cover all the cases.
Attachment #8465432 - Flags: review?(rlb) → review-
(Assignee)

Comment 18

3 years ago
Created attachment 8465510 [details] [diff] [review]
0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v5
Attachment #8465432 - Attachment is obsolete: true
Attachment #8465510 - Flags: review?(rlb)
Comment on attachment 8465510 [details] [diff] [review]
0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v5

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

LGTM.  Thanks for persisting through all the back-and-forth.
Attachment #8465510 - Flags: review?(rlb) → review+
(Assignee)

Comment 20

3 years ago
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/93c52fb1e891
https://hg.mozilla.org/mozilla-central/rev/93c52fb1e891
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.