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: reject Ed25519/Ed448 in Sign/Verify prototypes #52340

Merged
merged 1 commit into from
Apr 8, 2024
Merged

crypto: reject Ed25519/Ed448 in Sign/Verify prototypes #52340

merged 1 commit into from
Apr 8, 2024

Conversation

panva
Copy link
Member

@panva panva commented Apr 3, 2024 •
edited
Loading

It is possible to slip Ed25519/Ed448 keys to Sign.prototype.sign and Verify.prototype.sign given you provide a valid openssl digest and the result is an empty signature.

This PR checks for the key being a oneshot only key and throws a generic ERR_CRYPTO_UNSUPPORTED_OPERATION

fixes: #52097

@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++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 3, 2024
@panva panva changed the title crypto: reject Ed25519/Ed448 in crypto.sign and crypto.verify crypto: reject Ed25519/Ed448 in Sign/Verify prototypes Apr 3, 2024
@panva panva added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Apr 4, 2024

It looks like this change is consistently failing to build on win-vs2022-arm64 for some reason.

@panva
Copy link
Member Author

panva commented Apr 4, 2024 •
edited
Loading

It looks like this change is consistently failing to build on win-vs2022-arm64 for some reason.

fatal error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

Seems unrelated to me. If it happens to be related I'm not able to debug it and would appreciate help.

@panva
Copy link
Member Author

panva commented Apr 4, 2024

Let's see if rebasing does anything.

@tniessen
Copy link
Member

tniessen commented Apr 4, 2024 •
edited
Loading

The error was: fatal error C1060: compiler is out of heap space, so almost certainly unrelated. (Or a bug in MSVC.)

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

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

@panva
Copy link
Member Author

panva commented Apr 4, 2024

Well, nothing much I can do about fatal error C1060: compiler is out of heap space.

@aduh95
Copy link
Contributor

aduh95 commented Apr 4, 2024

/cc @nodejs/platform-windows-arm

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@StefanStojanovic
Copy link
Contributor

Well, nothing much I can do about fatal error C1060: compiler is out of heap space.

This error happens occasionally, but shouldn't happen as often as it did on this PR. One thing that comes to mind is that the V8 update landed recently in the main branch and that is something that could increase the frequency of this. I'll monitor ARM64 builds closely for the next few days to see if this is a rising concern.

@panva
Copy link
Member Author

panva commented Apr 5, 2024

@StefanStojanovic thank you for looking into it

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024
@nodejs-github-bot nodejs-github-bot merged commit 9f939f5 into nodejs:main Apr 8, 2024
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 9f939f5

@panva panva deleted the fix-oneshot-signer branch April 8, 2024 06:31
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@panva @marco-ippolito
crypto: reject Ed25519/Ed448 in Sign/Verify prototypes
c877bb2 
fixes: #52097
PR-URL: #52340
Fixes: #52097
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request May 2, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@panva @marco-ippolito
crypto: reject Ed25519/Ed448 in Sign/Verify prototypes
739958e 
fixes: #52097
PR-URL: #52340
Fixes: #52097
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@tniessen tniessen tniessen approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ed25519/Ed448 does not throw when used with invalid digest in crypto.createSign
6 participants
@panva @nodejs-github-bot @aduh95 @tniessen @StefanStojanovic @lpinca