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

Unspecified importKey behavior for PKCS8-encoded raw private keys #356

Open
ArnaudBrousseau opened this issue Oct 13, 2023 · 6 comments
Open

Comments

@ArnaudBrousseau
Copy link

ArnaudBrousseau commented Oct 13, 2023

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 ✅

$ 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 🚫

> 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

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:

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

See nodejs/node#50174 for context

@twiss
Copy link
Member

twiss commented Oct 17, 2023

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:

If the private key value is not a valid point on the Elliptic Curve identified by the namedCurve member of normalizedAlgorithm throw a DataError.

I haven't checked, but the key you posted might have a similar issue?

@panva
Copy link
Member

panva commented Oct 17, 2023

Using this vector from the chromium test suite here are the results.

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"])
runtime EC PKCS#8 w/o public key
Bun import fulfills
Chromium import fulfills
Deno Error: InvalidEncoding
Firefox DataError
Node.js import fulfills
WebKit DataError
Workerd import fulfills

@panva
Copy link
Member

panva commented Oct 17, 2023

  1. I'll take the "equal to the order" vector passing in Node.js issue up with my fellow collaborators (crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey nodejs/node#50234)
  2. I'll look into Deno not returning the correct Error name or at least open an issue about it in Deno's tracker ((ext/crypto) importKey fulfills incorrect error denoland/deno#20931)
  3. It would appear we need something like the following language in pkcs8 ECDSA / ECDH key import operations to reflect actual implementation state and warn developers about this observed interoperability issue.
If the implementation does not support the <name this key somehow> 
format and a <name this key somehow> is provided, throw a DataError.

@twiss
Copy link
Member

twiss commented Oct 17, 2023

@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.

@panva
Copy link
Member

panva commented Oct 17, 2023

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.

@ArnaudBrousseau
Copy link
Author

ArnaudBrousseau commented Oct 17, 2023

@twiss thank you for pointing a positive test case!

I don't know why the key you posted fails to parse, but it's not because of the missing public key

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: 308141020100301306072a8648ce3d020106082a8648ce3d0301070427302502010104207632de7338577bc12c1731fa29f08019206af381f74af60f4d5e0395218f205c. Annotating the DER encoding:

vv SEQUENCE
30 81 41
   ^^^^^ length: 0x81 41 => this is the "long-form" encoding
         0x81 in binary is 0b10000001
         - bit 7 is set to toggle "long-form", 
         - bit 0-6 contain the length of the length, in this case: 0b0000001=1
   
   vv INTEGER (length 1)
   02 01 00

   vv SEQUENCE (length 0x13 = 19 = (2+7) + (2+8))
   30 13
      vv OBJECT IDENTIFIER (length 7)
      06 07 2a 86 48 ce 3d 02 01
      vv OBJECT IDENTIFIER (length 8)
      06 08 2a 86 48 ce 3d 03 01 07

   vv OCTET STRING (length 0x27 = 39)
   04 27 30 25 02 01 01 04 20 7632de7338577bc12c1731fa29f08019206af381f74af60f4d5e0395218f205c
         ^^ this is also a SEQUENCE (length: 0x25=37). See "Elliptic Curve Private Key Format"
            in https://www.ietf.org/rfc/rfc5915.txt

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 3041020100301306072a8648ce3d020106082a8648ce3d0301070427302502010104207632de7338577bc12c1731fa29f08019206af381f74af60f4d5e0395218f205c, which import successfully in Chrome!

> 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants