Closed Bug 1271350 Opened 4 years ago Closed 4 years ago

crypto.subtle PBKDF2 interface problem

Categories

(Core :: Security, defect, major)

46 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mihaly.lengyel, Assigned: keeler)

References

Details

Attachments

(1 file)

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.
Severity: normal → major
Component: Untriaged → Security
Product: Firefox → Core
Thanks for filing this bug - NSS is definitely doing the wrong thing here.
Assignee: nobody → dkeeler
Duplicate of this bug: 1271351
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+
https://reviewboard.mozilla.org/r/52245/#review49343

Thanks for the review. I ended up basically formatting things like the PBKDF2 sha256 testcase.
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/
https://hg.mozilla.org/mozilla-central/rev/f9e0ca40cb96
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Are we sure about mozilla49? I can't find the policies regarding this.
What policies are you referring to? This should be fixed when you download a Firefox 49 build (currently Nightly).
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).
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?
We can't possibly put this into release. Maybe Beta. We should uplift to Aurora.
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 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+
Thank you!
You need to log in before you can comment on or make changes to this bug.