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

Jump to bottom

openssl: only verify RSA private key if supported #1904

Closed
wants to merge 1 commit into from
Closed

openssl: only verify RSA private key if supported #1904

wants to merge 1 commit into from

Conversation

dirkfeytons
Copy link
Contributor

@dirkfeytons dirkfeytons commented Sep 21, 2017 •
edited

In some cases the RSA key does not support verifying it because it's
located on a smart card, an engine wants to hide it, ...
Check the flags on the key before trying to verify it.
OpenSSL does the same thing internally; see ssl/ssl_rsa.c

The patch works for OpenSSL 1.0.1, 1.0.2 and 1.1.0.

@bagder bagder added the TLS label Sep 21, 2017
@bagder
Copy link
Member

bagder commented Sep 21, 2017 •
edited

Thanks, but needs a little polish. This was caught by the CI:

vtls/openssl.c: In function ‘cert_stuff’:
vtls/openssl.c:843:44: error: comparison between pointer and integer [-Werror]
     if(EVP_PKEY_id(SSL_get_privatekey(ssl) == EVP_PKEY_RSA) {
                                            ^
vtls/openssl.c:843:5: error: passing argument 1 of ‘EVP_PKEY_id’ makes pointer from integer without a cast [-Werror]
     if(EVP_PKEY_id(SSL_get_privatekey(ssl) == EVP_PKEY_RSA) {
     ^
In file included from /usr/include/openssl/x509.h:73:0,
                 from /usr/include/openssl/ssl.h:156,
                 from vtls/openssl.c:54:
/usr/include/openssl/evp.h:896:6: note: expected ‘const struct EVP_PKEY *’ but argument is of type ‘int’
 int  EVP_PKEY_id(const EVP_PKEY *pkey);
      ^
vtls/openssl.c:843:61: error: expected ‘)’ before ‘{’ token
     if(EVP_PKEY_id(SSL_get_privatekey(ssl) == EVP_PKEY_RSA) {
                                                             ^
vtls/openssl.c:864:3: error: expected expression before ‘}’ token
   }
   ^
vtls/openssl.c:552:8: error: unused variable ‘check_privkey’ [-Werror=unused-variable]
   bool check_privkey = TRUE;
        ^
cc1: all warnings being treated as errors

@dirkfeytons
Copy link
Contributor Author

Ah crap.
That's what you get for making some last minute changes before going to bed and then continuing in the morning without actually testing the changes again...

@dirkfeytons
Copy link
Contributor Author

Updated the commit. I'm not happy about having to declare a EVP_PKEY * at the top of the (large) function just to use it briefly near the end. I've named it priv_key now but that causes a variable shadowing issue (also highlighted by CI).
What's your recommendation?

@bagder
Copy link
Member

bagder commented Sep 21, 2017

You can add your new code that checks the private key within an extra set of { braces }, as then you can declare your own priv_key there locally and avoid the shadowing, the distance to the declaration and remove one #ifdef.

@dirkfeytons
openssl: only verify RSA private key if supported
a7bb4c3 
In some cases the RSA key does not support verifying it because it's
located on a smart card, an engine wants to hide it, ...
Check the flags on the key before trying to verify it.
OpenSSL does the same thing internally; see ssl/ssl_rsa.c
@bagder bagder closed this in fa9482a Sep 21, 2017
@bagder
Copy link
Member

bagder commented Sep 21, 2017

Excellent, thank you!

@dirkfeytons
Copy link
Contributor Author

Thank you!

This was referenced Nov 14, 2017
Closed
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
TLS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dirkfeytons @bagder