Closed
Bug 1271350
Opened 8 years ago
Closed 8 years ago
crypto.subtle PBKDF2 interface problem
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mihaly.lengyel, Assigned: keeler)
References
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
ttaubert
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details |
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.94 Safari/537.36 Steps to reproduce: I used window.crypto.subtle to derive a password with pbkdf2. Both importKey and deriveBits are callable with both Uint8Arrays and ArrayBuffers, see example below. window.crypto.subtle.importKey('raw', new Uint8Array([156, 19, 162, 59, 197, 138, 82, 190, 139, 180, 250, 26, 44, 189, 255, 1, 116, 114, 101, 115, 111, 114, 105, 116, 95, 100, 98, 95, 114, 101, 99, 111, 114, 100, 95, 105, 100]), {name: 'PBKDF2', hash: { name: 'SHA-256' } }, false, ['deriveBits']) .then(key => window.crypto.subtle.deriveBits({ name: "PBKDF2", hash: { name: 'SHA-256'}, salt: new Uint8Array(0), iterations: 1 }, key, 256)) .then(res => console.log(new Uint8Array(res)), rej => console.log(rej)); window.crypto.subtle.importKey('raw', new Uint8Array([156, 19, 162, 59, 197, 138, 82, 190, 139, 180, 250, 26, 44, 189, 255, 1, 116, 114, 101, 115, 111, 114, 105, 116, 95, 100, 98, 95, 114, 101, 99, 111, 114, 100, 95, 105, 100]).buffer, {name: 'PBKDF2', hash: { name: 'SHA-256' } }, false, ['deriveBits']) .then(key => window.crypto.subtle.deriveBits({ name: "PBKDF2", hash: { name: 'SHA-256'}, salt: new Uint8Array(0).buffer, iterations: 1 }, key, 256)) .then(res => console.log(new Uint8Array(res)), rej => console.log(rej)); Actual results: They gave different results. Expected results: I expected the same results or a rejection in the worst case. They give the same result in Chrome.
Reporter | ||
Updated•8 years ago
|
Severity: normal → major
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52245/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52245/
Attachment #8751848 -
Flags: review?(ttaubert)
Assignee | ||
Comment 2•8 years ago
|
||
Thanks for filing this bug - NSS is definitely doing the wrong thing here.
Assignee: nobody → dkeeler
Comment 4•8 years ago
|
||
Comment on attachment 8751848 [details] MozReview Request: bug 1271350 - work around PK11_CreatePBEV2AlgorithmID generating a random salt when it shouldn't r?ttaubert https://reviewboard.mozilla.org/r/52245/#review49343 Seems like the best solution, probably not the nicest but it seems that we have to keep the old NSS API for now. Thanks for fixing this! ::: dom/crypto/test/test_WebCrypto_PBKDF2.html:242 (Diff revision 1) > + var key = new Uint8Array([156, 19, 162, 59, 197, 138, 82, 190, 139, 180, > + 250, 26, 44, 189, 255, 1, 116, 114, 101, 115, > + 111, 114, 105, 116, 95, 100, 98, 95, 114, 101, > + 99, 111, 114, 100, 95, 105, 100]); > + var expectedDerivedKey = new Uint8Array([239, 41, 221, 56, 47, 166, 106, > + 131, 169, 91, 231, 204, 251, 113, > + 241, 204, 254, 228, 148, 151, 120, > + 85, 164, 194, 96, 217, 12, 47, 140, > + 145, 224, 98]); Please add these as hex strings to dom/crypto/test/test-vectors.js and reference them with `tv.pbkdf2_sha256.*`. ::: dom/crypto/test/test_WebCrypto_PBKDF2.html:263 (Diff revision 1) > + name: "PBKDF2", > + hash: "SHA-256", > + salt: new Uint8Array(0), > + iterations: 1 > + }; > + return crypto.subtle.deriveBits(deriveAlg, x, 256); I think this should be `deriveBits(deriveAlg, x, expectedDerivedKey.byteLength * 8)` or something along those lines.
Attachment #8751848 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 5•8 years ago
|
||
https://reviewboard.mozilla.org/r/52245/#review49343 Thanks for the review. I ended up basically formatting things like the PBKDF2 sha256 testcase.
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8751848 [details] MozReview Request: bug 1271350 - work around PK11_CreatePBEV2AlgorithmID generating a random salt when it shouldn't r?ttaubert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52245/diff/1-2/
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9e0ca40cb96
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 10•8 years ago
|
||
What policies are you referring to? This should be fixed when you download a Firefox 49 build (currently Nightly).
Comment 11•8 years ago
|
||
IMHO this should be included in the current release not the nightly. The PBKDF2 used to derive encryption keys is just wrong, which may easily result in user data loss (using Firefox).
Comment 12•8 years ago
|
||
There is no way to detect the wrong behavior, only user-agent checking is possible to prevent the error. We shouldn't do a stable release without the patch. WebCrypto is a W3C Recommendation and Firefox has no vendor prefix on this API. Do I miss something?
Comment 13•8 years ago
|
||
We can't possibly put this into release. Maybe Beta. We should uplift to Aurora.
Updated•8 years ago
|
Comment 14•8 years ago
|
||
Comment on attachment 8751848 [details] MozReview Request: bug 1271350 - work around PK11_CreatePBEV2AlgorithmID generating a random salt when it shouldn't r?ttaubert Approval Request Comment [Feature/regressing bug #]: bug 1021607 [User impact if declined]: No direct user impact, developers might stumble over it. [Describe test coverage new/current, TreeHerder]: Good test coverage. [Risks and why]: Risk is very low, it's a self-contained change that only affects WebCrypto API behavior in rare situations. [String/UUID change made/needed]: None.
Attachment #8751848 -
Flags: approval-mozilla-beta?
Attachment #8751848 -
Flags: approval-mozilla-aurora?
Comment 15•8 years ago
|
||
Comment on attachment 8751848 [details] MozReview Request: bug 1271350 - work around PK11_CreatePBEV2AlgorithmID generating a random salt when it shouldn't r?ttaubert This is too late for beta but happy to take it for aurora.
Attachment #8751848 -
Flags: approval-mozilla-beta?
Attachment #8751848 -
Flags: approval-mozilla-beta-
Attachment #8751848 -
Flags: approval-mozilla-aurora?
Attachment #8751848 -
Flags: approval-mozilla-aurora+
Comment 16•8 years ago
|
||
Thanks!
Comment 17•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/76d7c6af162c72bbc0b8fb4be01159408b9f2ff4
Comment 18•8 years ago
|
||
Thank you!
You need to log in
before you can comment on or make changes to this bug.
Description
•