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

x509_check_private_key doesn't do what it says #3973

Open
hannob opened this Issue Jul 20, 2017 · 12 comments

Comments

Projects
None yet
8 participants
@hannob
Contributor

hannob commented Jul 20, 2017

I noticed this function which seems to be a bit odd:
https://www.openssl.org/docs/manmaster/man3/X509_check_private_key.html

The description says
"check the consistency of a private key with the public key in an X509 certificate or certificate request"

Except that's not what the function is doing. If I understand it correctly it simply checks whether the public key parts of a private key match the public key part of a certificate. The effect is that one can easily forge a private key that "matches" a certificate based on the public key.

I'd say this is a security issue, yet it's a documented security issue. This is documented under the point "BUGS". I wonder how I should interpret that. It's a known bug, supposed to be fixed at some point?
The "BUGS" section also documents sideeffects of that behavior. (aka you can just use a public key and it will also be accepted)
So if this is fixed it may break existing applications. However I think it should still be fixed, as keeping a function with a known security limitation is worse.

Notably there are downstream exposures of that function that haven't documented this limitation, see e.g.:
http://php.net/manual/en/function.openssl-x509-check-private-key.php

@InfoHunter

This comment has been minimized.

Show comment
Hide comment
@InfoHunter

InfoHunter Jul 20, 2017

Contributor

That BUGS section is definitely I added long ago, I also found the problem of this function, but not sure if it worth a fix. Let's ping @openssl for comments.

Contributor

InfoHunter commented Jul 20, 2017

That BUGS section is definitely I added long ago, I also found the problem of this function, but not sure if it worth a fix. Let's ping @openssl for comments.

@richsalz

This comment has been minimized.

Show comment
Hide comment
@richsalz

richsalz Jul 20, 2017

Contributor

I think the manpage should be written so that the BUGS text is merged into the mainline.
If someone messes with the private key, then yeah, the routine will not be useful.
Are you going to open a PHP issue?

Contributor

richsalz commented Jul 20, 2017

I think the manpage should be written so that the BUGS text is merged into the mainline.
If someone messes with the private key, then yeah, the routine will not be useful.
Are you going to open a PHP issue?

@hannob

This comment has been minimized.

Show comment
Hide comment
@hannob

hannob Jul 20, 2017

Contributor

I think this isn't something that should be fixed by better documentation, but by fixing the function. "x509_check_private_key" implies to me that what this function does is to really check a private key and this very likely leads to misunderstandings if it doesn't. People tend to not read docs and I think APIs should stick to the least surprising behavior and also be secure by default if possible.

Contributor

hannob commented Jul 20, 2017

I think this isn't something that should be fixed by better documentation, but by fixing the function. "x509_check_private_key" implies to me that what this function does is to really check a private key and this very likely leads to misunderstandings if it doesn't. People tend to not read docs and I think APIs should stick to the least surprising behavior and also be secure by default if possible.

@richsalz

This comment has been minimized.

Show comment
Hide comment
@richsalz

richsalz Jul 20, 2017

Contributor
Contributor

richsalz commented Jul 20, 2017

@hannob

This comment has been minimized.

Show comment
Hide comment
@hannob

hannob Jul 20, 2017

Contributor

I think it should check the consistency of the private key, like the commandline option "openssl rsa -in [key] -check" would do, and return an error for a defect private key.

Contributor

hannob commented Jul 20, 2017

I think it should check the consistency of the private key, like the commandline option "openssl rsa -in [key] -check" would do, and return an error for a defect private key.

@sthen

This comment has been minimized.

Show comment
Hide comment
@sthen

sthen Jul 21, 2017

This is documented under BUGS in master, but in the release branches the *check_private_key functions are only documented under SSL_CTX_use_certificate() which doesn't mention this at all. So it's no surprise if downstream exposures of this don't include the warning.

sthen commented Jul 21, 2017

This is documented under BUGS in master, but in the release branches the *check_private_key functions are only documented under SSL_CTX_use_certificate() which doesn't mention this at all. So it's no surprise if downstream exposures of this don't include the warning.

@richsalz

This comment has been minimized.

Show comment
Hide comment
@richsalz

richsalz Jul 21, 2017

Contributor

Thanks, and yes any fixes to the doc for this function should be backported to all (soon to be both:) releases.

Contributor

richsalz commented Jul 21, 2017

Thanks, and yes any fixes to the doc for this function should be backported to all (soon to be both:) releases.

@dankohn

This comment has been minimized.

Show comment
Hide comment
@dankohn

dankohn Jul 22, 2017

For context, @hannob described his actions in uncovering this bug in a blog post and Symantec responded and agreed to change their procedures. [Updated after @hannob pointed out that I had the wrong link.]

dankohn commented Jul 22, 2017

For context, @hannob described his actions in uncovering this bug in a blog post and Symantec responded and agreed to change their procedures. [Updated after @hannob pointed out that I had the wrong link.]

@hannob

This comment has been minimized.

Show comment
Hide comment
@hannob

hannob Jul 23, 2017

Contributor

@dankohn you have linked symantec's blogpost twice and not mine :-) (and symantec hasn't linked back to me...)
Here: https://blog.hboeck.de/archives/888-How-I-tricked-Symantec-with-a-Fake-Private-Key.html

Contributor

hannob commented Jul 23, 2017

@dankohn you have linked symantec's blogpost twice and not mine :-) (and symantec hasn't linked back to me...)
Here: https://blog.hboeck.de/archives/888-How-I-tricked-Symantec-with-a-Fake-Private-Key.html

InfoHunter added a commit to InfoHunter/openssl that referenced this issue Jul 24, 2017

Backport X509_check_private_key.pod
to address #3973, and original PR to master branch is #3614

test case in the original PR is not applied.

levitte pushed a commit that referenced this issue Jul 27, 2017

Backport X509_check_private_key.pod
to address #3973, and original PR to master branch is #3614

test case in the original PR is not applied.

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #4002)
@AnnieYousar

This comment has been minimized.

Show comment
Hide comment
@AnnieYousar

AnnieYousar Jul 31, 2017

x509_check-private_key overlooks also weak keys. Not speaking on small key sizes, it does not recognize the silly exponent 1. Check as e.g. the key

-----BEGIN RSA PRIVATE KEY-----
MIICWQIBAAKBgQDI0rl3Jrbj+43Uki58yYHxfcnZPIfaesoY4eFlH5ClIfTKq4iE
oXmzTPxlBeanR16DZYldeT0iW/c4+k6HrqA3zh2sMrgGapv/sGaaBexMkTOyPgdN
6VggDw6A9Immtc6CFXiB+RMlxTyvoGUB4HSUHVG8HFVxDb4xhoEYV/O59QJ9ARxu
kLZ3/5exYd2x06yOYJiuoKDhysp6GaLdWnuKj4DAcjpRhMrHE0B1Uc5rvSpsZVRY
4/CFwdWBCc+sKfi1gL1tH5VGgnlV+IrvNgCEtVjOywrB3cYAFa2TOyivhoOSt9vr
hDDYZrwYGr+8RsgM18peHrpHHFWPgxRsopcCgYEAyNK5dgpIU0UV1Pp9GuvQHdE7
eKPZOdnoThdnS3yzSqZqOyrIEmcoLoI1UcVxVXjyxjsc+CTkPmtxdyTNfd70DA+H
d2zGeKG3ibUEvw6TF8Lq1cftVCTP9Q0OYQHdervHfoy7XaCtEyNcnDgTSUMZSvnV
e/OnQVUqrH+Riy0Ro5cCQQDsdBeptEbGiATD9g8/AoRXWzHDKsMyh0dGuGnGDYTZ
lM41qdQNMH2w0d5HMHjA04SgDFd1QPOb/trqlP6LDm47AkEA2WycXtAnbRXuvhXT
LWd6dmj1WiBrhz8F9Uegfwtr98Kuz4CYb0oQwDXOZQklHMt9u2MQ6Q8dYk2NxssL
i2cFjwIBAQIBAQJBAKbfxOMBBzkoZtxIDidtFngqI50kLRYtS9dK9wlqfaOSaPhy
C+g1iF2UOqw18dwDTj9qQlBYWYXR1sKjwAZ6Mk8=
-----END RSA PRIVATE KEY-----

with the command
openssl rsa -check
Note that neither e nor d are exactly 1, but the key is nevertheless weak, because they are 1 modulo Euler's totient function. In the text view dp=dq=1 reveals the weakness.
/Ann.

x509_check-private_key overlooks also weak keys. Not speaking on small key sizes, it does not recognize the silly exponent 1. Check as e.g. the key

-----BEGIN RSA PRIVATE KEY-----
MIICWQIBAAKBgQDI0rl3Jrbj+43Uki58yYHxfcnZPIfaesoY4eFlH5ClIfTKq4iE
oXmzTPxlBeanR16DZYldeT0iW/c4+k6HrqA3zh2sMrgGapv/sGaaBexMkTOyPgdN
6VggDw6A9Immtc6CFXiB+RMlxTyvoGUB4HSUHVG8HFVxDb4xhoEYV/O59QJ9ARxu
kLZ3/5exYd2x06yOYJiuoKDhysp6GaLdWnuKj4DAcjpRhMrHE0B1Uc5rvSpsZVRY
4/CFwdWBCc+sKfi1gL1tH5VGgnlV+IrvNgCEtVjOywrB3cYAFa2TOyivhoOSt9vr
hDDYZrwYGr+8RsgM18peHrpHHFWPgxRsopcCgYEAyNK5dgpIU0UV1Pp9GuvQHdE7
eKPZOdnoThdnS3yzSqZqOyrIEmcoLoI1UcVxVXjyxjsc+CTkPmtxdyTNfd70DA+H
d2zGeKG3ibUEvw6TF8Lq1cftVCTP9Q0OYQHdervHfoy7XaCtEyNcnDgTSUMZSvnV
e/OnQVUqrH+Riy0Ro5cCQQDsdBeptEbGiATD9g8/AoRXWzHDKsMyh0dGuGnGDYTZ
lM41qdQNMH2w0d5HMHjA04SgDFd1QPOb/trqlP6LDm47AkEA2WycXtAnbRXuvhXT
LWd6dmj1WiBrhz8F9Uegfwtr98Kuz4CYb0oQwDXOZQklHMt9u2MQ6Q8dYk2NxssL
i2cFjwIBAQIBAQJBAKbfxOMBBzkoZtxIDidtFngqI50kLRYtS9dK9wlqfaOSaPhy
C+g1iF2UOqw18dwDTj9qQlBYWYXR1sKjwAZ6Mk8=
-----END RSA PRIVATE KEY-----

with the command
openssl rsa -check
Note that neither e nor d are exactly 1, but the key is nevertheless weak, because they are 1 modulo Euler's totient function. In the text view dp=dq=1 reveals the weakness.
/Ann.

@snhenson

This comment has been minimized.

Show comment
Hide comment
@snhenson

snhenson Jul 31, 2017

Contributor

Since X509_check_private() just checks the public part of the private key matches the certificate the private key can contain anything in its other components and it will match.

If you want to check the private key is valid as well then that's trickier. You can't in general assume that the private key components are present: the key might come from a HSM for example.

You could perform operations such as sign/verify but that only works if the key algorithm (and any associated HSM) supports it. If it is, for example, an X.25519 key you can't do that.

One way round this is to have functions like EVP_PKEY_private_ckeck() and EVP_PKEY_public_check() (maybe one for parameters too?) which call appropriate functions in EVP_PKEY_METHOD for the algorithm.

Contributor

snhenson commented Jul 31, 2017

Since X509_check_private() just checks the public part of the private key matches the certificate the private key can contain anything in its other components and it will match.

If you want to check the private key is valid as well then that's trickier. You can't in general assume that the private key components are present: the key might come from a HSM for example.

You could perform operations such as sign/verify but that only works if the key algorithm (and any associated HSM) supports it. If it is, for example, an X.25519 key you can't do that.

One way round this is to have functions like EVP_PKEY_private_ckeck() and EVP_PKEY_public_check() (maybe one for parameters too?) which call appropriate functions in EVP_PKEY_METHOD for the algorithm.

@mattcaswell mattcaswell added this to the 1.1.1 milestone Jan 22, 2018

@richsalz richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

@mattcaswell

This comment has been minimized.

Show comment
Hide comment
@mattcaswell

mattcaswell Jul 2, 2018

Member

Not a 1.1.1 specific issue so removing that milestone.

Member

mattcaswell commented Jul 2, 2018

Not a 1.1.1 specific issue so removing that milestone.

@mattcaswell mattcaswell modified the milestones: 1.1.1, Assessed Jul 2, 2018

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