Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Oct 20, 2023
Merged

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

merged 1 commit into from
Oct 20, 2023

Conversation

panva
Copy link
Member

@panva panva commented Oct 18, 2023

@panva panva added crypto Issues and PRs related to the crypto subsystem. webcrypto labels Oct 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot 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 panva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023
@panva panva mentioned this pull request Oct 18, 2023
Open
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 18, 2023

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
Copy link
Member Author

panva commented Oct 18, 2023

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
Copy link
Contributor

Uzlopak commented Oct 18, 2023

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

@github-actions github-actions bot mentioned this pull request Oct 19, 2023
Open
25 tasks
@panva
Copy link
Member Author

panva commented Oct 19, 2023

cc @nodejs/crypto

tniessen
tniessen reviewed Oct 19, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 github-actions bot mentioned this pull request Oct 20, 2023
Open
20 tasks
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 20, 2023
@nodejs-github-bot nodejs-github-bot merged commit cd6b86b into nodejs:main Oct 20, 2023
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in cd6b86b

@panva panva deleted the webcrypto-key-check branch October 20, 2023 16:29
@panva panva added lts-watch-v18.x PRs that may need to be released in 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
Open
Open
Open
targos pushed a commit that referenced this pull request Oct 23, 2023
@panva @targos
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 targos mentioned this pull request Oct 23, 2023
Merged
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed lts-watch-v18.x PRs that may need to be released in v18.x. labels Oct 28, 2023
targos pushed a commit that referenced this pull request Oct 28, 2023
@panva @targos
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>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@panva @alexfernandez
crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey
20886cc 
PR-URL: nodejs#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
@panva @targos
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 targos mentioned this pull request Nov 12, 2023
Merged
@targos 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 targos mentioned this pull request Nov 28, 2023
Merged
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
Merged
codebytere added a commit to electron/electron that referenced this pull request Dec 8, 2023
@codebytere
crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey
1790982 
nodejs/node#50234
codebytere added a commit to electron/electron that referenced this pull request Dec 11, 2023
@electron-roller @ckerr @codebytere
chore: bump node to v20.10.0 (main) (#40675)
890a557 
* 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>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
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
@sercher
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@Uzlopak Uzlopak Uzlopak approved these changes

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. backported-to-v20.x PRs backported to the v20.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. webcrypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@panva @nodejs-github-bot @Uzlopak @aduh95 @tniessen @targos