Skip to content

Comments

OCSP sign does not RSA_METHOD_FLAG_NO_CHECK#12087

Closed
ashman-p wants to merge 3 commits intoopenssl:OpenSSL_1_1_1-stablefrom
ashman-p:ocsp-rsa-no-check
Closed

OCSP sign does not RSA_METHOD_FLAG_NO_CHECK#12087
ashman-p wants to merge 3 commits intoopenssl:OpenSSL_1_1_1-stablefrom
ashman-p:ocsp-rsa-no-check

Conversation

@ashman-p
Copy link
Contributor

@ashman-p ashman-p commented Jun 8, 2020

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

Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 8, 2020
@ashman-p
Copy link
Contributor Author

ashman-p commented Jun 8, 2020

The is related to issue #12064

@ashman-p ashman-p mentioned this pull request Jun 8, 2020
Closed
@ashman-p
OCSP sign does not RSA_METHOD_FLAG_NO_CHECK
125076a 
OCSP_basic_sign_ctx() in ocsp_srv.c , and
OCSP_request_sign() in ocsp_cl.c does not check for RSA_METHOD_FLAG_NO_CHECK. If RSA_set_flags() was csalled to enable RSA_METHOD_FLAG_NO_CHECK, then OCSP sign operations can fail (because the X509_check_private_key()).

CLA: trivial
@ashman-p ashman-p force-pushed the ocsp-rsa-no-check branch from 2cd4928 to 125076a Compare June 8, 2020 20:01
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jun 8, 2020
@mattcaswell
Copy link
Member

mattcaswell commented Jun 9, 2020

It seems to me that the current handling of RSA_METHOD_FLAG_NO_CHECK is just totally wrong - and implemented at the wrong level of abstraction. It is a flag set on the RSA key which is only ever respected by libssl and gets checked 3 times - twice before calls to X509_check_private_key() and once before a call to EVP_PKEY_cmp(). That final instance seems to more-or-less replicate the logic inside X509_check_private_key() which ends up calling EVP_PKEY_cmp() to actually check the consistency of the public and private key.

So everything always ends up in EVP_PKEY_cmp(). For RSA keys this is implemented by rsa_pub_cmp which looks like this:

openssl/crypto/rsa/rsa_ameth.c

Lines 119 to 125 in 7f699cb

static int rsa_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b)
{
if (BN_cmp(b->pkey.rsa->n, a->pkey.rsa->n) != 0
|| BN_cmp(b->pkey.rsa->e, a->pkey.rsa->e) != 0)
return 0;
return 1;
}

This means that we will have to propagate this flag check everywhere where a consistency check is made to be sure we've got every instance.

A possible alternative approach would be to strip the RSA_METHOD_FLAG_NO_CHECK checks from everywhere, and instead move them to rsa_pub_cmp causing it to always return 1 if the flag is set. This will automatically ensure we do the right thing everywhere, and I can't see why it would break anything.

@kaduk
Copy link
Contributor

kaduk commented Jun 9, 2020

My (admittedly quite vague) recollection is that the RSA_METHOD_FLAG_NO_CHECK mechanism was supposed to be superseded by the RSA_FLAG_EXT_PKEY flag.

@ashman-p
Relocate checking RSA_METHOD_FLAG_NO_CHECK
a4afb6b 
Move RSA_METHOD_FLAG_NO_CHECK checks to rsa_pub_cmp(),  rsa_ameth.c.

CLA: trivial
@mattcaswell mattcaswell added the cla: trivial One of the commits is marked as 'CLA: trivial' label Jun 10, 2020
@mattcaswell
Copy link
Member

mattcaswell commented Jun 10, 2020

My (admittedly quite vague) recollection is that the RSA_METHOD_FLAG_NO_CHECK mechanism was supposed to be superseded by the RSA_FLAG_EXT_PKEY flag.

These flags seem related but different:

# define RSA_METHOD_FLAG_NO_CHECK        0x0001/* don't check pub/private
                                                * match */

Compared to

/*
 * This flag means the private key operations will be handled by rsa_mod_exp
 * and that they do not depend on the private key components being present:
 * for example a key stored in external hardware. Without this flag
 * bn_mod_exp gets called when private key components are absent.
 */
# define RSA_FLAG_EXT_PKEY               0x0020

@mattcaswell mattcaswell added branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) branch: master Applies to master branch labels Jun 10, 2020
mattcaswell
mattcaswell requested changes Jun 10, 2020
View reviewed changes
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.

I think this change goes beyond the definition of "CLA: trivial". Please could you submit a CLA?

https://www.openssl.org/policies/cla.html

@ashman-p
Copy link
Contributor Author

ashman-p commented Jun 10, 2020

I think this change goes beyond the definition of "CLA: trivial". Please could you submit a CLA?

https://www.openssl.org/policies/cla.html

I tagged it trivial mostly because i followed your guidance on the changes. I am working the CLA issue and happy to do that. Thanks.

@ashman-p
Update rsa_ameth.c
bca2112 
Updated per comment.
@ashman-p
Copy link
Contributor Author

ashman-p commented Jul 7, 2020

CLA and ICLA updated and emailed.

@ashman-p ashman-p requested a review from mattcaswell July 7, 2020 02:05
@ashman-p ashman-p mentioned this pull request Jul 10, 2020
Closed
2 tasks
@ashman-p ashman-p closed this Jul 10, 2020
@ashman-p
Copy link
Contributor Author

ashman-p commented Jul 10, 2020

Replaced by new updated PR
OCSP sign does not RSA_METHOD_FLAG_NO_CHECK 2 #12419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell Awaiting requested review from mattcaswell

Assignees

No one assigned

Labels

branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) cla: trivial One of the commits is marked as 'CLA: trivial'

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ashman-p @mattcaswell @kaduk @openssl-machine