Move DTLS CCS processing into the state machine

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: NTim Hudson <tjh@openssl.org>

ssl/d1_both.c

@@ -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;

ssl/d1_clnt.c

@@ -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)

ssl/d1_srvr.c

@@ -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
                 /*

ssl/record/rec_layer_d1.c

@@ -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);
 }
 

ssl/s3_both.c

@@ -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);

ssl/ssl_locl.h

@@ -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;