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

OCSP sign does not RSA_METHOD_FLAG_NO_CHECK 2 #12419

Closed

Conversation

@ashman-p
Copy link
Contributor

@ashman-p ashman-p commented Jul 10, 2020

OCSP_basic_sign_ctx() in ocsp_srv.c , does not check for RSA_METHOD_FLAG_NO_CHECK. If RSA_set_flags() to enable RSA_METHOD_FLAG_NO_CHECK, then OCSP sign operations can fail because the X509_check_private_key().

After discussions with OpenSSL the check was moved to crypto/rsa/rsa_ameth.c as a common place to check. Checks in ssl_rsa.c were removed.

Fixes #12087

Checklist
  • documentation is added or updated
  • tests are added or updated
OCSP_basic_sign_ctx() in ocsp_srv.c , does not check for RSA_METHOD_FLAG_NO_CHECK. If RSA_set_flags() to enable RSA_METHOD_FLAG_NO_CHECK, then OCSP sign operations can fail because the X509_check_private_key().

After discussions with OpenSSL the check was moved to crypto/rsa/rsa_ameth.c as a common place to check. Checks in ssl_rsa.c were removed.
Copy link
Member

@mattcaswell mattcaswell left a comment

LGTM. Needs second review.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 20, 2020

Travis failures are not relevant.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Aug 20, 2020

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Sep 20, 2020

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

@t-j-h
t-j-h approved these changes Sep 20, 2020
@openssl-machine
Copy link

@openssl-machine openssl-machine commented Sep 21, 2020

This pull request is ready to merge

@t8m
t8m approved these changes Sep 21, 2020
openssl-machine pushed a commit that referenced this pull request Sep 21, 2020
OCSP_basic_sign_ctx() in ocsp_srv.c , does not check for RSA_METHOD_FLAG_NO_CHECK.
If a key has RSA_METHOD_FLAG_NO_CHECK set, OCSP sign operations can fail
because the X509_check_private_key() can fail.

The check for the RSA_METHOD_FLAG_NO_CHECK was moved to crypto/rsa/rsa_ameth.c
as a common place to check. Checks in ssl_rsa.c were removed.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12419)
openssl-machine pushed a commit that referenced this pull request Sep 21, 2020
OCSP_basic_sign_ctx() in ocsp_srv.c , does not check for RSA_METHOD_FLAG_NO_CHECK.
If a key has RSA_METHOD_FLAG_NO_CHECK set, OCSP sign operations can fail
because the X509_check_private_key() can fail.

The check for the RSA_METHOD_FLAG_NO_CHECK was moved to crypto/rsa/rsa_ameth.c
as a common place to check. Checks in ssl_rsa.c were removed.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12419)

(cherry picked from commit 56e8fe0)
@t8m
Copy link
Member

@t8m t8m commented Sep 21, 2020

I've amended the commit message slightly to clarify and merged to both master and 1.1.1 branches.

Thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants