Browse Source

fix: ECDH.setPrivateKey correctly sets the key (#27688)

Jeremy Rose 4 years ago
parent
commit
4ce9dd3a79

+ 1 - 0
patches/node/.patches

@@ -32,3 +32,4 @@ fix_add_safeforterminationscopes_for_sigint_interruptions.patch
 remove_makeexternal_case_for_uncached_internal_strings.patch
 fix_remove_outdated_--experimental-wasm-bigint_flag.patch
 darwin_libuv_use_posix_spawn.patch
+fix_parallel_test-crypto-ecdh-convert-key_to_use_compatible_group.patch

+ 9 - 11
patches/node/fix_comment_out_incompatible_crypto_modules.patch

@@ -9,7 +9,7 @@ with what's exposed through BoringSSL. I plan to upstream parts of this or
 otherwise introduce shims to reduce friction.
 
 diff --git a/src/node_crypto.cc b/src/node_crypto.cc
-index 91cb94d8dbe9db0adbee5e005649188e1ccbcbf9..c3d12dc4cc18888815ff5e2c30a21974322d1faf 100644
+index 91cb94d8dbe9db0adbee5e005649188e1ccbcbf9..2000c789d9daac835c0ecc1e4144179575c9b502 100644
 --- a/src/node_crypto.cc
 +++ b/src/node_crypto.cc
 @@ -5192,11 +5192,11 @@ bool DiffieHellman::Init(int primeLength, int g) {
@@ -48,18 +48,16 @@ index 91cb94d8dbe9db0adbee5e005649188e1ccbcbf9..c3d12dc4cc18888815ff5e2c30a21974
      return false;
    }
    BIGNUM* bn_p =
-@@ -5718,8 +5718,9 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo<Value>& args) {
- 
+@@ -5719,7 +5719,7 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo<Value>& args) {
    if (!EC_KEY_set_public_key(new_key.get(), pub.get()))
      return env->ThrowError("Failed to set generated public key");
--
-+#if 0
-   EC_KEY_copy(ecdh->key_.get(), new_key.get());
-+#endif
+ 
+-  EC_KEY_copy(ecdh->key_.get(), new_key.get());
++  ecdh->key_.reset(EC_KEY_dup(new_key.get()));
    ecdh->group_ = EC_KEY_get0_group(ecdh->key_.get());
  }
  
-@@ -6207,6 +6208,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
+@@ -6207,6 +6207,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
    EVPKeyCtxPointer Setup() override {
      EVPKeyPointer params;
      if (prime_info_.fixed_value_) {
@@ -67,7 +65,7 @@ index 91cb94d8dbe9db0adbee5e005649188e1ccbcbf9..c3d12dc4cc18888815ff5e2c30a21974
        DHPointer dh(DH_new());
        if (!dh)
          return nullptr;
-@@ -6223,6 +6225,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
+@@ -6223,6 +6224,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
        params = EVPKeyPointer(EVP_PKEY_new());
        CHECK(params);
        EVP_PKEY_assign_DH(params.get(), dh.release());
@@ -75,7 +73,7 @@ index 91cb94d8dbe9db0adbee5e005649188e1ccbcbf9..c3d12dc4cc18888815ff5e2c30a21974
      } else {
        EVPKeyCtxPointer param_ctx(EVP_PKEY_CTX_new_id(EVP_PKEY_DH, nullptr));
        if (!param_ctx)
-@@ -6230,7 +6233,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
+@@ -6230,7 +6232,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
  
        if (EVP_PKEY_paramgen_init(param_ctx.get()) <= 0)
          return nullptr;
@@ -84,7 +82,7 @@ index 91cb94d8dbe9db0adbee5e005649188e1ccbcbf9..c3d12dc4cc18888815ff5e2c30a21974
        if (EVP_PKEY_CTX_set_dh_paramgen_prime_len(param_ctx.get(),
                                                   prime_info_.prime_size_) <= 0)
          return nullptr;
-@@ -6238,7 +6241,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
+@@ -6238,7 +6240,7 @@ class DHKeyPairGenerationConfig : public KeyPairGenerationConfig {
        if (EVP_PKEY_CTX_set_dh_paramgen_generator(param_ctx.get(),
                                                   generator_) <= 0)
          return nullptr;

+ 23 - 0
patches/node/fix_parallel_test-crypto-ecdh-convert-key_to_use_compatible_group.patch

@@ -0,0 +1,23 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Jeremy Rose <[email protected]>
+Date: Tue, 9 Feb 2021 11:59:08 -0800
+Subject: fix parallel/test-crypto-ecdh-convert-key to use compatible group
+
+This fixes a node crypto test to use an algorithm that's boringssl also
+supports.
+
+This should be upstreamed.
+
+diff --git a/test/parallel/test-crypto-ecdh-convert-key.js b/test/parallel/test-crypto-ecdh-convert-key.js
+index 69ee339aa7a653a8f2b4523bf8b28f1b2254c705..93074a42f770fb4d26c609520fa4c72f520c0d1b 100644
+--- a/test/parallel/test-crypto-ecdh-convert-key.js
++++ b/test/parallel/test-crypto-ecdh-convert-key.js
+@@ -117,7 +117,7 @@ if (getCurves().includes('secp256k1')) {
+   // rather than Node's generic error message.
+   const badKey = 'f'.repeat(128);
+   assert.throws(
+-    () => ECDH.convertKey(badKey, 'secp256k1', 'hex', 'hex', 'compressed'),
++    () => ECDH.convertKey(badKey, 'secp521r1', 'hex', 'hex', 'compressed'),
+     /Failed to convert Buffer to EC_POINT/);
+ 
+   // Next statement should not throw an exception.

+ 0 - 1
script/node-disabled-tests.json

@@ -25,7 +25,6 @@
   "parallel/test-crypto-des3-wrap",
   "parallel/test-crypto-dh",
   "parallel/test-crypto-ecb",
-  "parallel/test-crypto-ecdh-convert-key",
   "parallel/test-crypto-engine",
   "parallel/test-crypto-hash-stream-pipe",
   "parallel/test-crypto-key-objects",