Move DTLS CCS processing into the state machine
authorMatt Caswell <matt@openssl.org>
Tue, 2 Jun 2015 10:33:07 +0000 (11:33 +0100)
committerMatt Caswell <matt@openssl.org>
Mon, 3 Aug 2015 10:18:05 +0000 (11:18 +0100)
Continuing on from the previous commit this moves the processing of DTLS
CCS messages out of the record layer and into the state machine.

Reviewed-by: Tim Hudson <tjh@openssl.org>
ssl/d1_both.c
ssl/d1_clnt.c
ssl/d1_srvr.c
ssl/record/rec_layer_d1.c
ssl/s3_both.c
ssl/ssl_locl.h

index a1499da..e097de4 100644 (file)
@@ -160,8 +160,8 @@ static void dtls1_set_message_header_int(SSL *s, unsigned char mt,
                                          unsigned short seq_num,
                                          unsigned long frag_off,
                                          unsigned long frag_len);
-static long dtls1_get_message_fragment(SSL *s, int st1, int stn, long max,
-                                       int *ok);
+static long dtls1_get_message_fragment(SSL *s, int st1, int stn, int mt,
+                                       long max, int *ok);
 
 static hm_fragment *dtls1_hm_fragment_new(unsigned long frag_len,
                                           int reassembly)
@@ -470,7 +470,7 @@ long dtls1_get_message(SSL *s, int st1, int stn, int mt, long max, int *ok)
     memset(msg_hdr, 0, sizeof(*msg_hdr));
 
  again:
-    i = dtls1_get_message_fragment(s, st1, stn, max, ok);
+    i = dtls1_get_message_fragment(s, st1, stn, mt, max, ok);
     if (i == DTLS1_HM_BAD_FRAGMENT || i == DTLS1_HM_FRAGMENT_RETRY) {
         /* bad fragment received */
         goto again;
@@ -485,6 +485,20 @@ long dtls1_get_message(SSL *s, int st1, int stn, int mt, long max, int *ok)
     }
 
     p = (unsigned char *)s->init_buf->data;
+
+    if (mt == SSL3_MT_CHANGE_CIPHER_SPEC) {
+        if (s->msg_callback) {
+            s->msg_callback(0, s->version, SSL3_RT_CHANGE_CIPHER_SPEC,
+                            p, 1, s, s->msg_callback_arg);
+        }
+        /*
+         * This isn't a real handshake message so skip the processing below.
+         * dtls1_get_message_fragment() will never return a CCS if mt == -1,
+         * so we are ok to continue in that case.
+         */
+        return i;
+    }
+
     msg_len = msg_hdr->msg_len;
 
     /* reconstruct message header */
@@ -835,11 +849,11 @@ dtls1_process_out_of_seq_message(SSL *s, const struct hm_header_st *msg_hdr,
 }
 
 static long
-dtls1_get_message_fragment(SSL *s, int st1, int stn, long max, int *ok)
+dtls1_get_message_fragment(SSL *s, int st1, int stn, int mt, long max, int *ok)
 {
     unsigned char wire[DTLS1_HM_HEADER_LENGTH];
     unsigned long len, frag_off, frag_len;
-    int i, al;
+    int i, al, recvd_type;
     struct hm_header_st msg_hdr;
 
  redo:
@@ -851,13 +865,46 @@ dtls1_get_message_fragment(SSL *s, int st1, int stn, long max, int *ok)
     }
 
     /* read handshake message header */
-    i = s->method->ssl_read_bytes(s, SSL3_RT_HANDSHAKE, NULL, wire,
+    i = s->method->ssl_read_bytes(s, SSL3_RT_HANDSHAKE, &recvd_type, wire,
                                   DTLS1_HM_HEADER_LENGTH, 0);
     if (i <= 0) {               /* nbio, or an error */
         s->rwstate = SSL_READING;
         *ok = 0;
         return i;
     }
+    if(recvd_type == SSL3_RT_CHANGE_CIPHER_SPEC) {
+        /* This isn't a real handshake message - its a CCS.
+         * There is no message sequence number in a CCS to give us confidence
+         * that this was really intended to be at this point in the handshake
+         * sequence. Therefore we only allow this if we were explicitly looking
+         * for it (i.e. if |mt| is -1 we still don't allow it).
+         */
+        if(mt == SSL3_MT_CHANGE_CIPHER_SPEC) {
+            if (wire[0] != SSL3_MT_CCS) {
+                al = SSL_AD_UNEXPECTED_MESSAGE;
+                SSLerr(SSL_F_SSL3_GET_MESSAGE, SSL_R_BAD_CHANGE_CIPHER_SPEC);
+                goto f_err;
+            }
+
+            memcpy(s->init_buf->data, wire, i);
+            s->init_num = i - 1;
+            s->init_msg = s->init_buf->data + 1;
+            s->s3->tmp.message_type = SSL3_MT_CHANGE_CIPHER_SPEC;
+            s->s3->tmp.message_size = i - 1;
+            s->state = stn;
+            *ok = 1;
+            return i-1;
+        } else {
+            /*
+             * We weren't expecting a CCS yet. Probably something got
+             * re-ordered or this is a retransmit. We should drop this and try
+             * again.
+             */
+            s->init_num = 0;
+            goto redo;
+        }
+    }
+
     /* Handshake fails if message header is incomplete */
     if (i != DTLS1_HM_HEADER_LENGTH) {
         al = SSL_AD_UNEXPECTED_MESSAGE;
index fde0def..566c154 100644 (file)
@@ -271,7 +271,6 @@ 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.
              */
@@ -376,7 +375,7 @@ int dtls1_connect(SSL *s)
                              sizeof(sctpauthkey), sctpauthkey);
 #endif
 
-                    s->state = SSL3_ST_CR_FINISHED_A;
+                    s->state = SSL3_ST_CR_CHANGE_A;
                 } else
                     s->state = DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A;
             }
@@ -628,7 +627,7 @@ int dtls1_connect(SSL *s)
                 if (s->tlsext_ticket_expected)
                     s->s3->tmp.next_state = SSL3_ST_CR_SESSION_TICKET_A;
                 else
-                    s->s3->tmp.next_state = SSL3_ST_CR_FINISHED_A;
+                    s->s3->tmp.next_state = SSL3_ST_CR_CHANGE_A;
             }
             s->init_num = 0;
             break;
@@ -638,7 +637,7 @@ int dtls1_connect(SSL *s)
             ret = ssl3_get_new_session_ticket(s);
             if (ret <= 0)
                 goto end;
-            s->state = SSL3_ST_CR_FINISHED_A;
+            s->state = SSL3_ST_CR_CHANGE_A;
             s->init_num = 0;
             break;
 
@@ -651,9 +650,19 @@ int dtls1_connect(SSL *s)
             s->init_num = 0;
             break;
 
+        case SSL3_ST_CR_CHANGE_A:
+        case SSL3_ST_CR_CHANGE_B:
+            ret = ssl3_get_change_cipher_spec(s, SSL3_ST_CR_CHANGE_A,
+                                              SSL3_ST_CR_CHANGE_B);
+            if (ret <= 0)
+                goto end;
+
+            s->state = SSL3_ST_CR_FINISHED_A;
+            s->init_num = 0;
+            break;
+
         case SSL3_ST_CR_FINISHED_A:
         case SSL3_ST_CR_FINISHED_B:
-            s->d1->change_cipher_spec_ok = 1;
             ret = ssl3_get_finished(s, SSL3_ST_CR_FINISHED_A,
                                     SSL3_ST_CR_FINISHED_B);
             if (ret <= 0)
index 7a40d66..19562e1 100644 (file)
@@ -257,7 +257,6 @@ 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.
              */
@@ -378,7 +377,7 @@ int dtls1_accept(SSL *s)
                 goto end;
             }
 
-            s->state = SSL3_ST_SR_FINISHED_A;
+            s->state = SSL3_ST_SR_CHANGE_A;
             break;
 
         case DTLS1_SCTP_ST_SW_WRITE_SOCK:
@@ -624,7 +623,7 @@ int dtls1_accept(SSL *s)
                  * pub key in a certificate, the CertificateVerify message is
                  * not sent.
                  */
-                s->state = SSL3_ST_SR_FINISHED_A;
+                s->state = SSL3_ST_SR_CHANGE_A;
                 s->init_num = 0;
             } else if (SSL_USE_SIGALGS(s)) {
                 s->state = SSL3_ST_SR_CERT_VRFY_A;
@@ -675,23 +674,23 @@ int dtls1_accept(SSL *s)
                 s->state = DTLS1_SCTP_ST_SR_READ_SOCK;
             else
 #endif
-                s->state = SSL3_ST_SR_FINISHED_A;
+                s->state = SSL3_ST_SR_CHANGE_A;
+            s->init_num = 0;
+            break;
+
+        case SSL3_ST_SR_CHANGE_A:
+        case SSL3_ST_SR_CHANGE_B:
+            ret = ssl3_get_change_cipher_spec(s, SSL3_ST_SR_CHANGE_A,
+                                              SSL3_ST_SR_CHANGE_B);
+            if (ret <= 0)
+                goto end;
+
+            s->state = SSL3_ST_SR_FINISHED_A;
             s->init_num = 0;
             break;
 
         case SSL3_ST_SR_FINISHED_A:
         case SSL3_ST_SR_FINISHED_B:
-            /*
-             * Enable CCS. Receiving a CCS clears the flag, so make
-             * sure not to re-enable it to ban duplicates. This *should* be the
-             * first time we have received one - but we check anyway to be
-             * cautious.
-             * 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)
@@ -779,7 +778,7 @@ int dtls1_accept(SSL *s)
                 goto end;
             s->state = SSL3_ST_SW_FLUSH;
             if (s->hit) {
-                s->s3->tmp.next_state = SSL3_ST_SR_FINISHED_A;
+                s->s3->tmp.next_state = SSL3_ST_SR_CHANGE_A;
 
 #ifndef OPENSSL_NO_SCTP
                 /*
index 2c8b94f..3da4f11 100644 (file)
@@ -379,8 +379,9 @@ int dtls1_process_buffered_records(SSL *s)
  * (possibly multiple records if we still don't have anything to return).
  *
  * This function must handle any surprises the peer may have for us, such as
- * Alert records (e.g. close_notify), ChangeCipherSpec records (not really
- * a surprise, but handled as if it were), or renegotiation requests.
+ * Alert records (e.g. close_notify) or renegotiation requests. ChangeCipherSpec
+ * messages are treated as if they were handshake messages *if* the |recd_type|
+ * argument is non NULL.
  * Also if record payloads contain fragments too small to process, we store
  * them until there is enough for the respective protocol (the record protocol
  * may use arbitrary fragmentation and even interleaving):
@@ -538,9 +539,14 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
         return (0);
     }
 
-    if (type == SSL3_RECORD_get_type(rr)) {
-        /* SSL3_RT_APPLICATION_DATA or
-         * SSL3_RT_HANDSHAKE */
+    if (type == SSL3_RECORD_get_type(rr)
+            || (SSL3_RECORD_get_type(rr) == SSL3_RT_CHANGE_CIPHER_SPEC
+                && type == SSL3_RT_HANDSHAKE && recvd_type != NULL)) {
+        /*
+         * SSL3_RT_APPLICATION_DATA or
+         * SSL3_RT_HANDSHAKE or
+         * SSL3_RT_CHANGE_CIPHER_SPEC
+         */
         /*
          * make sure that we are not getting application data when we are
          * doing a handshake for the first time
@@ -552,6 +558,9 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
             goto f_err;
         }
 
+        if (recvd_type != NULL)
+            *recvd_type = SSL3_RECORD_get_type(rr);
+
         if (len <= 0)
             return (len);
 
@@ -858,59 +867,11 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
     }
 
     if (SSL3_RECORD_get_type(rr) == SSL3_RT_CHANGE_CIPHER_SPEC) {
-        unsigned int ccs_hdr_len = DTLS1_CCS_HEADER_LENGTH;
-
-        if (s->version == DTLS1_BAD_VER)
-            ccs_hdr_len = 3;
-
-        /*
-         * 'Change Cipher Spec' is just a single byte, so we know exactly
-         * what the record payload has to look like
-         */
-        /* XDTLS: check that epoch is consistent */
-        if ((SSL3_RECORD_get_length(rr) != ccs_hdr_len)
-                || (SSL3_RECORD_get_off(rr) != 0)
-                || (SSL3_RECORD_get_data(rr)[0] != SSL3_MT_CCS)) {
-            i = SSL_AD_ILLEGAL_PARAMETER;
-            SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_BAD_CHANGE_CIPHER_SPEC);
-            goto err;
-        }
-
-        SSL3_RECORD_set_length(rr, 0);
-
-        if (s->msg_callback)
-            s->msg_callback(0, s->version, SSL3_RT_CHANGE_CIPHER_SPEC,
-                SSL3_RECORD_get_data(rr), 1, s, s->msg_callback_arg);
-
         /*
          * We can't process a CCS now, because previous handshake messages
          * are still missing, so just drop it.
          */
-        if (!s->d1->change_cipher_spec_ok) {
-            goto start;
-        }
-
-        s->d1->change_cipher_spec_ok = 0;
-
-        s->s3->change_cipher_spec = 1;
-        if (!ssl3_do_change_cipher_spec(s))
-            goto err;
-
-        /* do this whenever CCS is processed */
-        dtls1_reset_seq_numbers(s, SSL3_CC_READ);
-
-        if (s->version == DTLS1_BAD_VER)
-            s->d1->handshake_read_seq++;
-
-#ifndef OPENSSL_NO_SCTP
-        /*
-         * Remember that a CCS has been received, so that an old key of
-         * SCTP-Auth can be deleted when a CCS is sent. Will be ignored if no
-         * SCTP is used
-         */
-        BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_SCTP_AUTH_CCS_RCVD, 1, NULL);
-#endif
-
+        SSL3_RECORD_set_length(rr, 0);
         goto start;
     }
 
@@ -1026,7 +987,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
 
  f_err:
     ssl3_send_alert(s, SSL3_AL_FATAL, al);
- err:
     return (-1);
 }
 
index 32193c3..943cf73 100644 (file)
@@ -240,12 +240,23 @@ int ssl3_get_change_cipher_spec(SSL *s, int a, int b)
 
     /*
      * 'Change Cipher Spec' is just a single byte, which should already have
-     * been consumed by ssl_get_message() so there should be no bytes left
+     * been consumed by ssl_get_message() so there should be no bytes left,
+     * unless we're using DTLS1_BAD_VER, which has an extra 2 bytes
      */
-    if (n != 0) {
-        al = SSL_AD_ILLEGAL_PARAMETER;
-        SSLerr(SSL_F_SSL3_GET_CHANGE_CIPHER_SPEC, SSL_R_BAD_CHANGE_CIPHER_SPEC);
-        goto f_err;
+    if (SSL_IS_DTLS(s)) {
+        if ((s->version == DTLS1_BAD_VER && n != DTLS1_CCS_HEADER_LENGTH + 1)
+                    || (s->version != DTLS1_BAD_VER
+                        && n != DTLS1_CCS_HEADER_LENGTH - 1)) {
+                al = SSL_AD_ILLEGAL_PARAMETER;
+                SSLerr(SSL_F_SSL3_GET_CHANGE_CIPHER_SPEC, SSL_R_BAD_CHANGE_CIPHER_SPEC);
+                goto f_err;
+        }
+    } else {
+        if (n != 0) {
+            al = SSL_AD_ILLEGAL_PARAMETER;
+            SSLerr(SSL_F_SSL3_GET_CHANGE_CIPHER_SPEC, SSL_R_BAD_CHANGE_CIPHER_SPEC);
+            goto f_err;
+        }
     }
 
     /* Check we have a cipher to change to */
@@ -262,6 +273,22 @@ int ssl3_get_change_cipher_spec(SSL *s, int a, int b)
         goto f_err;
     }
 
+    if (SSL_IS_DTLS(s)) {
+        dtls1_reset_seq_numbers(s, SSL3_CC_READ);
+
+        if (s->version == DTLS1_BAD_VER)
+            s->d1->handshake_read_seq++;
+
+#ifndef OPENSSL_NO_SCTP
+        /*
+         * Remember that a CCS has been received, so that an old key of
+         * SCTP-Auth can be deleted when a CCS is sent. Will be ignored if no
+         * SCTP is used
+         */
+        BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_SCTP_AUTH_CCS_RCVD, 1, NULL);
+#endif
+    }
+
     return 1;
  f_err:
     ssl3_send_alert(s, SSL3_AL_FATAL, al);
index d13aa05..bc8388a 100644 (file)
@@ -1437,11 +1437,6 @@ typedef struct dtls1_state_st {
     unsigned short timeout_duration;
 
     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
     /* used when SSL_ST_XX_FLUSH is entered */
     int next_state;