From 782b6f6f9fcde4b7d94f4bd06b6d0d0937d31594 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 30 Dec 2022 12:38:53 +0100 Subject: [PATCH] crypto: ensure exported webcrypto EC keys use uncompressed point format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The WebCrypto spec apparently mandates that EC keys must be exported in uncompressed point format. This commit makes it so. Fixes: https://github.com/nodejs/node/issues/45859 PR-URL: https://github.com/nodejs/node/pull/46021 Reviewed-By: Tobias Nießen Reviewed-By: Filip Skokan --- src/crypto/crypto_ec.cc | 45 ++++++++++++++++++- .../test-webcrypto-export-import-ec.js | 13 ++++++ test/wpt/status/WebCryptoAPI.json | 13 ------ 3 files changed, 56 insertions(+), 15 deletions(-) diff --git a/src/crypto/crypto_ec.cc b/src/crypto/crypto_ec.cc index 2fc759e452fec9..3ccea99fb2bbe9 100644 --- a/src/crypto/crypto_ec.cc +++ b/src/crypto/crypto_ec.cc @@ -703,10 +703,51 @@ WebCryptoKeyExportStatus ECKeyExportTraits::DoExport( if (key_data->GetKeyType() != kKeyTypePrivate) return WebCryptoKeyExportStatus::INVALID_KEY_TYPE; return PKEY_PKCS8_Export(key_data.get(), out); - case kWebCryptoKeyFormatSPKI: + case kWebCryptoKeyFormatSPKI: { if (key_data->GetKeyType() != kKeyTypePublic) return WebCryptoKeyExportStatus::INVALID_KEY_TYPE; - return PKEY_SPKI_Export(key_data.get(), out); + + ManagedEVPPKey m_pkey = key_data->GetAsymmetricKey(); + if (EVP_PKEY_id(m_pkey.get()) != EVP_PKEY_EC) { + return PKEY_SPKI_Export(key_data.get(), out); + } else { + // Ensure exported key is in uncompressed point format. + // The temporary EC key is so we can have i2d_PUBKEY_bio() write out + // the header but it is a somewhat silly hoop to jump through because + // the header is for all practical purposes a static 26 byte sequence + // where only the second byte changes. + Mutex::ScopedLock lock(*m_pkey.mutex()); + const EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(m_pkey.get()); + const EC_GROUP* group = EC_KEY_get0_group(ec_key); + const EC_POINT* point = EC_KEY_get0_public_key(ec_key); + const point_conversion_form_t form = POINT_CONVERSION_UNCOMPRESSED; + const size_t need = + EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr); + if (need == 0) return WebCryptoKeyExportStatus::FAILED; + ByteSource::Builder data(need); + const size_t have = EC_POINT_point2oct( + group, point, form, data.data(), need, nullptr); + if (have == 0) return WebCryptoKeyExportStatus::FAILED; + ECKeyPointer ec(EC_KEY_new()); + CHECK_EQ(1, EC_KEY_set_group(ec.get(), group)); + ECPointPointer uncompressed(EC_POINT_new(group)); + CHECK_EQ(1, + EC_POINT_oct2point(group, + uncompressed.get(), + data.data(), + data.size(), + nullptr)); + CHECK_EQ(1, EC_KEY_set_public_key(ec.get(), uncompressed.get())); + EVPKeyPointer pkey(EVP_PKEY_new()); + CHECK_EQ(1, EVP_PKEY_set1_EC_KEY(pkey.get(), ec.get())); + BIOPointer bio(BIO_new(BIO_s_mem())); + CHECK(bio); + if (!i2d_PUBKEY_bio(bio.get(), pkey.get())) + return WebCryptoKeyExportStatus::FAILED; + *out = ByteSource::FromBIO(bio); + return WebCryptoKeyExportStatus::OK; + } + } default: UNREACHABLE(); } diff --git a/test/parallel/test-webcrypto-export-import-ec.js b/test/parallel/test-webcrypto-export-import-ec.js index ec5615b6582f88..94e4285165a174 100644 --- a/test/parallel/test-webcrypto-export-import-ec.js +++ b/test/parallel/test-webcrypto-export-import-ec.js @@ -327,6 +327,19 @@ async function testImportRaw({ name, publicUsages }, namedCurve) { await Promise.all(tests); })().then(common.mustCall()); + +// https://github.com/nodejs/node/issues/45859 +(async function() { + const 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]); // eslint-disable-line max-len + const 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]); // eslint-disable-line max-len + for (const name of ['ECDH', 'ECDSA']) { + const options = { name, namedCurve: 'P-256' }; + const key = await subtle.importKey('spki', compressed, options, true, []); + const spki = await subtle.exportKey('spki', key); + assert.deepStrictEqual(uncompressed, Buffer.from(spki)); + } +})().then(common.mustCall()); + { const rsaPublic = crypto.createPublicKey( fixtures.readKey('rsa_public_2048.pem')); diff --git a/test/wpt/status/WebCryptoAPI.json b/test/wpt/status/WebCryptoAPI.json index 16cd616c1f9675..9f101f6cdd92c9 100644 --- a/test/wpt/status/WebCryptoAPI.json +++ b/test/wpt/status/WebCryptoAPI.json @@ -1,17 +1,4 @@ { - "import_export/ec_importKey.https.any.js": { - "fail": { - "note": "Compressed point export should result in uncompressed point https://github.com/nodejs/node/issues/45859", - "expected": [ - "Good parameters: P-256 bits (spki, buffer(59, compressed), {name: ECDSA, namedCurve: P-256}, true, [])", - "Good parameters: P-384 bits (spki, buffer(72, compressed), {name: ECDSA, namedCurve: P-384}, true, [])", - "Good parameters: P-521 bits (spki, buffer(90, compressed), {name: ECDSA, namedCurve: P-521}, true, [])", - "Good parameters: P-256 bits (spki, buffer(59, compressed), {name: ECDH, namedCurve: P-256}, true, [])", - "Good parameters: P-384 bits (spki, buffer(72, compressed), {name: ECDH, namedCurve: P-384}, true, [])", - "Good parameters: P-521 bits (spki, buffer(90, compressed), {name: ECDH, namedCurve: P-521}, true, [])" - ] - } - }, "algorithm-discards-context.https.window.js": { "skip": "Not relevant in Node.js context" },