Closed Bug 1020882 Opened 5 years ago Closed 5 years ago

length param of HmacKeyGenParams should be optional

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

http://www.w3.org/TR/WebCryptoAPI/#dfn-HmacKeyGenParams

"The length (in bits) of the key to generate. If unspecified, the recommended length will be used, which is the size of the associated hash function's block size."
Is there a better place or way to do this?
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8434847 - Flags: review?(bzbarsky)
Comment on attachment 8434847 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch

The IDL interaction bits look good to me.  Richard, can you review the switch bit and tests?
Attachment #8434847 - Flags: review?(rlb)
Attachment #8434847 - Flags: review?(bzbarsky)
Attachment #8434847 - Flags: review+
Comment on attachment 8434847 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch

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

Thanks for catching this.  There might be a few other default cases that are missing, so your review is very helpful.

This is the right place to address the problem.

::: dom/crypto/WebCryptoTask.cpp
@@ +1032,5 @@
>  
> +      if (params.mLength.WasPassed()) {
> +        mLength = params.mLength.Value();
> +      } else {
> +        KeyAlgorithm hashAlg(global, hashName);

This seems duplicative of the HmacKeyAlgorithm construction below.

@@ +1039,5 @@
> +          case CKM_SHA224: mLength = 224; break;
> +          case CKM_SHA256: mLength = 256; break;
> +          case CKM_SHA384: mLength = 384; break;
> +          case CKM_SHA512: mLength = 512; break;
> +          default: mLength = 0; break;

I'm concerned about this default case.  If Mechanism() returns something unknown, we're in more serious trouble.  Could you check to see where we fail for an unknown hash algorithm?
Attachment #8434847 - Flags: review?(rlb) → review-
(In reply to Richard Barnes [:rbarnes] from comment #3)
> ::: dom/crypto/WebCryptoTask.cpp
> @@ +1032,5 @@
> >  
> > +      if (params.mLength.WasPassed()) {
> > +        mLength = params.mLength.Value();
> > +      } else {
> > +        KeyAlgorithm hashAlg(global, hashName);
> 
> This seems duplicative of the HmacKeyAlgorithm construction below.

I'm only doing this to let it parse the string into a Mechanism. HmacKeyAlgorithm wants the length given to the constructor and I can't change it afterwards. I could as well just do string comparisons like the KeyAlgorithm constructor does?
(In reply to Tim Taubert [:ttaubert] from comment #4)
> (In reply to Richard Barnes [:rbarnes] from comment #3)
> > ::: dom/crypto/WebCryptoTask.cpp
> > @@ +1032,5 @@
> > >  
> > > +      if (params.mLength.WasPassed()) {
> > > +        mLength = params.mLength.Value();
> > > +      } else {
> > > +        KeyAlgorithm hashAlg(global, hashName);
> > 
> > This seems duplicative of the HmacKeyAlgorithm construction below.
> 
> I'm only doing this to let it parse the string into a Mechanism.
> HmacKeyAlgorithm wants the length given to the constructor and I can't
> change it afterwards. I could as well just do string comparisons like the
> KeyAlgorithm constructor does?

Good point.  Let's just leave it.
You're absolutely right, I missed that the spec says we should fail with a DataError when length is zero. Added two more test cases.
Attachment #8434847 - Attachment is obsolete: true
Attachment #8435229 - Flags: review?(rlb)
Comment on attachment 8435229 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch, v2

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

Looks good to me!
Attachment #8435229 - Flags: review?(rlb) → review+
Comment on attachment 8435229 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch, v2

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

Looks good to me!
https://hg.mozilla.org/mozilla-central/rev/65f1eeeb742c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1022343
You need to log in before you can comment on or make changes to this bug.