From 7518c5392184cac3d118bfec4f154ab8c70432ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Mon, 8 Jul 2024 17:59:52 -0400 Subject: [PATCH 1/6] crypto: reject dh,x25519,x448 in {Sign,Verify}Final Fixes: https://github.com/nodejs/node/issues/53742 --- src/crypto/crypto_sig.cc | 28 ++++++++++++++---------- test/fixtures/keys/dh_private.pem | 9 ++++++++ test/fixtures/keys/dh_public.pem | 9 ++++++++ test/parallel/test-crypto-sign-verify.js | 28 ++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 test/fixtures/keys/dh_private.pem create mode 100644 test/fixtures/keys/dh_public.pem diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc index 3441d7e7718ad6..bb7cab780529dc 100644 --- a/src/crypto/crypto_sig.cc +++ b/src/crypto/crypto_sig.cc @@ -93,11 +93,12 @@ std::unique_ptr Node_SignFinal(Environment* env, } EVPKeyCtxPointer pkctx(EVP_PKEY_CTX_new(pkey.get(), nullptr)); if (pkctx && - EVP_PKEY_sign_init(pkctx.get()) && + EVP_PKEY_sign_init(pkctx.get()) > 0 && ApplyRSAOptions(pkey, pkctx.get(), padding, pss_salt_len) && - EVP_PKEY_CTX_set_signature_md(pkctx.get(), EVP_MD_CTX_md(mdctx.get())) && + EVP_PKEY_CTX_set_signature_md(pkctx.get(), + EVP_MD_CTX_md(mdctx.get())) > 0 && EVP_PKEY_sign(pkctx.get(), static_cast(sig->Data()), - &sig_len, m, m_len)) { + &sig_len, m, m_len) > 0) { CHECK_LE(sig_len, sig->ByteLength()); if (sig_len == 0) { sig = ArrayBuffer::NewBackingStore(env->isolate(), 0); @@ -527,14 +528,19 @@ SignBase::Error Verify::VerifyFinal(const ManagedEVPPKey& pkey, return kSignPublicKey; EVPKeyCtxPointer pkctx(EVP_PKEY_CTX_new(pkey.get(), nullptr)); - if (pkctx && - EVP_PKEY_verify_init(pkctx.get()) > 0 && - ApplyRSAOptions(pkey, pkctx.get(), padding, saltlen) && - EVP_PKEY_CTX_set_signature_md(pkctx.get(), - EVP_MD_CTX_md(mdctx.get())) > 0) { - const unsigned char* s = sig.data(); - const int r = EVP_PKEY_verify(pkctx.get(), s, sig.size(), m, m_len); - *verify_result = r == 1; + if (pkctx) { + const int init_ret = EVP_PKEY_verify_init(pkctx.get()); + if (init_ret == -2) { + return kSignPublicKey; + } + if (init_ret > 0 && + ApplyRSAOptions(pkey, pkctx.get(), padding, saltlen) && + EVP_PKEY_CTX_set_signature_md(pkctx.get(), + EVP_MD_CTX_md(mdctx.get())) > 0) { + const unsigned char* s = sig.data(); + const int r = EVP_PKEY_verify(pkctx.get(), s, sig.size(), m, m_len); + *verify_result = r == 1; + } } return kSignOk; diff --git a/test/fixtures/keys/dh_private.pem b/test/fixtures/keys/dh_private.pem new file mode 100644 index 00000000000000..262e6896495b69 --- /dev/null +++ b/test/fixtures/keys/dh_private.pem @@ -0,0 +1,9 @@ +-----BEGIN PRIVATE KEY----- +MIIBIQIBADCBlQYJKoZIhvcNAQMBMIGHAoGBANEfWLkepFV7Ym8nPQblm/B3+mGl +ptmFYnrDvCpm+cw3w8SSoVIhZTB/q561jy/zDh1ZAT+K/gO0Go80sUhpv9XeNSxw +eY9bZx6LVqWltgkuWmjtZRUwwExXopZpvcpw0Cn/XH9fb+o7+RDFTk/VJvEbJXcY +mDBF7to/skujEjiHAgECBIGDAoGAVxqjqDJvQY9R+XmxYM1SCaT9gJh8f+TYHn4y +0j5/7c7rej9toPLX3Et72182AZdw87y/AUexfrXT/F31v3wxYFxk2n1j8/7hTmpH +MZnWLYoa5Rjs0X3a3BExN08O1X7pfB+qI4E+Dpzeqx5dcELWcfKS9YCPBBfwaUyP +RXVC7TA= +-----END PRIVATE KEY----- \ No newline at end of file diff --git a/test/fixtures/keys/dh_public.pem b/test/fixtures/keys/dh_public.pem new file mode 100644 index 00000000000000..6b98cdd9439970 --- /dev/null +++ b/test/fixtures/keys/dh_public.pem @@ -0,0 +1,9 @@ +-----BEGIN PUBLIC KEY----- +MIIBIDCBlQYJKoZIhvcNAQMBMIGHAoGBANEfWLkepFV7Ym8nPQblm/B3+mGlptmF +YnrDvCpm+cw3w8SSoVIhZTB/q561jy/zDh1ZAT+K/gO0Go80sUhpv9XeNSxweY9b +Zx6LVqWltgkuWmjtZRUwwExXopZpvcpw0Cn/XH9fb+o7+RDFTk/VJvEbJXcYmDBF +7to/skujEjiHAgECA4GFAAKBgQDEEE8yLWxIej02E5FeKHpPvO6e2+nV/hhEdlrK +0N5awvX/xex4R/VCyKSdycv9dgPE+q84d+iwYhrEwZeUPzWwOpCuqvOZyeF9V63V +iNecJEKHjRR3SRh95+6BVB04JASNVj+YHKybdOhptAGgZVa+vUG8jznCamHtJB/h +Ulxzvw== +-----END PUBLIC KEY----- \ No newline at end of file diff --git a/test/parallel/test-crypto-sign-verify.js b/test/parallel/test-crypto-sign-verify.js index 56e5c16c2867f0..9a3b7add93740d 100644 --- a/test/parallel/test-crypto-sign-verify.js +++ b/test/parallel/test-crypto-sign-verify.js @@ -793,3 +793,31 @@ assert.throws( }, { code: 'ERR_CRYPTO_UNSUPPORTED_OPERATION', message: 'Unsupported crypto operation' }); } } + +{ + const keys = [ + { + privateKey: fixtures.readKey('dh_private.pem', 'ascii'), + publicKey: fixtures.readKey('dh_public.pem', 'ascii'), + }, + { + privateKey: fixtures.readKey('x25519_private.pem', 'ascii'), + publicKey: fixtures.readKey('x25519_public.pem', 'ascii'), + }, + { + privateKey: fixtures.readKey('x448_private.pem', 'ascii'), + publicKey: fixtures.readKey('x448_public.pem', 'ascii'), + }, + ]; + for (const { publicKey, privateKey } of keys) { + assert.throws(() => { + crypto.createSign('SHA256').update('Test123').sign(privateKey); + }, { code: 'ERR_OSSL_EVP_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE', message: /operation not supported for this keytype/ }); + assert.throws(() => { + crypto.createVerify('SHA256').update('Test123').verify(privateKey, 'sig'); + }, { code: 'ERR_OSSL_EVP_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE', message: /operation not supported for this keytype/ }); + assert.throws(() => { + crypto.createVerify('SHA256').update('Test123').verify(publicKey, 'sig'); + }, { code: 'ERR_OSSL_EVP_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE', message: /operation not supported for this keytype/ }); + } +} From e71ebfe824f8bb4a070b8706f18f91ccfb8c178e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Tue, 9 Jul 2024 09:43:28 -0400 Subject: [PATCH 2/6] format cpp --- src/crypto/crypto_sig.cc | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc index bb7cab780529dc..fa4ba62f8f94f5 100644 --- a/src/crypto/crypto_sig.cc +++ b/src/crypto/crypto_sig.cc @@ -92,13 +92,15 @@ std::unique_ptr Node_SignFinal(Environment* env, sig = ArrayBuffer::NewBackingStore(env->isolate(), sig_len); } EVPKeyCtxPointer pkctx(EVP_PKEY_CTX_new(pkey.get(), nullptr)); - if (pkctx && - EVP_PKEY_sign_init(pkctx.get()) > 0 && + if (pkctx && EVP_PKEY_sign_init(pkctx.get()) > 0 && ApplyRSAOptions(pkey, pkctx.get(), padding, pss_salt_len) && - EVP_PKEY_CTX_set_signature_md(pkctx.get(), - EVP_MD_CTX_md(mdctx.get())) > 0 && - EVP_PKEY_sign(pkctx.get(), static_cast(sig->Data()), - &sig_len, m, m_len) > 0) { + EVP_PKEY_CTX_set_signature_md(pkctx.get(), EVP_MD_CTX_md(mdctx.get())) > + 0 && + EVP_PKEY_sign(pkctx.get(), + static_cast(sig->Data()), + &sig_len, + m, + m_len) > 0) { CHECK_LE(sig_len, sig->ByteLength()); if (sig_len == 0) { sig = ArrayBuffer::NewBackingStore(env->isolate(), 0); @@ -533,10 +535,9 @@ SignBase::Error Verify::VerifyFinal(const ManagedEVPPKey& pkey, if (init_ret == -2) { return kSignPublicKey; } - if (init_ret > 0 && - ApplyRSAOptions(pkey, pkctx.get(), padding, saltlen) && - EVP_PKEY_CTX_set_signature_md(pkctx.get(), - EVP_MD_CTX_md(mdctx.get())) > 0) { + if (init_ret > 0 && ApplyRSAOptions(pkey, pkctx.get(), padding, saltlen) && + EVP_PKEY_CTX_set_signature_md(pkctx.get(), EVP_MD_CTX_md(mdctx.get())) > + 0) { const unsigned char* s = sig.data(); const int r = EVP_PKEY_verify(pkctx.get(), s, sig.size(), m, m_len); *verify_result = r == 1; From ea9bda83d1a089defdedbc59827894ba0c04d1de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Tue, 9 Jul 2024 09:56:12 -0400 Subject: [PATCH 3/6] generate fixture dh keys from openssl --- test/fixtures/keys/Makefile | 8 ++++++++ test/fixtures/keys/dh_private.pem | 16 ++++++++-------- test/fixtures/keys/dh_public.pem | 21 +++++++++++++-------- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/test/fixtures/keys/Makefile b/test/fixtures/keys/Makefile index 313183f6d6e3ed..cea10a6ab3ba15 100644 --- a/test/fixtures/keys/Makefile +++ b/test/fixtures/keys/Makefile @@ -25,6 +25,8 @@ all: \ dh1024.pem \ dh2048.pem \ dherror.pem \ + dh_private.pem \ + dh_public.pem \ dsa_params.pem \ dsa_private.pem \ dsa_private_encrypted.pem \ @@ -597,6 +599,12 @@ dh2048.pem: dherror.pem: dh1024.pem sed 's/^[^-].*/AAAAAAAAAA/g' dh1024.pem > dherror.pem +dh_private.pem: + openssl genpkey -algorithm dh -out dh_private.pem -pkeyopt dh_param:ffdhe2048 + +dh_public.pem: dh_private.pem + openssl pkey -in dh_private.pem -pubout -out dh_public.pem + dsa_params.pem: openssl dsaparam -out dsa_params.pem 2048 diff --git a/test/fixtures/keys/dh_private.pem b/test/fixtures/keys/dh_private.pem index 262e6896495b69..25c4edc5ea5a3b 100644 --- a/test/fixtures/keys/dh_private.pem +++ b/test/fixtures/keys/dh_private.pem @@ -1,9 +1,9 @@ -----BEGIN PRIVATE KEY----- -MIIBIQIBADCBlQYJKoZIhvcNAQMBMIGHAoGBANEfWLkepFV7Ym8nPQblm/B3+mGl -ptmFYnrDvCpm+cw3w8SSoVIhZTB/q561jy/zDh1ZAT+K/gO0Go80sUhpv9XeNSxw -eY9bZx6LVqWltgkuWmjtZRUwwExXopZpvcpw0Cn/XH9fb+o7+RDFTk/VJvEbJXcY -mDBF7to/skujEjiHAgECBIGDAoGAVxqjqDJvQY9R+XmxYM1SCaT9gJh8f+TYHn4y -0j5/7c7rej9toPLX3Et72182AZdw87y/AUexfrXT/F31v3wxYFxk2n1j8/7hTmpH -MZnWLYoa5Rjs0X3a3BExN08O1X7pfB+qI4E+Dpzeqx5dcELWcfKS9YCPBBfwaUyP -RXVC7TA= ------END PRIVATE KEY----- \ No newline at end of file +MIIBPgIBADCCARcGCSqGSIb3DQEDATCCAQgCggEBAP//////////rfhUWKK7Spqv +3FYgJz088di5xYPOLTaVqeE2QRRkM/vMk53OJJs++X0v42NjDHXY9oGyAq7EYXrT +3x7V1f1lYSQz9R9fBm7QhWNlVT3tGvO1VxNef1fJNZhPDHDg5ot34qaJ2vPv6HId +8VihNq3nNTCsyk9IOnl6vAqxgrMk+2HRCKlLssjj+7lq2rdg1/RoHU9Co945TfSu +Vu3nY3K7GQsHp8juCm1wngL84c334uzANATNKDQvYZFy/pzphYP/jk8SMu7ygYPD +/jsbTG+tczu1/LwuwiAFxY7xg30Wg7LG80omwbLv+ohrQjhhKFyX//////////8C +AQIEHgIcKNGyhQRxIhVXoyktdymwbN6MgXv85vPax+8eqQ== +-----END PRIVATE KEY----- diff --git a/test/fixtures/keys/dh_public.pem b/test/fixtures/keys/dh_public.pem index 6b98cdd9439970..b32815e88acc8c 100644 --- a/test/fixtures/keys/dh_public.pem +++ b/test/fixtures/keys/dh_public.pem @@ -1,9 +1,14 @@ -----BEGIN PUBLIC KEY----- -MIIBIDCBlQYJKoZIhvcNAQMBMIGHAoGBANEfWLkepFV7Ym8nPQblm/B3+mGlptmF -YnrDvCpm+cw3w8SSoVIhZTB/q561jy/zDh1ZAT+K/gO0Go80sUhpv9XeNSxweY9b -Zx6LVqWltgkuWmjtZRUwwExXopZpvcpw0Cn/XH9fb+o7+RDFTk/VJvEbJXcYmDBF -7to/skujEjiHAgECA4GFAAKBgQDEEE8yLWxIej02E5FeKHpPvO6e2+nV/hhEdlrK -0N5awvX/xex4R/VCyKSdycv9dgPE+q84d+iwYhrEwZeUPzWwOpCuqvOZyeF9V63V -iNecJEKHjRR3SRh95+6BVB04JASNVj+YHKybdOhptAGgZVa+vUG8jznCamHtJB/h -Ulxzvw== ------END PUBLIC KEY----- \ No newline at end of file +MIICJTCCARcGCSqGSIb3DQEDATCCAQgCggEBAP//////////rfhUWKK7Spqv3FYg +Jz088di5xYPOLTaVqeE2QRRkM/vMk53OJJs++X0v42NjDHXY9oGyAq7EYXrT3x7V +1f1lYSQz9R9fBm7QhWNlVT3tGvO1VxNef1fJNZhPDHDg5ot34qaJ2vPv6HId8Vih +Nq3nNTCsyk9IOnl6vAqxgrMk+2HRCKlLssjj+7lq2rdg1/RoHU9Co945TfSuVu3n +Y3K7GQsHp8juCm1wngL84c334uzANATNKDQvYZFy/pzphYP/jk8SMu7ygYPD/jsb +TG+tczu1/LwuwiAFxY7xg30Wg7LG80omwbLv+ohrQjhhKFyX//////////8CAQID +ggEGAAKCAQEA2whDVdYtNbr/isSFdw7rOSdbmcWrxiX6ppqDZ6yp8XjUj3/CEf/P +60X7HndX+nXD7YaPtVZxktkIpArI7C+AH7fZxBduuv2eLnvYwK82jFHKe7zvfdMr +26akMCV0kBA3ktgcftHlqYsIj52BaJlG37FRha3SDOL2yJOij3hNQhHCXTWLg7tP +GtXmD202OoZ6Ll+LxBzBCFnxVauiKnzBGeawy4gDycUEHmq5oDRR68I2gmxmsLg5 +MQVAP5ljp+FEu4+TZm6hR4wQ5PRjCQ+teq+VqMro7EbbvZpn+X9kAgKSl2WDu0fT +FbUnBn3HPBmUa/Fv/ooXrlckTUDjLkbWZQ== +-----END PUBLIC KEY----- From 6f98eb17a01773fefa668f6d8ee2d27e2b6a20e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Tue, 9 Jul 2024 10:14:37 -0400 Subject: [PATCH 4/6] add test comment --- test/parallel/test-crypto-sign-verify.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/parallel/test-crypto-sign-verify.js b/test/parallel/test-crypto-sign-verify.js index 9a3b7add93740d..3b74c874b81868 100644 --- a/test/parallel/test-crypto-sign-verify.js +++ b/test/parallel/test-crypto-sign-verify.js @@ -795,6 +795,8 @@ assert.throws( } { + // dh, x25519 and x448 should not be used for signing/verifying + // https://github.com/nodejs/node/issues/53742 const keys = [ { privateKey: fixtures.readKey('dh_private.pem', 'ascii'), From 560daa4c867577fc8d09562c0fcccf3ab88b4315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Wed, 10 Jul 2024 16:05:03 -0400 Subject: [PATCH 5/6] fix linter-js error --- test/parallel/test-crypto-sign-verify.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-crypto-sign-verify.js b/test/parallel/test-crypto-sign-verify.js index 3b74c874b81868..f0cfd7a6016ece 100644 --- a/test/parallel/test-crypto-sign-verify.js +++ b/test/parallel/test-crypto-sign-verify.js @@ -795,7 +795,7 @@ assert.throws( } { - // dh, x25519 and x448 should not be used for signing/verifying + // Dh, x25519 and x448 should not be used for signing/verifying // https://github.com/nodejs/node/issues/53742 const keys = [ { From 3f90853e83e3fc2d1457dded5d83b9c8100500a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Wed, 7 Aug 2024 10:46:10 -0400 Subject: [PATCH 6/6] Update test-crypto-sign-verify.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Tobias Nießen --- test/parallel/test-crypto-sign-verify.js | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/test/parallel/test-crypto-sign-verify.js b/test/parallel/test-crypto-sign-verify.js index f0cfd7a6016ece..9dd586a1a1f9a0 100644 --- a/test/parallel/test-crypto-sign-verify.js +++ b/test/parallel/test-crypto-sign-verify.js @@ -797,21 +797,9 @@ assert.throws( { // Dh, x25519 and x448 should not be used for signing/verifying // https://github.com/nodejs/node/issues/53742 - const keys = [ - { - privateKey: fixtures.readKey('dh_private.pem', 'ascii'), - publicKey: fixtures.readKey('dh_public.pem', 'ascii'), - }, - { - privateKey: fixtures.readKey('x25519_private.pem', 'ascii'), - publicKey: fixtures.readKey('x25519_public.pem', 'ascii'), - }, - { - privateKey: fixtures.readKey('x448_private.pem', 'ascii'), - publicKey: fixtures.readKey('x448_public.pem', 'ascii'), - }, - ]; - for (const { publicKey, privateKey } of keys) { + for (const algo of ['dh', 'x25519', 'x448']) { + const privateKey = fixtures.readKey(`${algo}_private.pem`, 'ascii'); + const publicKey = fixtures.readKey(`${algo}_public.pem`, 'ascii'); assert.throws(() => { crypto.createSign('SHA256').update('Test123').sign(privateKey); }, { code: 'ERR_OSSL_EVP_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE', message: /operation not supported for this keytype/ });