Ensure SSL3_FLAGS_CCS_OK (or d1->change_cipher_spec_ok for DTLS) is reset
authorEmilia Kasper <emilia@openssl.org>
Wed, 19 Nov 2014 16:01:36 +0000 (17:01 +0100)
committerEmilia Kasper <emilia@openssl.org>
Thu, 20 Nov 2014 13:57:15 +0000 (14:57 +0100)
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>
CHANGES
ssl/d1_clnt.c
ssl/d1_srvr.c
ssl/dtls1.h
ssl/s3_clnt.c
ssl/s3_srvr.c
ssl/ssl3.h
ssl/t1_lib.c

diff --git a/CHANGES b/CHANGES
index 0d9bd50..aa07cc0 100644 (file)
--- a/CHANGES
+++ b/CHANGES
 
  Changes between 1.0.1j and 1.0.2 [xx XXX xxxx]
 
 
  Changes between 1.0.1j and 1.0.2 [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
    *) 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
 
  Changes between 1.0.1j and 1.0.1k [xx XXX xxxx]
 
 
  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
    *) 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
index 171d144..ea36ea4 100644 (file)
@@ -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;
                        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
                        break;
 
 #ifndef OPENSSL_NO_SCTP
@@ -510,7 +513,6 @@ int dtls1_connect(SSL *s)
                                else
 #endif
                                        s->state=SSL3_ST_CW_CHANGE_A;
                                else
 #endif
                                        s->state=SSL3_ST_CW_CHANGE_A;
-                               s->s3->change_cipher_spec=0;
                                }
 
                        s->init_num=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;
 #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:
                        break;
 
                case SSL3_ST_CW_CHANGE_A:
index 1d2201d..aef38bb 100644 (file)
@@ -264,6 +264,9 @@ int dtls1_accept(SSL *s)
                                }
 
                        s->init_num=0;
                                }
 
                        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)
                                {
 
                        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:
 
                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;
                        /* 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:
 
                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;
                        ret=ssl3_get_finished(s,SSL3_ST_SR_FINISHED_A,
                                SSL3_ST_SR_FINISHED_B);
                        if (ret <= 0) goto end;
index 5cb79f1..af86f60 100644 (file)
@@ -256,6 +256,10 @@ typedef struct dtls1_state_st
        unsigned int handshake_fragment_len;
 
        unsigned int retransmitting;
        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
        unsigned int change_cipher_spec_ok;
 
 #ifndef OPENSSL_NO_SCTP
index 64439c1..ee0493f 100644 (file)
@@ -280,6 +280,9 @@ int ssl3_connect(SSL *s)
                        s->state=SSL3_ST_CW_CLNT_HELLO_A;
                        s->ctx->stats.sess_connect++;
                        s->init_num=0;
                        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:
                        break;
 
                case SSL3_ST_CW_CLNT_HELLO_A:
@@ -428,12 +431,10 @@ int ssl3_connect(SSL *s)
                        else
                                {
                                s->state=SSL3_ST_CW_CHANGE_A;
                        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;
                                }
                        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;
                                }
 
                        s->init_num=0;
@@ -445,7 +446,6 @@ int ssl3_connect(SSL *s)
                        if (ret <= 0) goto end;
                        s->state=SSL3_ST_CW_CHANGE_A;
                        s->init_num=0;
                        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:
                        break;
 
                case SSL3_ST_CW_CHANGE_A:
@@ -505,7 +505,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->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 */
                        s->state=SSL3_ST_CW_FLUSH;
 
                        /* clear flags */
@@ -554,7 +553,6 @@ int ssl3_connect(SSL *s)
 
                case SSL3_ST_CR_FINISHED_A:
                case SSL3_ST_CR_FINISHED_B:
 
                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);
                        s->s3->flags |= SSL3_FLAGS_CCS_OK;
                        ret=ssl3_get_finished(s,SSL3_ST_CR_FINISHED_A,
                                SSL3_ST_CR_FINISHED_B);
@@ -992,7 +990,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->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 */
                        }
                }
 #endif /* OPENSSL_NO_TLSEXT */
@@ -1008,7 +1005,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;
                }
                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 */
            s->hit=1;
            }
        /* a miss or crap from the other end */
index 876a245..f79957f 100644 (file)
@@ -309,6 +309,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->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)
                                {
 
                        if (s->state != SSL_ST_RENEGOTIATE)
                                {
@@ -683,8 +686,14 @@ int ssl3_accept(SSL *s)
 
                case SSL3_ST_SR_CERT_VRFY_A:
                case SSL3_ST_SR_CERT_VRFY_B:
 
                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;
                        /* we should decide if we expected this one */
                        ret=ssl3_get_cert_verify(s);
                        if (ret <= 0) goto end;
@@ -703,6 +712,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:
 #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;
                        ret=ssl3_get_next_proto(s);
                        if (ret <= 0) goto end;
                        s->init_num = 0;
@@ -712,7 +734,18 @@ int ssl3_accept(SSL *s)
 
                case SSL3_ST_SR_FINISHED_A:
                case SSL3_ST_SR_FINISHED_B:
 
                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;
                        ret=ssl3_get_finished(s,SSL3_ST_SR_FINISHED_A,
                                SSL3_ST_SR_FINISHED_B);
                        if (ret <= 0) goto end;
@@ -784,7 +817,6 @@ int ssl3_accept(SSL *s)
 #else
                                if (s->s3->next_proto_neg_seen)
                                        {
 #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
                                        s->s3->tmp.next_state=SSL3_ST_SR_NEXT_PROTO_A;
                                        }
                                else
index 276a776..efff233 100644 (file)
@@ -433,8 +433,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
 #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
 #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
 /* 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
@@ -498,8 +502,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;
         * 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;
        int change_cipher_spec;
 
        int warn_alert;
index 86203f1..8b2b16b 100644 (file)
@@ -2504,7 +2504,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
 #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)
                {
 
        if (s->s3->alpn_selected)
                {