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

Set maskHash when creating parameters. #3920

Closed
wants to merge 4 commits into from

Conversation

snhenson
Copy link
Contributor

@snhenson snhenson commented Jul 13, 2017

Checklist
  • tests are added or updated

This addresses #2963 when generating an RSA-PSS key we didn't set the cached mask parameter so if we signed using the key immediately it wouldn't use the correct PSS parameters.

@tomato42
Copy link
Contributor

using 6d3167a8075 it crashes when generating and signing a file with specified MGF1 MD:

valgrind apps/openssl req -x509 -newkey rsa-pss -keyout ca.key -out ca.crt 
-subj /CN=CA -nodes -batch -config /etc/pki/tls/openssl.cnf -pkeyopt rsa_keygen_bits:2048 
-pkeyopt rsa_pss_keygen_md:sha256 -pkeyopt rsa_pss_keygen_mgf1_md:sha1 -pkeyopt 
rsa_pss_keygen_saltlen:20 -sha256 -sigopt rsa_mgf1_md:sha1
==29957== Memcheck, a memory error detector
==29957== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==29957== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==29957== Command: apps/openssl req -x509 -newkey rsa-pss -keyout ca.key -out ca.crt -subj /CN=CA -nodes -batch -config /etc/pki/tls/openssl.cnf -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_pss_keygen_md:sha256 -pkeyopt rsa_pss_keygen_mgf1_md:sha1 -pkeyopt rsa_pss_keygen_saltlen:20 -sha256 -sigopt rsa_mgf1_md:sha1
==29957== 
Generating a 2048 bit RSA-PSS private key
....................................................+++
.................+++
==29957== Invalid read of size 8
==29957==    at 0x54CA94: rsa_mgf1_decode (rsa_ameth.c:213)
==29957==    by 0x54CA94: rsa_pss_params_create (rsa_ameth.c:570)
==29957==    by 0x551E76: rsa_set_pss_param (rsa_pmeth.c:678)
==29957==    by 0x551E76: pkey_rsa_keygen (rsa_pmeth.c:711)
==29957==    by 0x51C05E: EVP_PKEY_keygen (pmeth_gn.c:107)
==29957==    by 0x429380: req_main (req.c:520)
==29957==    by 0x4053C5: do_cmd (openssl.c:488)
==29957==    by 0x4059E2: main (openssl.c:167)
==29957==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==29957== 
==29957== 
==29957== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==29957==  Access not within mapped region at address 0x0
==29957==    at 0x54CA94: rsa_mgf1_decode (rsa_ameth.c:213)
==29957==    by 0x54CA94: rsa_pss_params_create (rsa_ameth.c:570)
==29957==    by 0x551E76: rsa_set_pss_param (rsa_pmeth.c:678)
==29957==    by 0x551E76: pkey_rsa_keygen (rsa_pmeth.c:711)
==29957==    by 0x51C05E: EVP_PKEY_keygen (pmeth_gn.c:107)
==29957==    by 0x429380: req_main (req.c:520)
==29957==    by 0x4053C5: do_cmd (openssl.c:488)
==29957==    by 0x4059E2: main (openssl.c:167)
==29957==  If you believe this happened as a result of a stack
==29957==  overflow in your program's main thread (unlikely but
==29957==  possible), you can try to increase the size of the
==29957==  main thread stack using the --main-stacksize= flag.
==29957==  The main thread stack size used in this run was 8388608.
==29957== 
==29957== HEAP SUMMARY:
==29957==     in use at exit: 91,804 bytes in 2,938 blocks
==29957==   total heap usage: 4,599 allocs, 1,661 frees, 267,809 bytes allocated
==29957== 
==29957== LEAK SUMMARY:
==29957==    definitely lost: 40 bytes in 1 blocks
==29957==    indirectly lost: 64 bytes in 1 blocks
==29957==      possibly lost: 0 bytes in 0 blocks
==29957==    still reachable: 91,700 bytes in 2,936 blocks
==29957==         suppressed: 0 bytes in 0 blocks
==29957== Rerun with --leak-check=full to see details of leaked memory
==29957== 
==29957== For counts of detected and suppressed errors, rerun with: -v
==29957== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)

@snhenson
Copy link
Contributor Author

New version pushed.

@tomato42
Copy link
Contributor

I'm now getting

parameter error "rsa_mgf1_md:sha1"
67538048:error:0408F098:rsa routines:pkey_rsa_ctrl:mgf1 digest not allowed:crypto/rsa/rsa_pmeth.c:509:

for the same command. Is that really correct?

@tomato42
Copy link
Contributor

tomato42 commented Jul 13, 2017

At the same time

openssl req -x509 -newkey rsa-pss -keyout ca.key -out ca.crt -subj /CN=CA 
-nodes -batch -config /etc/pki/tls/openssl.cnf -pkeyopt rsa_keygen_bits:2048 
-pkeyopt rsa_pss_keygen_md:sha256 -pkeyopt rsa_pss_keygen_mgf1_md:sha512 
-pkeyopt rsa_pss_keygen_saltlen:64 -sigopt rsa_mgf1_md:sha256 -sha256

is successful and generates a key/certificate with sha512 as the MGF MD and sha256 as the signature MD.

Both using 40876f13

@snhenson
Copy link
Contributor Author

No that's a separate bug. Should be fixed in new commit.

@tomato42
Copy link
Contributor

looks good, can't cause it to misbehave any more

two nits regarding documentation though:

  • -pkeyopt is not listed at the beginning of the req man page (SYNOPSIS)
  • -sigopt is missing completely from req man page; the dgst man page does not refer to pkeyutl man page with the list of available options to -sigopt

@dot-asm dot-asm added the approval: done This pull request has the required number of approvals label Jul 14, 2017
@snhenson
Copy link
Contributor Author

Update: I'm going to add a few more commits to this. There weren't any tests to check the two fixes and to check one case (key generation caching bug) needs an extension of evp_test which I'm working on.

@snhenson
Copy link
Contributor Author

Update: added keygen support to evp_test and tests to cover the fixes.

@snhenson
Copy link
Contributor Author

Please reconfirm +1, well if you agree that is ;-)

@richsalz
Copy link
Contributor

+1 reconfirm

@snhenson snhenson closed this Jul 19, 2017
levitte pushed a commit that referenced this pull request Jul 19, 2017
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #3920)
levitte pushed a commit that referenced this pull request Jul 19, 2017
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #3920)
levitte pushed a commit that referenced this pull request Jul 19, 2017
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #3920)
levitte pushed a commit that referenced this pull request Jul 19, 2017
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #3920)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants