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

WebCryptoAPI EC key SPKI export does not always use uncompressed point format #45859

Closed
panva opened this issue Dec 14, 2022 · 9 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. webcrypto

Comments

@panva
Copy link
Member

panva commented Dec 14, 2022

Our WebCryptoAPI implementation supports import of compressed point format EC keys in both raw, and spki forms. That is an optional to implement part of the API.

The specification however requires that when keys are exported they unconditionally use the uncompressed point format. This is currently not the case in Node.js when compressed point spki was used to import the CryptoKey.

The following script demonstrates the issue.

Script
import * as assert from 'assert';

const { subtle } = globalThis.crypto;

const keyData = {
  'P-256': {
    uncompressed: Buffer.from([
      48, 89, 48, 19, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 8, 42, 134, 72, 206, 61, 3, 1, 7, 3, 66, 0, 4, 210, 16, 176, 166, 249, 217, 240, 18, 134, 128, 88, 180, 63, 164, 244, 113, 1, 133, 67, 187, 160, 12, 146, 80, 223, 146, 87, 194, 172, 174, 93, 209, 206, 3, 117, 82, 212, 129, 69, 12, 227, 155, 77, 16, 149, 112, 27, 23, 91, 250, 179, 75, 142, 108, 9, 158, 24, 241, 193, 152, 53, 131, 97, 232,
    ]),
    compressed: Buffer.from([
      48, 57, 48, 19, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 8, 42, 134, 72, 206, 61, 3, 1, 7, 3, 34, 0, 2, 210, 16, 176, 166, 249, 217, 240, 18, 134, 128, 88, 180, 63, 164, 244, 113, 1, 133, 67, 187, 160, 12, 146, 80, 223, 146, 87, 194, 172, 174, 93, 209,
    ]),
  },

  'P-384': {
    uncompressed: Buffer.from([
      48, 118, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 34, 3, 98, 0, 4, 33, 156, 20, 214, 102, 23, 179, 110, 198, 216, 133, 107, 56, 91, 115, 167, 77, 52, 79, 216, 174, 117, 239, 4, 100, 53, 221, 165, 78, 59, 68, 189, 95, 189, 235, 209, 208, 141, 214, 158, 45, 125, 193, 220, 33, 140, 180, 53, 189, 40, 19, 140, 199, 120, 51, 122, 132, 47, 107, 214, 27, 36, 14, 116, 36, 159, 36, 102, 124, 42, 88, 16, 167, 107, 252, 40, 224, 51, 95, 136, 166, 80, 29, 236, 1, 151, 109, 168, 90, 251, 0, 134, 156, 182, 172, 232,
    ]),
    compressed: Buffer.from([
      48, 70, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 34, 3, 50, 0, 2, 33, 156, 20, 214, 102, 23, 179, 110, 198, 216, 133, 107, 56, 91, 115, 167, 77, 52, 79, 216, 174, 117, 239, 4, 100, 53, 221, 165, 78, 59, 68, 189, 95, 189, 235, 209, 208, 141, 214, 158, 45, 125, 193, 220, 33, 140, 180, 53,
    ]),
  },
  'P-521': {
    uncompressed: Buffer.from([
      48, 129, 155, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 35, 3, 129, 134, 0, 4, 1, 86, 244, 121, 248, 223, 30, 32, 167, 255, 192, 76, 228, 32, 195, 225, 84, 174, 37, 25, 150, 190, 228, 47, 3, 75, 132, 212, 27, 116, 63, 52, 228, 95, 49, 27, 129, 58, 156, 222, 200, 205, 165, 155, 187, 189, 49, 212, 96, 179, 41, 37, 33, 231, 193, 183, 34, 229, 102, 124, 3, 219, 47, 174, 117, 63, 1, 80, 23, 54, 207, 226, 71, 57, 67, 32, 216, 228, 175, 194, 253, 57, 181, 169, 51, 16, 97, 184, 30, 34, 65, 40, 43, 158, 23, 137, 24, 34, 181, 183, 158, 5, 47, 69, 151, 181, 150, 67, 253, 57, 55, 156, 81, 189, 81, 37, 196, 244, 139, 195, 240, 37, 206, 60, 211, 105, 83, 40, 108, 203, 56, 251,
    ]),
    compressed: Buffer.from([
      48, 88, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 35, 3, 68, 0, 3, 1, 86, 244, 121, 248, 223, 30, 32, 167, 255, 192, 76, 228, 32, 195, 225, 84, 174, 37, 25, 150, 190, 228, 47, 3, 75, 132, 212, 27, 116, 63, 52, 228, 95, 49, 27, 129, 58, 156, 222, 200, 205, 165, 155, 187, 189, 49, 212, 96, 179, 41, 37, 33, 231, 193, 183, 34, 229, 102, 124, 3, 219, 47, 174, 117, 63,
    ]),
  },
};

const curves = Object.keys(keyData);
const testVectors = [
  {
    name: 'ECDSA',
    privateUsages: ['sign'],
    publicUsages: ['verify'],
  },
  {
    name: 'ECDH',
    privateUsages: ['deriveKey', 'deriveBits'],
    publicUsages: [],
  },
];

async function testRoundTripSpki({ name, publicUsages }, namedCurve) {
  const { compressed, uncompressed } = keyData[namedCurve];

  const key = await subtle.importKey(
    'spki',
    compressed,
    { name, namedCurve },
    true,
    publicUsages
  );

  const spki = await subtle.exportKey('spki', key);
  assert.deepStrictEqual(
    Buffer.from(spki).toString('hex'),
    uncompressed.toString('hex')
  );
}

async function testRoundTripRaw({ name, publicUsages }, namedCurve) {
  let { compressed, uncompressed } = keyData[namedCurve];
  let i = uncompressed.indexOf(Buffer.from([0x00, 0x04])) + 1;
  uncompressed = uncompressed.subarray(i);

  i =
    compressed.indexOf(Buffer.from([0x00, 0x02])) + 1 ||
    compressed.indexOf(Buffer.from([0x00, 0x03])) + 1;

  compressed = compressed.subarray(i);

  const key = await subtle.importKey(
    'raw',
    compressed,
    { name, namedCurve },
    true,
    publicUsages
  );

  const raw = await subtle.exportKey('raw', key);

  assert.strictEqual(
    Buffer.from(raw).toString('hex'),
    uncompressed.toString('hex')
  );
}

for (const namedCurve of curves) {
  for (const vector of testVectors) {
    await testRoundTripSpki(vector, namedCurve).then(
      () =>
        console.log(namedCurve, 'spki'.padEnd(4), vector.name.padEnd(5), '✅'),
      () =>
        console.log(namedCurve, 'spki'.padEnd(4), vector.name.padEnd(5), '❌')
    );
    await testRoundTripRaw(vector, namedCurve).then(
      () =>
        console.log(namedCurve, 'raw'.padEnd(4), vector.name.padEnd(5), '✅'),
      () =>
        console.log(namedCurve, 'raw'.padEnd(4), vector.name.padEnd(5), '❌')
    );
    console.log('\n');
  }
}
P-256 spki ECDSA ❌
P-256 raw  ECDSA ✅


P-256 spki ECDH  ❌
P-256 raw  ECDH  ✅


P-384 spki ECDSA ❌
P-384 raw  ECDSA ✅


P-384 spki ECDH  ❌
P-384 raw  ECDH  ✅


P-521 spki ECDSA ❌
P-521 raw  ECDSA ✅


P-521 spki ECDH  ❌
P-521 raw  ECDH  ✅

I'm opening this issue so that I have an issue tracker link to add to the WPT status file once it gets updated.

@panva panva added confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. webcrypto labels Dec 14, 2022
@panva
Copy link
Member Author

panva commented Dec 14, 2022

@tniessen and I tried to find a way of ensuring EC keys use uncompressed point format upon export but short of some very brute and inefficient JS hacks using the Web Crypto API itself we didn't find a way.

@panva
Copy link
Member Author

panva commented Dec 14, 2022

@jasnell do you have any ideas on how to resolve this issue in either the spki import invocation or the spki export invocation?

@panva
Copy link
Member Author

panva commented Dec 14, 2022

cc @nodejs/crypto

@tniessen
Copy link
Member

@tniessen and I tried to find a way of ensuring EC keys use uncompressed point format upon export but short of some very brute and inefficient JS hacks using the Web Crypto API itself we didn't find a way.

To clarify, it's certainly possible in C++ but it requires some refactoring because the internal code passes KeyObjectData objects around, which we cannot modify, so we either need to create a temporary copy or pass around lower-level OpenSSL types.

@panva
Copy link
Member Author

panva commented Dec 14, 2022

I think that if we can identify the KeyObjectData is using compressed point then creating a temporary copy for the spki export in this edge-case would be okay?

@bnoordhuis
Copy link
Member

This is about crypto.subtle.exportKey('spki', key) not writing uncompressed ECDH and ECDSA keys, correct?

Seems like that should be a trivial fix in PKEY_SPKI_Export() from src/crypto/crypto_keys.cc; the logic to go from compressed to uncompressed already exists in ECDH::ConvertKey(), it's basically a wrapper around openssl's EC_POINT_point2oct(). Is there some subtlety that I'm missing?

@panva
Copy link
Member Author

panva commented Dec 29, 2022

It's just that the existing functionality works with ec point and the key representation we have is ec key.

If you see an obvious way to do this, great!

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 30, 2022
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: nodejs#45859
@bnoordhuis
Copy link
Member

#46021. It's admittedly not very elegant but oh well.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 30, 2022
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: nodejs#45859
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 30, 2022
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: nodejs#45859
@tniessen
Copy link
Member

Is there some subtlety that I'm missing?
It's admittedly not very elegant but oh well.

Only that one :) Thanks for working on this.

@panva panva closed this as completed in 8ec1b08 Jan 1, 2023
RafaelGSS pushed a commit that referenced this issue Jan 2, 2023
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: #45859
PR-URL: #46021
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jan 4, 2023
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: #45859
PR-URL: #46021
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jan 5, 2023
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: #45859
PR-URL: #46021
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: #45859
PR-URL: #46021
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
The WebCrypto spec apparently mandates that EC keys must be exported in
uncompressed point format. This commit makes it so.

Fixes: #45859
PR-URL: #46021
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. webcrypto
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants