openssl: only verify RSA private key if supported #1904

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@dirkfeytons
Contributor

dirkfeytons commented Sep 21, 2017

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 SSL/TLS label Sep 21, 2017

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Sep 21, 2017

Member

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
Member

bagder commented Sep 21, 2017

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

This comment has been minimized.

Show comment Hide comment
@dirkfeytons

dirkfeytons Sep 21, 2017

Contributor

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...

Contributor

dirkfeytons commented Sep 21, 2017

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

This comment has been minimized.

Show comment Hide comment
@dirkfeytons

dirkfeytons Sep 21, 2017

Contributor

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?

Contributor

dirkfeytons commented Sep 21, 2017

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

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Sep 21, 2017

Member

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.

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.

openssl: only verify RSA private key if supported
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

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Sep 21, 2017

Member

Excellent, thank you!

Member

bagder commented Sep 21, 2017

Excellent, thank you!

@dirkfeytons

This comment has been minimized.

Show comment Hide comment
@dirkfeytons

dirkfeytons Sep 21, 2017

Contributor

Thank you!

Contributor

dirkfeytons commented Sep 21, 2017

Thank you!

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