crypto: avoid unitializing ECDH objects on error by tniessen · Pull Request #34302 · nodejs/node

tniessen · 2020-07-10T23:54:41Z

The previous code changed the private key of the ECDH object, but removed the public key if deriving it from the private key failed. Instead, if deriving the public key fails, neither the private nor the public key stored in the ECDH object should be updated.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2020-07-17T19:41:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32391/

nodejs-github-bot · 2020-07-20T16:24:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32422/

addaleax · 2020-07-21T19:16:51Z

I’m guessing this test failure is not unrelated? https://ci.nodejs.org/job/node-test-commit-linux-containered/21411/nodes=ubi81_sharedlibs_openssl111fips_x64/testReport/junit/(root)/test/parallel_test_crypto_dh/

internal/crypto/diffiehellman.js:178
  this[kHandle].setPublicKey(toBuf(key, encoding));
                ^

Error: Failed to convert Buffer to EC_POINT
    at ECDH.setPublicKey (internal/crypto/diffiehellman.js:178:17)
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-crypto-dh.js:316:9)
    at Module._compile (internal/modules/cjs/loader.js:1250:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1271:10)
    at Module.load (internal/modules/cjs/loader.js:1099:32)
    at Function.Module._load (internal/modules/cjs/loader.js:964:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47

tniessen · 2020-08-14T08:44:02Z

I ran the test about 100,000 times locally without errors, so the error seems to be platform-specific.

CI stress test: https://ci.nodejs.org/job/node-stress-single-test/167/

It seems to never fail on rhel7-s390x.
It always fails on alpine-last-latest-x64, alpine-latest-x64 with a crash:

Crash

10:56:17 not ok 1 parallel/test-crypto-dh
10:56:17   ---
10:56:17   duration_ms: 1.313
10:56:17   severity: crashed
10:56:17   exitcode: -11
10:56:17   stack: |-
10:56:17   ...

It seems to be flaky on win10-vs2019, producing either one of these errors:

Failed to convert Buffer to EC_POINT

11:15:26 not ok 1 parallel/test-crypto-dh
11:15:26   ---
11:15:26   duration_ms: 2.214
11:15:26   severity: fail
11:15:26   exitcode: 1
11:15:26   stack: |-
11:15:26     internal/crypto/diffiehellman.js:178
11:15:26       this[kHandle].setPublicKey(toBuf(key, encoding));
11:15:26                     ^
11:15:26     
11:15:26     Error: Failed to convert Buffer to EC_POINT
11:15:26         at ECDH.setPublicKey (internal/crypto/diffiehellman.js:178:17)
11:15:26         at Object.<anonymous> (C:\workspace\node-stress-single-test\nodes\win10-vs2019\test\parallel\test-crypto-dh.js:316:9)
11:15:26         at Module._compile (internal/modules/cjs/loader.js:1252:30)
11:15:26         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1273:10)
11:15:26         at Module.load (internal/modules/cjs/loader.js:1101:32)
11:15:26         at Function.Module._load (internal/modules/cjs/loader.js:966:14)
11:15:26         at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
11:15:26         at internal/main/run_main_module.js:17:47
11:15:26   ...

ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY

11:43:04 not ok 1 parallel/test-crypto-dh
11:43:04   ---
11:43:04   duration_ms: 2.113
11:43:04   severity: fail
11:43:04   exitcode: 1
11:43:04   stack: |-
11:43:04     internal/crypto/diffiehellman.js:127
11:43:04         throw new ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY();
11:43:04         ^
11:43:04     
11:43:04     Error [ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY]: Public key is not valid for specified curve
11:43:04         at new NodeError (internal/errors.js:253:15)
11:43:04         at ECDH.dhComputeSecret [as computeSecret] (internal/crypto/diffiehellman.js:127:11)
11:43:04         at Object.<anonymous> (C:\workspace\node-stress-single-test\nodes\win10-vs2019\test\parallel\test-crypto-dh.js:360:28)
11:43:04         at Module._compile (internal/modules/cjs/loader.js:1252:30)
11:43:04         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1273:10)
11:43:04         at Module.load (internal/modules/cjs/loader.js:1101:32)
11:43:04         at Function.Module._load (internal/modules/cjs/loader.js:966:14)
11:43:04         at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
11:43:04         at internal/main/run_main_module.js:17:47 {
11:43:04       code: 'ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY'
11:43:04     }
11:43:04   ...

New stress test with a few more platforms: https://ci.nodejs.org/job/node-stress-single-test/168/

The test always succeeds on rhel7-s390x, debian9-64, and ubuntu1804-64.
The test always crashes on alpine-last-latest-x64, alpine-latest-x64.
- crashed (-11)
The test is flaky on win10-vs2019 with only 20 executions succeeding, and 980 failing.
- crashed (-1073741819)
- ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY
- Failed to convert Buffer to EC_POINT
The test is flaky on osx1015 with 979 succeeding, and 21 failing.
- crashed (-11)
- ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY

Seems like a memory-related bug. Can reproduce in valgrind on Ubuntu.

The previous code changed the private key of the ECDH object, but removed the public key if deriving it from the private key failed. Instead, if deriving the public key fails, neither the private nor the public key stored in the ECDH object should be updated.

nodejs-github-bot · 2020-08-14T14:19:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32790/

tniessen · 2020-08-14T18:22:56Z

Landed in 4a9e312, thanks for reviewing.

The previous code changed the private key of the ECDH object, but removed the public key if deriving it from the private key failed. Instead, if deriving the public key fails, neither the private nor the public key stored in the ECDH object should be updated. PR-URL: #34302 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

tniessen added crypto Issues and PRs related to the crypto subsystem. openssl Issues and PRs related to the OpenSSL dependency. labels Jul 10, 2020

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 10, 2020

bnoordhuis approved these changes Jul 11, 2020

View reviewed changes

jasnell approved these changes Jul 12, 2020

View reviewed changes

tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 17, 2020

This was referenced Jul 18, 2020

CI Relability 2020-07-18 JaneaSystems/nodejs-reliability#41

Open

CI Relability 2020-07-19 JaneaSystems/nodejs-reliability#42

Open

CI Relability 2020-07-20 JaneaSystems/nodejs-reliability#43

Open

addaleax approved these changes Jul 20, 2020

View reviewed changes

github-actions bot mentioned this pull request Jul 21, 2020

CI Relability 2020-07-21 JaneaSystems/nodejs-reliability#44

Open

10 tasks

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 21, 2020

This was referenced Jul 22, 2020

CI Relability 2020-07-22 JaneaSystems/nodejs-reliability#45

Open

CI Relability 2020-07-23 JaneaSystems/nodejs-reliability#46

Open

tniessen force-pushed the crypto-ec-dont-create-invalid-state branch from 46040f1 to a8c7d72 Compare August 14, 2020 14:16

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2020

tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 14, 2020

tniessen closed this Aug 14, 2020

tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 14, 2020

AKJK-Internet-Megacorps approved these changes Aug 16, 2020

View reviewed changes

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

crypto: avoid unitializing ECDH objects on error#34302

crypto: avoid unitializing ECDH objects on error#34302
tniessen wants to merge 1 commit intonodejs:masterfrom
tniessen:crypto-ec-dont-create-invalid-state

tniessen commented Jul 10, 2020

Uh oh!

nodejs-github-bot commented Jul 17, 2020

Uh oh!

nodejs-github-bot commented Jul 20, 2020

Uh oh!

addaleax commented Jul 21, 2020

Uh oh!

tniessen commented Aug 14, 2020 •

edited

Loading

Uh oh!

nodejs-github-bot commented Aug 14, 2020

Uh oh!

tniessen commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

tniessen commented Jul 10, 2020

Checklist

Uh oh!

nodejs-github-bot commented Jul 17, 2020

Uh oh!

nodejs-github-bot commented Jul 20, 2020

Uh oh!

addaleax commented Jul 21, 2020

Uh oh!

tniessen commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 14, 2020

Uh oh!

tniessen commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tniessen commented Aug 14, 2020 •

edited

Loading