crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey by panva · Pull Request #50234 · nodejs/node

panva · 2023-10-18T07:00:12Z

As per

Using OpenSSL's EVP_PKEY_check*

nodejs-github-bot · 2023-10-18T07:00:19Z

Review requested:

@nodejs/crypto

nodejs-github-bot · 2023-10-18T07:13:10Z

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

nodejs-github-bot · 2023-10-18T11:55:16Z

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

nodejs-github-bot · 2023-10-18T13:17:18Z

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

nodejs-github-bot · 2023-10-18T14:35:27Z

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

Uzlopak · 2023-10-18T16:09:23Z

Actually such a thing is a critical security issue, as there are actually existing attacks on elliptic curves when using a point which is not on the elliptic curves. You could do twist attacks and creating smaller subgroups to determine the secret key.

https://cryptodeeptech.ru/twist-attack/

When running a Twist Attack , the “private key” can be obtained by a certain choice of the “public key” (selected point of the secp256k1 elliptic curve), that is, the value in the transaction is revealed.After that, information about the private key will also be revealed, but for this you need to perform several ECC operations.

@nodejs/security

panva · 2023-10-18T16:14:11Z

Actually such a thing is a critical security issue, as there are actually existing attacks on elliptic curves when using a point which is not on the elliptic curves. You could do twist attacks and creating smaller subgroups to determine the secret key.

cryptodeeptech.ru/twist-attack

When running a Twist Attack , the “private key” can be obtained by a certain choice of the “public key” (selected point of the secp256k1 elliptic curve), that is, the value in the transaction is revealed.After that, information about the private key will also be revealed, but for this you need to perform several ECC operations.

@nodejs/security

I am not certain there is an issue since the actual use of the keys created this way will result in a) ECDSA verify false b) ECDH failing. I believe this check is merely a fail-fast mechanism.

Uzlopak · 2023-10-18T16:18:51Z

It was important to talk about it. If openssl throws anyway, we are on the safe side.

panva · 2023-10-19T08:01:53Z

cc @nodejs/crypto

tniessen

Nice work! Is there any chance you might add WPTs for this?

src/crypto/crypto_keys.cc

nodejs-github-bot · 2023-10-20T06:36:37Z

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

PR-URL: #50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs/node#50234

* chore: bump node in DEPS to v20.10.0 * chore: update feat_initialize_asar_support.patch no code changes; patch just needed an update due to nearby upstream changes Xref: nodejs/node#49986 * chore: update pass_all_globals_through_require.patch no manual changes; patch applied with fuzz Xref: nodejs/node#49657 * chore: update refactor_allow_embedder_overriding_of_internal_fs_calls Xref: nodejs/node#49912 no code changes; patch just needed an update due to nearby upstream changes * chore: update chore_allow_the_node_entrypoint_to_be_a_builtin_module.patch Xref: nodejs/node#49986 minor manual changes needed to sync with upstream change * update fix_expose_the_built-in_electron_module_via_the_esm_loader.patch Xref: nodejs/node#50096 Xref: nodejs/node#50314 in lib/internal/modules/esm/load.js, update the code that checks for `format === 'electron'`. I'd like 👀 on this Xref: nodejs/node#49657 add braces in lib/internal/modules/esm/translators.js to sync with upstream * fix: lazyload fs in esm loaders to apply asar patches * nodejs/node#50127 * nodejs/node#50096 * esm: jsdoc for modules code nodejs/node#49523 * test: set test-cli-node-options as flaky nodejs/node#50296 * deps: update c-ares to 1.20.1 nodejs/node#50082 * esm: bypass CommonJS loader under --default-type=module nodejs/node#49986 * deps: update uvwasi to 0.0.19 nodejs/node#49908 * lib,test: do not hardcode Buffer.kMaxLength nodejs/node#49876 * crypto: account for disabled SharedArrayBuffer nodejs/node#50034 * test: fix edge snapshot stack traces nodejs/node#49659 * src: generate snapshot with --predictable nodejs/node#48749 * chore: fixup patch indices * fs: throw errors from sync branches instead of separate implementations nodejs/node#49913 * crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey nodejs/node#50234 * esm: detect ESM syntax in ambiguous JavaScrip nodejs/node#50096 * fixup! test: fix edge snapshot stack traces * esm: unflag extensionless ES module JavaScript and Wasm in module scope nodejs/node#49974 * [tagged-ptr] Arrowify objects https://chromium-review.googlesource.com/c/v8/v8/+/4705331 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Charles Kerr <charles@charleskerr.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

PR-URL: nodejs/node#50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs/node#50234

* chore: bump Node.js to v22.9.0 * build: drop base64 dep in GN build nodejs/node#52856 * build,tools: make addons tests work with GN nodejs/node#50737 * fs: add fast api for InternalModuleStat nodejs/node#51344 * src: move package_json_reader cache to c++ nodejs/node#50322 * crypto: disable PKCS#1 padding for privateDecrypt nodejs-private/node-private#525 * src: move more crypto code to ncrypto nodejs/node#54320 * crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey nodejs/node#50234 * src: shift more crypto impl details to ncrypto nodejs/node#54028 * src: switch crypto APIs to use Maybe<void> nodejs/node#54775 * crypto: remove DEFAULT_ENCODING nodejs/node#47182 * deps: update libuv to 1.47.0 nodejs/node#50650 * build: fix conflict gyp configs nodejs/node#53605 * lib,src: drop --experimental-network-imports nodejs/node#53822 * esm: align sync and async load implementations nodejs/node#49152 * esm: remove unnecessary toNamespacedPath calls nodejs/node#53656 * module: detect ESM syntax by trying to recompile as SourceTextModule nodejs/node#52413 * test: adapt debugger tests to V8 11.4 nodejs/node#49639 * lib: update usage of always on Atomics API nodejs/node#49639 * test: adapt test-fs-write to V8 internal changes nodejs/node#49639 * test: adapt to new V8 trusted memory spaces nodejs/node#50115 * deps: update libuv to 1.47.0 nodejs/node#50650 * src: use non-deprecated v8::Uint8Array::kMaxLength nodejs/node#50115 * src: update default V8 platform to override functions with location nodejs/node#51362 * src: add missing TryCatch nodejs/node#51362 * lib,test: handle new Iterator global nodejs/node#51362 * src: use non-deprecated version of CreateSyntheticModule nodejs/node#50115 * src: remove calls to recently deprecated V8 APIs nodejs/node#52996 * src: use new V8 API to define stream accessor nodejs/node#53084 * src: do not use deprecated V8 API nodejs/node#53084 * src: do not use soon-to-be-deprecated V8 API nodejs/node#53174 * src: migrate to new V8 interceptors API nodejs/node#52745 * src: use supported API to get stalled TLA messages nodejs/node#51362 * module: print location of unsettled top-level await in entry points nodejs/node#51999 * test: make snapshot comparison more flexible nodejs/node#54375 * test: do not set concurrency on parallelized runs nodejs/node#52177 * src: move FromNamespacedPath to path.cc nodejs/node#53540 * test: adapt to new V8 trusted memory spaces nodejs/node#50115 * build: add option to enable clang-cl on Windows nodejs/node#52870 * chore: fixup patch indices * chore: add/remove changed files * esm: drop support for import assertions nodejs/node#54890 * build: compile with C++20 support nodejs/node#52838 * deps: update nghttp2 to 1.62.1 nodejs/node#52966 * src: parse inspector profiles with simdjson nodejs/node#51783 * build: add GN build files nodejs/node#47637 * deps,lib,src: add experimental web storage nodejs/node#52435 * build: add missing BoringSSL dep * src: rewrite task runner in c++ nodejs/node#52609 * fixup! build: add GN build files * src: stop using deprecated fields of v8::FastApiCallbackOptions nodejs/node#54077 * fix: shadow variable * build: add back incorrectly removed SetAccessor patch * fixup! fixup! build: add GN build files * crypto: fix integer comparison in crypto for BoringSSL * src,lib: reducing C++ calls of esm legacy main resolve nodejs/node#48325 * src: move more crypto_dh.cc code to ncrypto nodejs/node#54459 * chore: fixup GN files for previous commit * src: move more crypto code to ncrypto nodejs/node#54320 * Fixup Perfetto ifdef guards * fix: missing electron_natives dep * fix: node_use_node_platform = false * fix: include src/node_snapshot_stub.cc in libnode * 5507047: [import-attributes] Remove support for import assertions https://chromium-review.googlesource.com/c/v8/v8/+/5507047 * fix: restore v8-sandbox.h in filenames.json * fix: re-add original-fs generation logic * fix: ngtcp2 openssl dep * test: try removing NAPI_VERSION undef * chore(deps): bump @types/node * src: move more crypto_dh.cc code to ncrypto nodejs/node#54459 * esm: remove unnecessary toNamespacedPath calls nodejs/node#53656 * buffer: fix out of range for toString nodejs/node#54553 * lib: rewrite AsyncLocalStorage without async_hooks nodejs/node#48528 * module: print amount of load time of a cjs module nodejs/node#52213 * test: skip reproducible snapshot test on 32-bit nodejs/node#53592 * fixup! src: move more crypto_dh.cc code to ncrypto * test: adjust emittedUntil return type * chore: remove redundant wpt streams patch * fixup! chore(deps): bump @types/node * fix: gn executable name on Windows * fix: build on Windows * fix: rename conflicting win32 symbols in //third_party/sqlite On Windows otherwise we get: lld-link: error: duplicate symbol: sqlite3_win32_write_debug >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:47987 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_sleep >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48042 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_is_nt >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48113 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_unicode >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48470 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_unicode_to_utf8 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48486 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_mbcs_to_utf8 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48502 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_mbcs_to_utf8_v2 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48518 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_mbcs >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48534 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_mbcs_v2 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48550 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj * docs: remove unnecessary ts-expect-error after types bump * src: move package resolver to c++ nodejs/node#50322 * build: set ASAN detect_container_overflow=0 nodejs/node#55584 * chore: fixup rebase * test: disable failing ASAN test * win: almost fix race detecting ESRCH in uv_kill libuv/libuv#4341

panva added crypto Issues and PRs related to the crypto subsystem. webcrypto labels Oct 18, 2023

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 18, 2023

panva force-pushed the webcrypto-key-check branch from 692c8ff to be33e09 Compare October 18, 2023 07:03

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023

panva requested review from jasnell and tniessen October 18, 2023 07:09

panva mentioned this pull request Oct 18, 2023

Unspecified importKey behavior for PKCS8-encoded raw private keys w3c/webcrypto#356

Open

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023

panva force-pushed the webcrypto-key-check branch from be33e09 to 57324bb Compare October 18, 2023 11:45

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023

github-actions bot mentioned this pull request Oct 19, 2023

CI Reliability 2023-10-19 nodejs/reliability#691

Open

25 tasks

tniessen reviewed Oct 19, 2023

View reviewed changes

src/crypto/crypto_keys.cc Show resolved Hide resolved

src/crypto/crypto_keys.cc Outdated Show resolved Hide resolved

src/crypto/crypto_keys.cc Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Oct 20, 2023

CI Reliability 2023-10-20 nodejs/reliability#692

Open

20 tasks

panva force-pushed the webcrypto-key-check branch from 57324bb to 1e7c9ca Compare October 20, 2023 06:26

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2023

panva deleted the webcrypto-key-check branch October 20, 2023 16:29

panva added lts-watch-v18.x lts-watch-v20.x PRs that may need to be released in v20.x labels Oct 20, 2023

This was referenced Oct 21, 2023

CI Reliability 2023-10-21 nodejs/reliability#693

Open

CI Reliability 2023-10-22 nodejs/reliability#694

Open

CI Reliability 2023-10-23 nodejs/reliability#695

Open

targos pushed a commit that referenced this pull request Oct 23, 2023

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

a362c27

PR-URL: #50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos mentioned this pull request Oct 23, 2023

v21.1.0 release proposal #50335

Merged

targos added backported-to-v18.x and removed lts-watch-v18.x labels Oct 28, 2023

targos pushed a commit that referenced this pull request Oct 28, 2023

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

76e4d41

PR-URL: #50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos pushed a commit that referenced this pull request Nov 11, 2023

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

773320e

PR-URL: #50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

targos added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed lts-watch-v20.x PRs that may need to be released in v20.x labels Nov 12, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

codebytere added a commit to electron/electron that referenced this pull request Dec 8, 2023

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

1790982

nodejs/node#50234

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024

crypto: ensure valid point on elliptic curve in

17876b4

PR-URL: nodejs/node#50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024

crypto: ensure valid point on elliptic curve in

2208b07

PR-URL: nodejs/node#50234 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

codebytere added a commit to electron/electron that referenced this pull request Oct 16, 2024

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

2030560

nodejs/node#50234

codebytere added a commit to electron/electron that referenced this pull request Oct 21, 2024

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

20e5fb1

nodejs/node#50234

codebytere added a commit to electron/electron that referenced this pull request Oct 21, 2024

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

d7f29b5

nodejs/node#50234

codebytere added a commit to electron/electron that referenced this pull request Oct 22, 2024

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

918b1f9

nodejs/node#50234

codebytere added a commit to electron/electron that referenced this pull request Oct 28, 2024

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

e02401d

nodejs/node#50234

codebytere added a commit to electron/electron that referenced this pull request Oct 29, 2024

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

3bfd27e

nodejs/node#50234

codebytere added a commit to electron/electron that referenced this pull request Oct 31, 2024

crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

6ae97bf

nodejs/node#50234

Uh oh!

Conversation

panva commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

Uzlopak commented Oct 18, 2023

Uh oh!

panva commented Oct 18, 2023

Uh oh!

Uzlopak commented Oct 18, 2023

Uh oh!

panva commented Oct 19, 2023

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants