Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto(webcrypto): ECDH Named curve mismatch #35812

Closed
panva opened this issue Oct 26, 2020 · 5 comments
Closed

crypto(webcrypto): ECDH Named curve mismatch #35812

panva opened this issue Oct 26, 2020 · 5 comments
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@panva
Copy link
Member

panva commented Oct 26, 2020

  • Version: v15.0.1
  • Platform: Darwin C02CX0K5MD6V 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64
  • Subsystem: crypto.webcrypto

What steps will reproduce the bug?

This rejects

subtle.importKey(
    "jwk",
    {
        kty: "EC",
        crv: "P-256",
        x: "kgR_PqO07L8sZOBbw6rvv7O_f7clqDeiE3WnMkb5EoI",
        y: "djI-XqCqSyO9GFk_QT_stROMCAROIvU8KOORBgQUemE",
        d: "5aPFSt0UFVXYGu-ZKyC9FQIUOAMmnjzdIwkxCMe3Iok",
        alg: 'ECDH-ES',
    },
    {
        name: "ECDH",
        namedCurve: "P-256"
    },
    false,
    ["deriveKey", "deriveBits"]
)

This works, difference is the presence of the JWK "alg" (Algorithm) Parameter

subtle.importKey(
    "jwk",
    {
        kty: "EC",
        crv: "P-256",
        x: "kgR_PqO07L8sZOBbw6rvv7O_f7clqDeiE3WnMkb5EoI",
        y: "djI-XqCqSyO9GFk_QT_stROMCAROIvU8KOORBgQUemE",
        d: "5aPFSt0UFVXYGu-ZKyC9FQIUOAMmnjzdIwkxCMe3Iok",
    },
    {
        name: "ECDH",
        namedCurve: "P-256"
    },
    false,
    ["deriveKey", "deriveBits"]
)

What is the expected behavior?

The key import promise should resolve. (I think, at least it does in Chromium's Web Crypto API implementation.

// cc @jasnell

@PoojaDurgad PoojaDurgad added the crypto Issues and PRs related to the crypto subsystem. label Oct 27, 2020
@mmomtchev
Copy link
Contributor

I am not an expert on this, but I think P-256 curve should be used only with ES256 algorithm
Maybe Chromium ignores crv when you set the algorithm to ECDH-ES
I don't think that P-256 with ECDH-ES is a valid combination, I don't see it in the RFC
https://tools.ietf.org/html/rfc7518

@panva
Copy link
Member Author

panva commented Oct 28, 2020

@mmomtchev this ticket is not about JOSE per se. But even if it was then of course the three original EC curves are usable for ECDH-ES. secp256k1 isn't per it's jose registration, but that curve isn't supported by webcrypto at all

@panva
Copy link
Member Author

panva commented Oct 28, 2020

Looking at the spec (webcrypto one) ecdh jwk key import does not work with jwk.alg at all. It seems this is indeed a bug in node's implementation.

@jasnell
Copy link
Member

jasnell commented Oct 28, 2020

Sounds like it. Do you want to take a look at a fix @panva?

@panva
Copy link
Member Author

panva commented Oct 28, 2020

@jasnell i'll take a stab at it.

panva added a commit to panva/node that referenced this issue Oct 28, 2020
This fixes the importKey operation when importing a JWK for the ECDH
algorithm. As per the Web Crypto API specification the JWK `alg`
property is not checked (as opposed to ECDSA).

fixes nodejs#35812
@danbev danbev closed this as completed in e8fe38f Oct 30, 2020
targos pushed a commit that referenced this issue Nov 3, 2020
This fixes the importKey operation when importing a JWK for the ECDH
algorithm. As per the Web Crypto API specification the JWK `alg`
property is not checked (as opposed to ECDSA).

PR-URL: #35855
Fixes: #35812
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants