Use size of server key when selecting signature algorithm. #4389
Conversation
openssl-machine
added
the
hold: cla required
ssl/t1_lib.c
Outdated
| /* validate that key is large enough for the signature algorithm */ | ||
| const RSA *rsa = EVP_PKEY_get0_RSA(s->cert->pkeys[SSL_PKEY_RSA_PSS_SIGN].privatekey); | ||
| const EVP_MD *hash = EVP_get_digestbynid(lu->hash); | ||
| if ((NULL == rsa) |
There was a problem hiding this comment.
Our coding style says to leave a blank line after declarations. And we'd write the if test like this:
if (rsa == NULL
|| hash == NULL
|| RSA_size(rsa) < (2 * EVP_MD_size(hash) + 2))
Consider a new define RSA_PSS_KEY_BIG_ENOUGH or something, that captures that last less-than test.
There was a problem hiding this comment.
I'd be happy to add a define or function for the check. Where is the best location for such a function?
ssl/t1_lib.c
Outdated
| @@ -2379,6 +2387,15 @@ int tls_choose_sigalg(SSL *s, int *al) | |||
| } else if (lu->sig_idx != s->cert->key - s->cert->pkeys) { | |||
| continue; | |||
| } | |||
| if (lu->sig == EVP_PKEY_RSA_PSS) { | |||
| /* validate that key is large enough for the signature algorithm */ | |||
| const RSA *rsa = EVP_PKEY_get0_RSA(s->cert->pkeys[SSL_PKEY_RSA_PSS_SIGN].privatekey); | |||
There was a problem hiding this comment.
and, of course, same comment as above. Maybe even make a function that takes rsa and hash as parameters.
|
Also, you will have to sign our CLA; this is not a trivial change. Please see https://www.openssl.org/policies/cla.html for details. |
|
you can put it up near the top of the same file |
|
As indicated in the original issue we shouldn't hit this unless the key size is quite small anyway. If we care about that then do not use EVP_get_digestbynid(): that's quite an expensive operation to perform multiple times. Use the hash_idx field instead like this ssl_md(lu->hash_idx). I agree that bypassing the whole check if the key is big enough is a good idea too. |
|
Now I see the ssl_md() function (and tls1_lookup_md()). I'll use that instead of EVP_get_digestbynid(). I agree 1024 bit keys are small and should not be used. However, I think this is worthwhile to fix because I can get two different behaviors depending on how the client orders the same set of signature algorithms. Using s_client with the -sigalgs option to connect to s_server running with a 1024 bit key I see the following Works: -sigalgs "RSA-PSS+SHA256:RSA+SHA256:RSA-PSS+SHA384:RSA+SHA384:RSA-PSS+SHA512:RSA+SHA512" Since it works in one case, I would expect it to work in both. |
|
Close/open to kick the CLA bot. |
openssl-machine
added
the
hold: cla required
|
@nrobbin you need to edit the commit to set the Author properly. Perhaps best to squash and fix the author in the new single commit? |
openssl-machine
removed
the
hold: cla required
|
The code in the pull request is no longer working. It looks like the definition of SSL_PKEY_RSA_PSS_SIGN changed. I'll re-work against the latest code. |
… or SSL_PKEY_RSA). Extract the RSA key using EVP_PKEY_get0. Type is checked externally to be either EVP_PKEY_RSA_PSS or EVP_PKEY_RSA.
levitte
added
branch: master
Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4389)
… or SSL_PKEY_RSA). Extract the RSA key using EVP_PKEY_get0. Type is checked externally to be either EVP_PKEY_RSA_PSS or EVP_PKEY_RSA. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4389)
|
Merged. Thanks for all your efforts to get this code usable by OpenSSL. Nice security/usability feature! |
The server should not pick a signature algorithm that is incompatible with its configured key if the client offers a compatible algorithm of lower preference.
Fixes #4042