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: fix RSA-PSS default saltLength #39999

Closed
wants to merge 1 commit into from
Closed

crypto: fix RSA-PSS default saltLength #39999

wants to merge 1 commit into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 5, 2021

Based on my understanding of RFC 8017, when hashAlgorithm is set but saltLength is not, the value of saltLength associated with the key pair should default to the digest size of hashAlgorithm, not to 0.

I am not sure why OpenSSL uses 0. I suspect it is because we don't call EVP_PKEY_CTX_set_rsa_pss_keygen_saltlen and 0 is the least restrictive value, at least within OpenSSL. This behavior can still be restored by explicitly setting saltLength to 0.

I'd argue that this is a bug fix. If we are concerned about semverity, I could modify this PR to only affect the new options (#39927) and keep the behavior of the old options intact. Personally, I don't think it's necessary to go that route.

@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 Sep 5, 2021
jasnell
jasnell approved these changes Sep 5, 2021
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

+1 to treating this as a bug fix

@tniessen tniessen requested a review from panva September 5, 2021 18:42
@nodejs-github-bot
Copy link
Collaborator

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

@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Sep 5, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@panva
Copy link
Member

panva commented Sep 7, 2021

@tniessen something's up with openssl3 and the first of the two tests > see here.

tniessen added a commit to tniessen/node that referenced this pull request Sep 7, 2021
@tniessen
crypto: fix default MGF1 hash for OpenSSL 3
2fd4b39 
Refs: nodejs#39999
@tniessen tniessen mentioned this pull request Sep 7, 2021
Closed
@tniessen
Copy link
Member Author

tniessen commented Sep 7, 2021

Thanks @panva. I believe it's a difference in behavior between OpenSSL 1.1.1 and OpenSSL 3. It seems to be unrelated to this change and there weren't any tests for this case as far as I can tell. I attempted a fix in #40031.

@tniessen tniessen added the blocked PRs that are blocked by other issues or PRs. label Sep 7, 2021
panva pushed a commit that referenced this pull request Sep 9, 2021
@tniessen @panva
crypto: fix default MGF1 hash for OpenSSL 3
5fd7a72 
Refs: #39999

PR-URL: #40031
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

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

@panva panva removed the blocked PRs that are blocked by other issues or PRs. label Sep 9, 2021
@tniessen tniessen force-pushed the crypto-fix-rsa-pss-keygen-default-saltlength branch from d6dbc3c to db358bf Compare September 9, 2021 15:39
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@panva
Copy link
Member

panva commented Sep 9, 2021

Landed in a42bd7e

@panva panva closed this Sep 9, 2021
panva pushed a commit that referenced this pull request Sep 9, 2021
@tniessen @panva
crypto: fix RSA-PSS default saltLength
a42bd7e 
PR-URL: #39999
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@tniessen
Copy link
Member Author

Thanks for reviewing everyone.

This was referenced Sep 14, 2021
Open
Open
This was referenced Sep 16, 2021
Open
Open
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@tniessen @BethGriggs
crypto: fix default MGF1 hash for OpenSSL 3
fc45cbe 
Refs: #39999

PR-URL: #40031
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@tniessen @BethGriggs
crypto: fix RSA-PSS default saltLength
d657ae6 
PR-URL: #39999
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
@tniessen tniessen deleted the crypto-fix-rsa-pss-keygen-default-saltlength branch October 7, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@panva panva panva 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tniessen @nodejs-github-bot @panva @jasnell @cjihrig