-
Notifications
You must be signed in to change notification settings - Fork 56
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
Unspecified importKey
behavior for PKCS8-encoded raw private keys
#356
Comments
Hey 👋 I don't know why the key you posted fails to parse, but it's not because of the missing public key. Chromium has a positive test for that here, which indeed works: 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, 31, 227, 57, 80, 197, 244, 97, 18, 74, 233, 146, 194, 189, 253, 241, 199, 59, 22, 21, 245, 113, 189, 86, 126, 96, 209, 154, 161, 244, 140, 223, 66]);
await crypto.subtle.importKey("pkcs8", buffer, { name: "ECDSA", namedCurve: "P-256" }, true, ["sign"])
// CryptoKey {type: 'private', extractable: true, algorithm: {…}, usages: Array(1)} The ASN.1 structure is the same: https://lapo.it/asn1js/#MEECAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcEJzAlAgEBBCAf4zlQxfRhEkrpksK9_fHHOxYV9XG9Vn5g0Zqh9IzfQg. Instead, the negative test you linked says "The private key is exactly equal to the order", and the key is rejected because of that, which the spec says to do:
I haven't checked, but the key you posted might have a similar issue? |
Using this vector from the chromium test suite here are the results.
|
|
@panva Thanks for testing this everywhere! Before we resign to considering this as undefined behavior, I think it would be ideal if we could first try to reach a consistent behavior across implementations, if possible; ideally accepting the key as that's what the spec currently implies. E.g., it might be worth opening an issue with Deno about that first, rather than just the error name? I can also open issues with the browser. And then we could add this test case to WPT, as well. If we get pushback, or implementations are unable to support this, we can always reconsider, but tbh I would prefer not to add such text if we can avoid it. |
I don't think the effort is worth the added value of these keys being universally accepted by implementations. Worst case scenario the bug reports sit in Firefox, WebKit and Deno trackers for years and the spec says nothing about this issue. If it could have been done for compressed points, and noone's using those because of it, it can be done for these bare keys too. The issue from my POV is not that some runtimes don't support this, but that the spec doesn't give them option to. |
@twiss thank you for pointing a positive test case!
You're right! After looking at the bytes in more details, the bytes fail to parse because of invalid (rather: non-canonical) length bytes. The bytes I was trying to import were, in hex:
So I think the issue is that this is valid BER encoding, but invalid DER encoding since DER says "always use the smallest possible length representation". Instead of 0x8141 the length should be encoded as simply 0x41. The corrected bytes are > var 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,31,227,57,80,197,244,97,18,74,233,146,194,189,253,241,199,59,22,21,245,113,189,86,126,96,209,154,161,244,140,223,66])
> await crypto.subtle.importKey("pkcs8", buffer, { name: "ECDSA", namedCurve: "P-256" }, true, ["sign"])
CryptoKey {type: 'private', extractable: true, algorithm: {…}, usages: Array(1)} So, in addition to the problems discussed above, I think we've discovered that NodeJS/Bun's parsers (openSSL under the hood?) are potentially too lax and allow for non-canonical DER encoding. Namely: they allow the use of long-form length bytes when short-form is canonical. Note: even with the corrected bytes, Firefox still fails to import. Haven't tried other runtimes but I suspect the results will be consistent with the table @panva compiled above in #356 (comment) |
Summary
The current spec doesn't specify a behavior for PKC8-encoded raw private key import (when I say "raw private key" I mean "without the optional public key bytes").
Chrome and other browsers seem firmly in the camp of "this should be an error" (see this test) and throw a
DataError
, while Bun and NodeJS import the key without throwing an error.Steps to repro
Here's a snippet to reproduce the inconsistent behavior:
In Node/Bun ✅
In browsers 🚫
The imported bytes above represent a P-256 private key without the public key bytes (in hex:
308141020100301306072a8648ce3d020106082a8648ce3d0301070427302502010104207632de7338577bc12c1731fa29f08019206af381f74af60f4d5e0395218f205c
). Here's a asn1js link to see the structure@panva helped me run this case across multiple implementations, here are the results:
See nodejs/node#50174 for context
The text was updated successfully, but these errors were encountered: