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.

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

Fix a crash or unbounded allocation in RSA_padding_add_PKCS1_PSS_mgf1 #2801

Conversation

bernd-edlinger
Copy link
Member

and RSA_verify_PKCS1_PSS_mgf1 with 512-bit RSA vs. sha-512.

@guidovranken: here is how I would like to fix the pss signature functions.
I hope this makes it clear what I meant with the comments on #2699.

I have test cases but don't know how to integrate that in the test framework.

openssl genrsa -out rsa.pem 512
openssl dgst -sign rsa.pem -sha512  -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-3 -sigopt rsa_mgf1_md:sha512 < /dev/null  > sig
=>crash
openssl dgst -sign rsa.pem -sha512  -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:2147483647 -sigopt rsa_mgf1_md:sha1 < /dev/null  > sig
=>allocates 2GB and crashes if that succeeds

for the problem in RSA_padding_verify I need first a data block
that can be decrypted by the RSA key and clear text ends with 0xBC.

openssl dgst -sign rsa.pem -sha1  -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-3 -sigopt rsa_mgf1_md:sha512 < /dev/null  > sig
=> prepares test data, does not crash
openssl dgst -prverify rsa.pem -sha512  -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-3 -sigopt rsa_mgf1_md:sha512 < /dev/null  -signature sig
=> tries to allocate -1, which fails silently but is flagged by asan

@bernd-edlinger bernd-edlinger force-pushed the fix_crash_in_RSA_padding_add_PKCS1_PSS_mgf1 branch from 41f282b to 93e60f4 Compare March 3, 2017 19:55
@bernd-edlinger
Copy link
Member Author

rebased.

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

LGTM. What branches is this for?

@bernd-edlinger
Copy link
Member Author

This is for master, I need to make an extra PRs for 1.1.0 and 1.0.2.
In 1.1.0 we do not have sLen = -3, so the test case can not be used as it is.
Porting the test case to 1.0.2 will probably not be worth the effort.

PS: Should I squash the two commits?

@mattcaswell mattcaswell added the branch: master Merge to master branch label Mar 8, 2017
@mattcaswell
Copy link
Member

No need to squash the commits IMO.

@dot-asm dot-asm added the approval: done This pull request has the required number of approvals label Mar 13, 2017
@dot-asm
Copy link
Contributor

dot-asm commented Mar 13, 2017

Applied to master. Thanks!

levitte pushed a commit that referenced this pull request Mar 13, 2017
and RSA_verify_PKCS1_PSS_mgf1 with 512-bit RSA vs. sha-512.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from #2801)
levitte pushed a commit that referenced this pull request Mar 13, 2017
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from #2801)
@dot-asm dot-asm closed this Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants