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

Inconsistent behavior between Node and browser crypto with PKCS8 EC imports #50174

Closed
ArnaudBrousseau opened this issue Oct 13, 2023 · 5 comments
Labels
crypto Issues and PRs related to the crypto subsystem. question Issues that look for answers. webcrypto

Comments

@ArnaudBrousseau
Copy link

ArnaudBrousseau commented Oct 13, 2023

Version

v20.8.0

Platform

MacOS

Subsystem

crypto

What steps will reproduce the bug?

In Node console:

$ node
Welcome to Node.js v20.8.0.
Type ".help" for more information.
> require("crypto")
> buffer = new Uint8Array([48,129,65,2,1,0,48,19,6,7,42,134,72,206,61,2,1,6,8,42,134,72,206,61,3,1,7,4,39,48,37,2,1,1,4,32,118,50,222,115,56,87,123,193,44,23,49,250,41,240,128,25,32,106,243,129,247,74,246,15,77,94,3,149,33,143,32,92])
> await crypto.subtle.importKey("pkcs8", buffer, { name: "ECDSA", namedCurve: "P-256" }, true, ["sign"])
CryptoKey {
  type: 'private',
  extractable: true,
  algorithm: { name: 'ECDSA', namedCurve: 'P-256' },
  usages: [ 'sign' ]
}

In browsers (tried both Firefox and Chrome):

> buffer = new Uint8Array([48,129,65,2,1,0,48,19,6,7,42,134,72,206,61,2,1,6,8,42,134,72,206,61,3,1,7,4,39,48,37,2,1,1,4,32,118,50,222,115,56,87,123,193,44,23,49,250,41,240,128,25,32,106,243,129,247,74,246,15,77,94,3,149,33,143,32,92])
> await crypto.subtle.importKey("pkcs8", buffer, { name: "ECDSA", namedCurve: "P-256" }, true, ["sign"])
Uncaught Error

Additional information

The bytes I'm importing are, in hex format: 308141020100301306072a8648ce3d020106082a8648ce3d0301070427302502010104207632de7338577bc12c1731fa29f08019206af381f74af60f4d5e0395218f205c

You can use asn1js to decode this pkc8 structure: https://lapo.it/asn1js/#MIFBAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBCcwJQIBAQQgdjLeczhXe8EsFzH6KfCAGSBq84H3SvYPTV4DlSGPIFw
image

This is a P-256 private key with no public key bytes (they are optional in pkcs8)

I'm honestly not sure that browsers are doing the right thing here but at the very least this behavior ("error when no public key bytes are present") seems intentional: Chrome has an explicit test case for it:

// The private key is exactly equal to the order, and the public key is
// omitted.
{
  "crv": "P-256",
  "key_format": "pkcs8",
  "key": "3041020100301306072A8648CE3D020106082A8648CE3D030107042730250201010420FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551",
  "error": "DataError"
},

This specific private key (from the test case) also succeeds in NodeJS:

// This is `3041020100301306072A8648CE3D020106082A8648CE3D030107042730250201010420FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551`
buffer = new Uint8Array([48,65,2,1,0,48,19,6,7,42,134,72,206,61,2,1,6,8,42,134,72,206,61,3,1,7,4,39,48,37,2,1,1,4,32,255,255,255,255,0,0,0,0,255,255,255,255,255,255,255,255,188,230,250,173,167,23,158,132,243,185,202,194,252,99,37,81])
> await crypto.subtle.importKey("pkcs8", buffer, { name: "ECDSA", namedCurve: "P-256" }, true, ["sign"])
CryptoKey {
  type: 'private',
  extractable: true,
  algorithm: { name: 'ECDSA', namedCurve: 'P-256' },
  usages: [ 'sign' ]
}
@panva panva added question Issues that look for answers. crypto Issues and PRs related to the crypto subsystem. webcrypto labels Oct 13, 2023
@panva
Copy link
Member

panva commented Oct 13, 2023

The specification1 does not specify the intended behaviour in this case. In Node.js we get the successful import behaviour "for free" from OpenSSL which re-calculates the public key for us. I suspect that browser crypto engines don't do this, which is why they chose to throw a DataError instead.

To me they're both correct, albeit not interoperable, behaviours.

Footnotes

  1. https://w3c.github.io/webcrypto/

@ArnaudBrousseau
Copy link
Author

@panva I think that's right: they're both "correct" but not interoperable. Does Node's "crypto" try to be interoperable in general?

For context: the lack of interop hurts when unit testing code meant to run both in browsers and in Node environments. I was surprised when errors happened during manual testing (in browser) while my unit tests were green (using Node)

@panva
Copy link
Member

panva commented Oct 13, 2023

Does Node's "crypto" try to be interoperable in general?

The very reason Node.js has WebCryptoAPI is interoperability and code portability across other Web-interoperable runtimes.

When it comes to Web APIs, such as WebCryptoAPI, the benchmark for interop is WPTs (Web Platform Tests), you can see how Node.js fares in that regard (also in comparison to other runtimes participating in the WPT reporting) using the wpt.fyi dashboard 1 UI.

I went ahead and tested your vector in different WebCryptoAPI implementations as well:

runtime result
Bun ✔️
Chromium DataError
Deno DataError
Firefox DataError
Node.js ✔️
WebKit DataError
Workerd DataError

As I've noted above, there's no language present in the specification, or its underpinnings, demanding one behaviour or another, and there are no WPTs for the behaviour either. If something is left unspecified, there can't be any talks of interoperability, it's up to every implementer's best judgement / capabilities. Node.js is able to work with the key, other runtimes can't.

FWIW there are even optional to implement features in the specification itself, such as supporting compressed public keys. In those cases the following language is present in the specification:

If the implementation does not support the compressed point format and a compressed point is provided, throw a DataError.

If you'll open an issue about this specification blind spot it is most likely going to end up being an optional to implement feature as well with a similar language, meaning all runtimes will be in compliance.

Footnotes

  1. https://wpt.fyi/results/WebCryptoAPI?label=master&label=experimental&product=node.js&product=deno&product=chrome&product=edge&product=firefox&product=safari&view=subtest&q=node.js%3A%21missing

@ArnaudBrousseau
Copy link
Author

Interesting, thank you for running the vector in all implementations! I've opened an issue about this blind spot, we'll see what the feedback is: w3c/webcrypto#356

@panva
Copy link
Member

panva commented Oct 13, 2023

I'm keeping an eye on the w3c/webcrypto repo as well as WPTs for changes. If this turns out to go some place where changes are needed we'll make them. Until then I believe this can be considered resolved?

@panva panva closed this as completed Oct 13, 2023
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. question Issues that look for answers. webcrypto
Projects
None yet
Development

No branches or pull requests

2 participants