1760806 - crypto.subtle.importKey doesn't check the curve when importing JWK ECC keys

Status: RESOLVED FIXED

(Core :: DOM: Web Crypto, defect, P2)

Description

[Daniel Huigens [:twiss]]

Steps to reproduce:

In Web Crypto, when importing an ECDSA or ECDH key in JsonWebKey format, the importKey() function doesn't check that the JWK "crv" field matches the "namedCurve" field of the algorithm identifier. It uses the crv field for the resulting key, which means that you can silently end up with a key of a different curve than you expected. For ECDSA keys, it also doesn't check the "alg" field ("ES256" / "ES384" / "ES521"), which should also correspond to the specified curve.

Actual results:

Some examples:

const ecdsaKey = await crypto.subtle.importKey('jwk', {
    "alg": "ES256",
    "crv": "P-256",
    "ext": true,
    "key_ops": ["verify"],
    "kty": "EC",
    "x": "OHqWk67pCFd4uTsIdPqBYY07hyO437NrWfeQUQemMUw",
    "y": "vw6udLfdNpXVS5r7MmsPWCZwPKv3ZWh2PyXOyReeVVY"
}, {
    "name": "ECDSA",
    "namedCurve": "P-521"
}, true, ["verify"]);
console.log(ecdsaKey.algorithm.namedCurve); // P-256

const ecdhKey = await crypto.subtle.importKey('jwk', {
    "crv": "P-256",
    "d": "kRyHUtNO4MjE4tG5RFjDYFIE7cMFTQieOozwvwxdiwU",
    "ext": true,
    "key_ops": ["deriveBits"],
    "kty": "EC",
    "x": "CRWJeGR9DSxUDaPwBm5FuY3aaQohKOYU2hxnaFiq94I",
    "y": "C-Qk2LL2DbA0yndHXDGU-_hwK53lQOcGG1JpWN-KxEc"
}, {
    "name": "ECDH",
    "namedCurve": "P-521"
}, true, ["deriveBits"]);
console.log(ecdhKey.algorithm.namedCurve); // P-256

Expected results:

Both calls to importKey() should throw a DataError. This is required by the Web Crypto spec in the steps to import ECDSA and ECDH keys, here for ECDSA and here for ECDH (search for If format is "jwk":).

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:keeler, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Daniel Veditz [:dveditz]]

Ben: if there's a need to block public visibility of this issue after two years is it worse than the "S4" severity indicates? What security rating (sec-high, sec-low?) would you give it.

Comment 3

[Benjamin Beurdouche [:beurdouche]]

We did a group triaging session and decided to have a second look at it just in case, Anna, will take another look shortly

Comment 4

[Anna Weine]

Attachment: Bug 1760806 - WebCrypto: ECDH and ECDSA JWK import to check that the crv in params and crv in alg are the same

Comment 5

[Anna Weine]

Hi Daniel,
should we add this example into the list of WebCrypto tests?

Thanks!

Comment 6

[Daniel Huigens [:twiss]]

Yeah, it would be good to test this in wpt (and perhaps a bit more generally, e.g. other combinations as well); there's even a note about it here. I can take a stab at it, but I'll be on vacation the next two weeks so it'd probably be after that. If you want to beat me to it feel free ofc :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcacbdd1e5b5
WebCrypto: ECDH and ECDSA JWK import to check that the crv in params and crv in alg are the same r=keeler

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bcacbdd1e5b5

Comment 9

[Liz Henry (:lizzard)]

Does this still need a rating (sec-low, etc)?

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

I think we are good with just sec-audit, though to confirm I'll refer to Dan.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for ESR128 uplift? It grafts cleanly.

Comment 12

[Anna Weine]

Attachment: Bug 1760806 - WebCrypto: ECDH and ECDSA JWK import to check that the crv in params and crv in alg are the same

https://treeherder.mozilla.org/jobs?repo=try&revision=ed7936b105dea8e588650feb6baf26a50a6439fc

Original Revision: https://phabricator.services.mozilla.com/D217273

Comment 13

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: None
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Just in case. It's a webcrypto code, so in order to check the change you should call https://bugzilla.mozilla.org/show_bug.cgi?id=1760806#c0 code through browser terminal
• Risk associated with taking this patch: Little risk
• Explanation of risk level: the keys could be generated with a wrong parameters, that will cause the other operations not to work without revealing any user data
• String changes made/needed: No
• Is Android affected?: no

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/2416fd1777b2