diff options
author | Emilia Kasper <emilia@openssl.org> | 2014-11-19 17:01:36 +0100 |
---|---|---|
committer | Emilia Kasper <emilia@openssl.org> | 2014-11-20 15:17:36 +0100 |
commit | e5f261df7369a8d1734045ed59e12b42142a9147 (patch) | |
tree | 7a149a3254d47240c1de4424e913f7ad4dd10fca | |
parent | 9baee0216fe3bf572435a867963bdeea8ad95b59 (diff) |
Ensure SSL3_FLAGS_CCS_OK (or d1->change_cipher_spec_ok for DTLS) is reset
once the ChangeCipherSpec message is received. Previously, the server would
set the flag once at SSL3_ST_SR_CERT_VRFY and again at SSL3_ST_SR_FINISHED.
This would allow a second CCS to arrive and would corrupt the server state.
(Because the first CCS would latch the correct keys and subsequent CCS
messages would have to be encrypted, a MitM attacker cannot exploit this,
though.)
Thanks to Joeri de Ruiter for reporting this issue.
Reviewed-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit e94a6c0ede623960728415b68650a595e48f5a43)
-rw-r--r-- | CHANGES | 10 | ||||
-rw-r--r-- | ssl/d1_clnt.c | 5 | ||||
-rw-r--r-- | ssl/d1_srvr.c | 26 | ||||
-rw-r--r-- | ssl/dtls1.h | 4 | ||||
-rw-r--r-- | ssl/s3_clnt.c | 10 | ||||
-rw-r--r-- | ssl/s3_srvr.c | 40 | ||||
-rw-r--r-- | ssl/ssl3.h | 13 | ||||
-rw-r--r-- | ssl/t1_lib.c | 2 |
8 files changed, 90 insertions, 20 deletions
@@ -43,6 +43,11 @@ (CVE-2014-3566) [Adam Langley, Bodo Moeller] + *) Tighten handling of the ChangeCipherSpec (CCS) message: reject + early CCS messages during renegotiation. (Note that because + renegotiation is encrypted, this early CCS was not exploitable.) + [Emilia Käsper] + *) Tighten client-side session ticket handling during renegotiation: ensure that the client only accepts a session ticket if the server sends the extension anew in the ServerHello. Previously, a TLS client would @@ -376,6 +381,11 @@ Changes between 1.0.1j and 1.0.1k [xx XXX xxxx] + *) Tighten handling of the ChangeCipherSpec (CCS) message: reject + early CCS messages during renegotiation. (Note that because + renegotiation is encrypted, this early CCS was not exploitable.) + [Emilia Käsper] + *) Tighten client-side session ticket handling during renegotiation: ensure that the client only accepts a session ticket if the server sends the extension anew in the ServerHello. Previously, a TLS client would diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c index 171d144586..ea36ea448a 100644 --- a/ssl/d1_clnt.c +++ b/ssl/d1_clnt.c @@ -267,6 +267,9 @@ int dtls1_connect(SSL *s) memset(s->s3->client_random,0,sizeof(s->s3->client_random)); s->d1->send_cookie = 0; s->hit = 0; + s->d1->change_cipher_spec_ok = 0; + /* Should have been reset by ssl3_get_finished, too. */ + s->s3->change_cipher_spec = 0; break; #ifndef OPENSSL_NO_SCTP @@ -510,7 +513,6 @@ int dtls1_connect(SSL *s) else #endif s->state=SSL3_ST_CW_CHANGE_A; - s->s3->change_cipher_spec=0; } s->init_num=0; @@ -531,7 +533,6 @@ int dtls1_connect(SSL *s) #endif s->state=SSL3_ST_CW_CHANGE_A; s->init_num=0; - s->s3->change_cipher_spec=0; break; case SSL3_ST_CW_CHANGE_A: diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c index a6e7d053ca..084118375f 100644 --- a/ssl/d1_srvr.c +++ b/ssl/d1_srvr.c @@ -264,6 +264,9 @@ int dtls1_accept(SSL *s) } s->init_num=0; + s->d1->change_cipher_spec_ok = 0; + /* Should have been reset by ssl3_get_finished, too. */ + s->s3->change_cipher_spec = 0; if (s->state != SSL_ST_RENEGOTIATE) { @@ -694,8 +697,14 @@ int dtls1_accept(SSL *s) case SSL3_ST_SR_CERT_VRFY_A: case SSL3_ST_SR_CERT_VRFY_B: - - s->d1->change_cipher_spec_ok = 1; + /* + * This *should* be the first time we enable CCS, but be + * extra careful about surrounding code changes. We need + * to set this here because we don't know if we're + * expecting a CertificateVerify or not. + */ + if (!s->s3->change_cipher_spec) + s->d1->change_cipher_spec_ok = 1; /* we should decide if we expected this one */ ret=ssl3_get_cert_verify(s); if (ret <= 0) goto end; @@ -711,7 +720,18 @@ int dtls1_accept(SSL *s) case SSL3_ST_SR_FINISHED_A: case SSL3_ST_SR_FINISHED_B: - s->d1->change_cipher_spec_ok = 1; + /* + * Enable CCS for resumed handshakes. + * In a full handshake, we end up here through + * SSL3_ST_SR_CERT_VRFY_B, so change_cipher_spec_ok was + * already set. Receiving a CCS clears the flag, so make + * sure not to re-enable it to ban duplicates. + * s->s3->change_cipher_spec is set when a CCS is + * processed in d1_pkt.c, and remains set until + * the client's Finished message is read. + */ + if (!s->s3->change_cipher_spec) + s->d1->change_cipher_spec_ok = 1; ret=ssl3_get_finished(s,SSL3_ST_SR_FINISHED_A, SSL3_ST_SR_FINISHED_B); if (ret <= 0) goto end; diff --git a/ssl/dtls1.h b/ssl/dtls1.h index 5cb79f1dac..af86f60fb5 100644 --- a/ssl/dtls1.h +++ b/ssl/dtls1.h @@ -256,6 +256,10 @@ typedef struct dtls1_state_st unsigned int handshake_fragment_len; unsigned int retransmitting; + /* + * Set when the handshake is ready to process peer's ChangeCipherSpec message. + * Cleared after the message has been processed. + */ unsigned int change_cipher_spec_ok; #ifndef OPENSSL_NO_SCTP diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index f8d76781f2..c9b5ee1831 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -273,6 +273,9 @@ int ssl3_connect(SSL *s) s->state=SSL3_ST_CW_CLNT_HELLO_A; s->ctx->stats.sess_connect++; s->init_num=0; + s->s3->flags &= ~SSL3_FLAGS_CCS_OK; + /* Should have been reset by ssl3_get_finished, too. */ + s->s3->change_cipher_spec = 0; break; case SSL3_ST_CW_CLNT_HELLO_A: @@ -421,12 +424,10 @@ int ssl3_connect(SSL *s) else { s->state=SSL3_ST_CW_CHANGE_A; - s->s3->change_cipher_spec=0; } if (s->s3->flags & TLS1_FLAGS_SKIP_CERT_VERIFY) { s->state=SSL3_ST_CW_CHANGE_A; - s->s3->change_cipher_spec=0; } s->init_num=0; @@ -438,7 +439,6 @@ int ssl3_connect(SSL *s) if (ret <= 0) goto end; s->state=SSL3_ST_CW_CHANGE_A; s->init_num=0; - s->s3->change_cipher_spec=0; break; case SSL3_ST_CW_CHANGE_A: @@ -498,7 +498,6 @@ int ssl3_connect(SSL *s) s->method->ssl3_enc->client_finished_label, s->method->ssl3_enc->client_finished_label_len); if (ret <= 0) goto end; - s->s3->flags |= SSL3_FLAGS_CCS_OK; s->state=SSL3_ST_CW_FLUSH; /* clear flags */ @@ -547,7 +546,6 @@ int ssl3_connect(SSL *s) case SSL3_ST_CR_FINISHED_A: case SSL3_ST_CR_FINISHED_B: - s->s3->flags |= SSL3_FLAGS_CCS_OK; ret=ssl3_get_finished(s,SSL3_ST_CR_FINISHED_A, SSL3_ST_CR_FINISHED_B); @@ -986,7 +984,6 @@ int ssl3_get_server_hello(SSL *s) s->session->cipher = pref_cipher ? pref_cipher : ssl_get_cipher_by_char(s, p+j); s->hit = 1; - s->s3->flags |= SSL3_FLAGS_CCS_OK; } } #endif /* OPENSSL_NO_TLSEXT */ @@ -1002,7 +999,6 @@ int ssl3_get_server_hello(SSL *s) SSLerr(SSL_F_SSL3_GET_SERVER_HELLO,SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT); goto f_err; } - s->s3->flags |= SSL3_FLAGS_CCS_OK; s->hit=1; } /* a miss or crap from the other end */ diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 4914838847..6f82d3ceb4 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -301,6 +301,9 @@ int ssl3_accept(SSL *s) s->init_num=0; s->s3->flags &= ~SSL3_FLAGS_SGC_RESTART_DONE; s->s3->flags &= ~TLS1_FLAGS_SKIP_CERT_VERIFY; + s->s3->flags &= ~SSL3_FLAGS_CCS_OK; + /* Should have been reset by ssl3_get_finished, too. */ + s->s3->change_cipher_spec = 0; if (s->state != SSL_ST_RENEGOTIATE) { @@ -676,8 +679,14 @@ int ssl3_accept(SSL *s) case SSL3_ST_SR_CERT_VRFY_A: case SSL3_ST_SR_CERT_VRFY_B: - - s->s3->flags |= SSL3_FLAGS_CCS_OK; + /* + * This *should* be the first time we enable CCS, but be + * extra careful about surrounding code changes. We need + * to set this here because we don't know if we're + * expecting a CertificateVerify or not. + */ + if (!s->s3->change_cipher_spec) + s->s3->flags |= SSL3_FLAGS_CCS_OK; /* we should decide if we expected this one */ ret=ssl3_get_cert_verify(s); if (ret <= 0) goto end; @@ -696,6 +705,19 @@ int ssl3_accept(SSL *s) #if !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_NEXTPROTONEG) case SSL3_ST_SR_NEXT_PROTO_A: case SSL3_ST_SR_NEXT_PROTO_B: + /* + * Enable CCS for resumed handshakes with NPN. + * In a full handshake with NPN, we end up here through + * SSL3_ST_SR_CERT_VRFY_B, where SSL3_FLAGS_CCS_OK was + * already set. Receiving a CCS clears the flag, so make + * sure not to re-enable it to ban duplicates. + * s->s3->change_cipher_spec is set when a CCS is + * processed in s3_pkt.c, and remains set until + * the client's Finished message is read. + */ + if (!s->s3->change_cipher_spec) + s->s3->flags |= SSL3_FLAGS_CCS_OK; + ret=ssl3_get_next_proto(s); if (ret <= 0) goto end; s->init_num = 0; @@ -705,7 +727,18 @@ int ssl3_accept(SSL *s) case SSL3_ST_SR_FINISHED_A: case SSL3_ST_SR_FINISHED_B: - s->s3->flags |= SSL3_FLAGS_CCS_OK; + /* + * Enable CCS for resumed handshakes without NPN. + * In a full handshake, we end up here through + * SSL3_ST_SR_CERT_VRFY_B, where SSL3_FLAGS_CCS_OK was + * already set. Receiving a CCS clears the flag, so make + * sure not to re-enable it to ban duplicates. + * s->s3->change_cipher_spec is set when a CCS is + * processed in s3_pkt.c, and remains set until + * the client's Finished message is read. + */ + if (!s->s3->change_cipher_spec) + s->s3->flags |= SSL3_FLAGS_CCS_OK; ret=ssl3_get_finished(s,SSL3_ST_SR_FINISHED_A, SSL3_ST_SR_FINISHED_B); if (ret <= 0) goto end; @@ -777,7 +810,6 @@ int ssl3_accept(SSL *s) #else if (s->s3->next_proto_neg_seen) { - s->s3->flags |= SSL3_FLAGS_CCS_OK; s->s3->tmp.next_state=SSL3_ST_SR_NEXT_PROTO_A; } else diff --git a/ssl/ssl3.h b/ssl/ssl3.h index 274e6773c8..36320ffed0 100644 --- a/ssl/ssl3.h +++ b/ssl/ssl3.h @@ -429,8 +429,12 @@ typedef struct ssl3_buffer_st #define TLS1_FLAGS_TLS_PADDING_BUG 0x0008 #define TLS1_FLAGS_SKIP_CERT_VERIFY 0x0010 #define TLS1_FLAGS_KEEP_HANDSHAKE 0x0020 +/* + * Set when the handshake is ready to process peer's ChangeCipherSpec message. + * Cleared after the message has been processed. + */ #define SSL3_FLAGS_CCS_OK 0x0080 - + /* SSL3_FLAGS_SGC_RESTART_DONE is set when we * restart a handshake because of MS SGC and so prevents us * from restarting the handshake in a loop. It's reset on a @@ -492,8 +496,11 @@ typedef struct ssl3_state_st * and freed and MD_CTX-es for all required digests are stored in * this array */ EVP_MD_CTX **handshake_dgst; - /* this is set whenerver we see a change_cipher_spec message - * come in when we are not looking for one */ + /* + * Set whenever an expected ChangeCipherSpec message is processed. + * Unset when the peer's Finished message is received. + * Unexpected ChangeCipherSpec messages trigger a fatal alert. + */ int change_cipher_spec; int warn_alert; diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 160ce7628a..8e802a2e3f 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -2560,7 +2560,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, unsigned char **p, unsigned char #ifndef OPENSSL_NO_NEXTPROTONEG s->s3->next_proto_neg_seen = 0; #endif - s->tlsext_ticket_expected = 0; + s->tlsext_ticket_expected = 0; if (s->s3->alpn_selected) { |