Fix weak digest in TLS 1.2 with SNI. #4577
Conversation
1ce95f1 was incomplete and did not handle the case when SSL_set_SSL_CTX was called from the cert_cb callback rather than the SNI callback. The consequence is any server using OpenSSL 1.0.2 and the cert_cb callback for SNI only ever signs a weak digest, SHA-1, even when connecting to clients which use secure ones. Fix this and add regression tests for both this and the original issue. Fixes openssl#4554.
| @@ -3189,6 +3190,9 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx) | |||
| ssl->cert->ciphers_rawlen = ocert->ciphers_rawlen; | |||
| ocert->ciphers_raw = NULL; | |||
| } | |||
| for (i = 0; i < SSL_PKEY_NUM; i++) { | |||
| ssl->cert->pkeys[i].digest = ocert->pkeys[i].digest; | |||
There was a problem hiding this comment.
I'm left wondering what should be happening to pkeys[i].valid_flags
There was a problem hiding this comment.
valid_flags is a function of the certificate, so if they've been computed at this point, you have worse problems. (Though yeah the real problem is you all should not be storing handshake state on CERT.)
Looks like they get set fairly late, when you go to pick the cipher suite.
There was a problem hiding this comment.
Friendly ping.
(Since you are all doing a release soon, it seems good to get this in for that. Having OpenSSL servers out there which only sign SHA-1 is quite problematic for the ecosystem.)
There was a problem hiding this comment.
(Storing handshake state on CERT is a historical accident caused by all other structs being public.)
| @@ -1164,6 +1190,11 @@ int main(int argc, char *argv[]) | |||
| no_protocol = 1; | |||
| #endif | |||
| tls1 = 1; | |||
| } else if (strcmp(*argv, "-tls12") == 0) { | |||
| #ifdef OPENSSL_NO_TLS1 | |||
There was a problem hiding this comment.
shouldn't that be OPENSSL_NO_TLS1_2?
There was a problem hiding this comment.
This targets the 1.0.2 branch, where OPENSLS_NO_TLS1_2 doesn't exist.
There was a problem hiding this comment.
Ahh...right. In which case shouldn't this be removed altogether?
There was a problem hiding this comment.
I don't think so -- OPENSSL_NO_TLS1 means no TLS 1.0, and no TLS 1.2 (and no TLS 1.1, not that this program exercises that).
| @@ -3189,6 +3190,9 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx) | |||
| ssl->cert->ciphers_rawlen = ocert->ciphers_rawlen; | |||
| ocert->ciphers_raw = NULL; | |||
| } | |||
| for (i = 0; i < SSL_PKEY_NUM; i++) { | |||
| ssl->cert->pkeys[i].digest = ocert->pkeys[i].digest; | |||
There was a problem hiding this comment.
(Storing handshake state on CERT is a historical accident caused by all other structs being public.)
| int nid = NID_undef; | ||
|
|
||
| if (server_digest_expect == NULL) | ||
| return 0; |
There was a problem hiding this comment.
nit: the tristate return is never used, so you could just return 1 on success/no expectation, and 0 on failure?
There was a problem hiding this comment.
That would mean also changing the verify_* calls since the two calling conventions are inconsistent. (0 is a no-expectation-configured success, not failure, here.) It seemed weird to make it different from verify_{alpn,servername,ticket}. Thoughts?
There was a problem hiding this comment.
Yeah, I suspected you might have done it for consistency. Let's leave it alone then, in 1.1.* this test is deprecated anyway.
|
Pushed. Thanks. |
1ce95f1 was incomplete and did not handle the case when SSL_set_SSL_CTX was called from the cert_cb callback rather than the SNI callback. The consequence is any server using OpenSSL 1.0.2 and the cert_cb callback for SNI only ever signs a weak digest, SHA-1, even when connecting to clients which use secure ones. Fix this and add regression tests for both this and the original issue. Fixes #4554. Reviewed-by: Emilia Käsper <emilia@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #4577)
1ce95f1 was incomplete and did not handle the case when
SSL_set_SSL_CTXwas called from the cert_cb callback rather than the SNI callback. The consequence is any server using OpenSSL 1.0.2 and the cert_cb callback for SNI only ever signs a weak digest, SHA-1, even when connecting to clients which use secure ones.Fix this and add regression tests for both this and the original issue.
Fixes #4554.
Checklist