Closed Bug 1500292 Opened 1 year ago Closed 1 year ago

Allow zero-length keys in WebCrypto's importKey for PBKDF2

Categories

(Core :: DOM: Security, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files)

The sync team is trying to move away from a hand-rolled js pbkdf2 implementation [0] by using WebCrypto, however sometimes we need to stretch an empty passphrase.
At the moment, the following line rejects:
> crypto.subtle.importKey("raw", new Uint8Array([]), {name: "PBKDF2"}, false, ["deriveBits"]);

Would it be easy to fix this behavior?

[0] https://searchfox.org/mozilla-central/rev/c56977420df7a1b692ce0f7e499ddb364d9fd7b2/services/crypto/modules/utils.js#190
Hmmm. I'm not clear on whether this is allowed by the spec [1]. 

Have you run the web platform tests with and without this change? I know our coverage is poor, but just curious.

Dana: Thoughts?

[1] https://www.w3.org/TR/WebCryptoAPI/#pbkdf2
Flags: needinfo?(dkeeler)
From my reading, the construction of HMAC (i.e. the underlying PRF in PBKDF2 in this case) involves padding the key, so I believe a zero-length key would be valid. Also it turns out we already have bug 1488432 for this (but I'll duplicate that to this since we already have a patch here).
Flags: needinfo?(dkeeler)
I'm going to add the requested unit test to the patch and run these web platform tests, who should I tag for review later?
Thank you.
I have updated my patch, I've also noticed that I'm getting a lot of UNEXPECTED-PASS from WebCryptoAPI web-platform-tests, so I assume that's a good thing.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6fdedd504f6f0635ea6b142463f9c8bce0fdd37

Assuming we are happy with the patch, should I update the wpt expectations in a 2nd commit?
(In reply to Edouard Oger [:eoger] from comment #7)

> Assuming we are happy with the patch, should I update the wpt expectations
> in a 2nd commit?

Yes, I'd say keep it on this bug. Thanks!
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Attachment #9018692 - Attachment description: Bug 1500292 - Remove 0-length key checks in ImportSymmetricKeyTask and DerivePbkdfBitsTask. → Bug 1500292 p1 - Remove 0-length key checks in ImportSymmetricKeyTask and DerivePbkdfBitsTask.
Blocks: 1490671
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ecb7bf0e6f2
p1 - Remove 0-length key checks in ImportSymmetricKeyTask and DerivePbkdfBitsTask. r=jcj
https://hg.mozilla.org/integration/autoland/rev/e9bf22cdf91a
p2 - Update web-platform-tests expected data. a=testonly r=jcj
https://hg.mozilla.org/mozilla-central/rev/4ecb7bf0e6f2
https://hg.mozilla.org/mozilla-central/rev/e9bf22cdf91a
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Looks like this also bumped the number of WebCryptoAPI passing tests on wpt.fyi from 12097 to 19078 out of 23410 :)
Thanks a lot to the both of you for the swift reviews and guidance!
You need to log in before you can comment on or make changes to this bug.