Clean up {dtls1,ssl3}_read_bytes()

Now that {dtls1,ssl3}_read_bytes() have been refactored, do a clean up
pass - this cleans up various parts of the code and reduces differences
between these two functions.

ok = 1; *(&(ok)) tb@

ok inoguchi@

2 files changed, 166 insertions, 200 deletions

diff --git a/src/lib/libssl/d1_pkt.c b/src/lib/libssl/d1_pkt.c
index f17608608e..456f871a43 100644
--- a/src/lib/libssl/d1_pkt.c
+++ b/src/lib/libssl/d1_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.122 2022/03/26 15:00:51 jsing Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.123 2022/03/26 15:05:53 jsing Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -685,29 +685,37 @@ dtls1_read_handshake_unexpected(SSL *s)
 int
 dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
 {
-        int al, i, ret;
+        SSL3_RECORD_INTERNAL *rr;
         int rrcount = 0;
         unsigned int n;
-        SSL3_RECORD_INTERNAL *rr;
+        int ret;
 
-        if (s->s3->rbuf.buf == NULL) /* Not initialized yet */
+        if (s->s3->rbuf.buf == NULL) {
                 if (!ssl3_setup_buffers(s))
-                        return (-1);
+                        return -1;
+        }
 
-        if ((type &&
-             type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_HANDSHAKE) ||
-            (peek && (type != SSL3_RT_APPLICATION_DATA))) {
+        if (len < 0) {
                 SSLerror(s, ERR_R_INTERNAL_ERROR);
                 return -1;
         }
 
-        if (!s->internal->in_handshake && SSL_in_init(s)) {
-                i = s->internal->handshake_func(s);
-                if (i < 0)
-                        return (i);
-                if (i == 0) {
+        if (type != 0 && type != SSL3_RT_APPLICATION_DATA &&
+            type != SSL3_RT_HANDSHAKE) {
+                SSLerror(s, ERR_R_INTERNAL_ERROR);
+                return -1;
+        }
+        if (peek && type != SSL3_RT_APPLICATION_DATA) {
+                SSLerror(s, ERR_R_INTERNAL_ERROR);
+                return -1;
+        }
+
+        if (SSL_in_init(s) && !s->internal->in_handshake) {
+                if ((ret = s->internal->handshake_func(s)) < 0)
+                        return ret;
+                if (ret == 0) {
                         SSLerror(s, SSL_R_SSL_HANDSHAKE_FAILURE);
-                        return (-1);
+                        return -1;
                 }
         }
 
@@ -727,33 +735,24 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
 
         s->internal->rwstate = SSL_NOTHING;
 
-        /* s->s3->rrec.type         - is the type of record
-         * s->s3->rrec.data,    - data
-         * s->s3->rrec.off,     - offset into 'data' for next read
-         * s->s3->rrec.length,  - number of bytes. */
-        rr = &(s->s3->rrec);
+        rr = &s->s3->rrec;
 
-        /* We are not handshaking and have no data yet,
-         * so process data buffered during the last handshake
-         * in advance, if any.
+        /*
+         * We are not handshaking and have no data yet, so process data buffered
+         * during the last handshake in advance, if any.
          */
         if (s->s3->hs.state == SSL_ST_OK && rr->length == 0)
-                dtls1_retrieve_buffered_record(s, &(s->d1->buffered_app_data));
+                dtls1_retrieve_buffered_record(s, &s->d1->buffered_app_data);
 
-        /* Check for timeout */
         if (dtls1_handle_timeout(s) > 0)
                 goto start;
 
-        /* get new packet if necessary */
-        if ((rr->length == 0) || (s->internal->rstate == SSL_ST_READ_BODY)) {
-                ret = dtls1_get_record(s);
-                if (ret <= 0) {
-                        ret = dtls1_read_failed(s, ret);
-                        /* anything other than a timeout is an error */
-                        if (ret <= 0)
-                                return (ret);
-                        else
-                                goto start;
+        if (rr->length == 0 || s->internal->rstate == SSL_ST_READ_BODY) {
+                if ((ret = dtls1_get_record(s)) <= 0) {
+                        /* Anything other than a timeout is an error. */
+                        if ((ret = dtls1_read_failed(s, ret)) <= 0)
+                                return ret;
+                        goto start;
                 }
         }
 
@@ -762,17 +761,16 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
                 goto start;
         }
 
-        /* we now have a packet which can be read and processed */
+        /* We now have a packet which can be read and processed. */
 
-        if (s->s3->change_cipher_spec /* set when we receive ChangeCipherSpec,
-                                       * reset by ssl3_get_finished */
-            && (rr->type != SSL3_RT_HANDSHAKE)) {
-                /* We now have application data between CCS and Finished.
+        if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE) {
+                /*
+                 * We now have application data between CCS and Finished.
                  * Most likely the packets were reordered on their way, so
                  * buffer the application data for later processing rather
                  * than dropping the connection.
                  */
-                if (dtls1_buffer_record(s, &(s->d1->buffered_app_data),
+                if (dtls1_buffer_record(s, &s->d1->buffered_app_data,
                     rr->seq_num) < 0) {
                         SSLerror(s, ERR_R_INTERNAL_ERROR);
                         return (-1);
@@ -781,35 +779,41 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
                 goto start;
         }
 
-        /* If the other end has shut down, throw anything we read away
-         * (even in 'peek' mode) */
+        /*
+         * If the other end has shut down, throw anything we read away (even in
+         * 'peek' mode).
+         */
         if (s->internal->shutdown & SSL_RECEIVED_SHUTDOWN) {
-                rr->length = 0;
                 s->internal->rwstate = SSL_NOTHING;
-                return (0);
+                rr->length = 0;
+                return 0;
         }
 
         /* SSL3_RT_APPLICATION_DATA or SSL3_RT_HANDSHAKE */
         if (type == rr->type) {
-                /* make sure that we are not getting application data when we
-                 * are doing a handshake for the first time */
+                /*
+                 * Make sure that we are not getting application data when we
+                 * are doing a handshake for the first time.
+                 */
                 if (SSL_in_init(s) && type == SSL3_RT_APPLICATION_DATA &&
                     !tls12_record_layer_read_protected(s->internal->rl)) {
-                        al = SSL_AD_UNEXPECTED_MESSAGE;
                         SSLerror(s, SSL_R_APP_DATA_IN_HANDSHAKE);
-                        goto fatal_err;
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                            SSL_AD_UNEXPECTED_MESSAGE);
+                        return -1;
                 }
 
                 if (len <= 0)
-                        return (len);
+                        return len;
 
                 if ((unsigned int)len > rr->length)
                         n = rr->length;
                 else
                         n = (unsigned int)len;
 
-                memcpy(buf, &(rr->data[rr->off]), n);
+                memcpy(buf, &rr->data[rr->off], n);
                 if (!peek) {
+                        memset(&rr->data[rr->off], 0, n);
                         rr->length -= n;
                         rr->off += n;
                         if (rr->length == 0) {
@@ -818,7 +822,7 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
                         }
                 }
 
-                return (n);
+                return n;
         }
 
         /*
@@ -838,42 +842,16 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
                 return (0);
         }
 
-        if (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC) {
-                if ((ret = ssl3_read_change_cipher_spec(s)) <= 0)
-                        return ret;
-                goto start;
-        }
-
-        if (rr->type == SSL3_RT_HANDSHAKE) {
-                if ((ret = dtls1_read_handshake_unexpected(s)) <= 0)
-                        return ret;
-                goto start;
-        }
-
-        switch (rr->type) {
-        default:
-                al = SSL_AD_UNEXPECTED_MESSAGE;
-                SSLerror(s, SSL_R_UNEXPECTED_RECORD);
-                goto fatal_err;
-        case SSL3_RT_CHANGE_CIPHER_SPEC:
-        case SSL3_RT_ALERT:
-        case SSL3_RT_HANDSHAKE:
-                /* we already handled all of these, with the possible exception
-                 * of SSL3_RT_HANDSHAKE when s->internal->in_handshake is set, but that
-                 * should not happen when type != rr->type */
-                al = SSL_AD_UNEXPECTED_MESSAGE;
-                SSLerror(s, ERR_R_INTERNAL_ERROR);
-                goto fatal_err;
-        case SSL3_RT_APPLICATION_DATA:
-                /* At this point, we were expecting handshake data,
-                 * but have application data.  If the library was
-                 * running inside ssl3_read() (i.e. in_read_app_data
-                 * is set) and it makes sense to read application data
-                 * at this point (session renegotiation not yet started),
-                 * we will indulge it.
+        if (rr->type == SSL3_RT_APPLICATION_DATA) {
+                /*
+                 * At this point, we were expecting handshake data, but have
+                 * application data. If the library was running inside
+                 * ssl3_read() (i.e. in_read_app_data is set) and it makes
+                 * sense to read application data at this point (session
+                 * renegotiation not yet started), we will indulge it.
                  */
-                if (s->s3->in_read_app_data &&
-                    (s->s3->total_renegotiations != 0) &&
+                if (s->s3->in_read_app_data != 0 &&
+                    s->s3->total_renegotiations != 0 &&
                     (((s->s3->hs.state & SSL_ST_CONNECT) &&
                     (s->s3->hs.state >= SSL3_ST_CW_CLNT_HELLO_A) &&
                     (s->s3->hs.state <= SSL3_ST_CR_SRVR_HELLO_A)) || (
@@ -881,19 +859,31 @@ dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
                     (s->s3->hs.state <= SSL3_ST_SW_HELLO_REQ_A) &&
                     (s->s3->hs.state >= SSL3_ST_SR_CLNT_HELLO_A)))) {
                         s->s3->in_read_app_data = 2;
-                        return (-1);
+                        return -1;
                 } else {
-                        al = SSL_AD_UNEXPECTED_MESSAGE;
                         SSLerror(s, SSL_R_UNEXPECTED_RECORD);
-                        goto fatal_err;
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                            SSL_AD_UNEXPECTED_MESSAGE);
+                        return -1;
                 }
         }
-        /* not reached */
 
- fatal_err:
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
+        if (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC) {
+                if ((ret = ssl3_read_change_cipher_spec(s)) <= 0)
+                        return ret;
+                goto start;
+        }
 
-        return (-1);
+        if (rr->type == SSL3_RT_HANDSHAKE) {
+                if ((ret = dtls1_read_handshake_unexpected(s)) <= 0)
+                        return ret;
+                goto start;
+        }
+
+        /* Unknown record type. */
+        SSLerror(s, SSL_R_UNEXPECTED_RECORD);
+        ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+        return -1;
 }
 
 int
diff --git a/src/lib/libssl/ssl_pkt.c b/src/lib/libssl/ssl_pkt.c
index 57230f8fae..3dd0269540 100644
--- a/src/lib/libssl/ssl_pkt.c
+++ b/src/lib/libssl/ssl_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_pkt.c,v 1.57 2022/03/17 17:28:08 jsing Exp $ */
+/* $OpenBSD: ssl_pkt.c,v 1.58 2022/03/26 15:05:53 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1008,37 +1008,40 @@ ssl3_read_handshake_unexpected(SSL *s)
 int
 ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
 {
-        int al, i, ret;
+        SSL3_RECORD_INTERNAL *rr;
         int rrcount = 0;
         unsigned int n;
-        SSL3_RECORD_INTERNAL *rr;
+        int ret;
 
-        if (s->s3->rbuf.buf == NULL) /* Not initialized yet */
+        if (s->s3->rbuf.buf == NULL) {
                 if (!ssl3_setup_read_buffer(s))
-                        return (-1);
+                        return -1;
+        }
 
         if (len < 0) {
                 SSLerror(s, ERR_R_INTERNAL_ERROR);
                 return -1;
         }
 
-        if ((type && type != SSL3_RT_APPLICATION_DATA &&
-            type != SSL3_RT_HANDSHAKE) ||
-            (peek && (type != SSL3_RT_APPLICATION_DATA))) {
+        if (type != 0 && type != SSL3_RT_APPLICATION_DATA &&
+            type != SSL3_RT_HANDSHAKE) {
+                SSLerror(s, ERR_R_INTERNAL_ERROR);
+                return -1;
+        }
+        if (peek && type != SSL3_RT_APPLICATION_DATA) {
                 SSLerror(s, ERR_R_INTERNAL_ERROR);
                 return -1;
         }
 
-        if ((type == SSL3_RT_HANDSHAKE) &&
-            (s->s3->handshake_fragment_len > 0)) {
-                /* (partially) satisfy request from storage */
+        if (type == SSL3_RT_HANDSHAKE && s->s3->handshake_fragment_len > 0) {
+                /* Partially satisfy request from fragment storage. */
                 unsigned char *src = s->s3->handshake_fragment;
                 unsigned char *dst = buf;
                 unsigned int k;
 
                 /* peek == 0 */
                 n = 0;
-                while ((len > 0) && (s->s3->handshake_fragment_len > 0)) {
+                while (len > 0 && s->s3->handshake_fragment_len > 0) {
                         *dst++ = *src++;
                         len--;
                         s->s3->handshake_fragment_len--;
@@ -1050,18 +1053,12 @@ ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
                 return n;
         }
 
-        /*
-         * Now s->s3->handshake_fragment_len == 0 if
-         * type == SSL3_RT_HANDSHAKE.
-         */
-        if (!s->internal->in_handshake && SSL_in_init(s)) {
-                /* type == SSL3_RT_APPLICATION_DATA */
-                i = s->internal->handshake_func(s);
-                if (i < 0)
-                        return (i);
-                if (i == 0) {
+        if (SSL_in_init(s) && !s->internal->in_handshake) {
+                if ((ret = s->internal->handshake_func(s)) < 0)
+                        return ret;
+                if (ret == 0) {
                         SSLerror(s, SSL_R_SSL_HANDSHAKE_FAILURE);
-                        return (-1);
+                        return -1;
                 }
         }
 
@@ -1081,61 +1078,56 @@ ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
 
         s->internal->rwstate = SSL_NOTHING;
 
-        /*
-         * s->s3->rrec.type         - is the type of record
-         * s->s3->rrec.data,    - data
-         * s->s3->rrec.off,     - offset into 'data' for next read
-         * s->s3->rrec.length,  - number of bytes.
-         */
-        rr = &(s->s3->rrec);
+        rr = &s->s3->rrec;
 
-        /* get new packet if necessary */
-        if ((rr->length == 0) || (s->internal->rstate == SSL_ST_READ_BODY)) {
-                ret = ssl3_get_record(s);
-                if (ret <= 0)
-                        return (ret);
+        if (rr->length == 0 || s->internal->rstate == SSL_ST_READ_BODY) {
+                if ((ret = ssl3_get_record(s)) <= 0)
+                        return ret;
         }
 
-        /* we now have a packet which can be read and processed */
+        /* We now have a packet which can be read and processed. */
 
-        if (s->s3->change_cipher_spec /* set when we receive ChangeCipherSpec,
-                                       * reset by ssl3_get_finished */
-            && (rr->type != SSL3_RT_HANDSHAKE)) {
-                al = SSL_AD_UNEXPECTED_MESSAGE;
+        if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE) {
                 SSLerror(s, SSL_R_DATA_BETWEEN_CCS_AND_FINISHED);
-                goto fatal_err;
+                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+                return -1;
         }
 
-        /* If the other end has shut down, throw anything we read away
-         * (even in 'peek' mode) */
+        /*
+         * If the other end has shut down, throw anything we read away (even in
+         * 'peek' mode).
+         */
         if (s->internal->shutdown & SSL_RECEIVED_SHUTDOWN) {
-                rr->length = 0;
                 s->internal->rwstate = SSL_NOTHING;
-                return (0);
+                rr->length = 0;
+                return 0;
         }
 
         /* SSL3_RT_APPLICATION_DATA or SSL3_RT_HANDSHAKE */
         if (type == rr->type) {
-                /* make sure that we are not getting application data when we
-                 * are doing a handshake for the first time */
+                /*
+                 * Make sure that we are not getting application data when we
+                 * are doing a handshake for the first time.
+                 */
                 if (SSL_in_init(s) && type == SSL3_RT_APPLICATION_DATA &&
                     !tls12_record_layer_read_protected(s->internal->rl)) {
-                        al = SSL_AD_UNEXPECTED_MESSAGE;
                         SSLerror(s, SSL_R_APP_DATA_IN_HANDSHAKE);
-                        goto fatal_err;
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                            SSL_AD_UNEXPECTED_MESSAGE);
+                        return -1;
                 }
 
                 if (len <= 0)
-                        return (len);
+                        return len;
 
                 if ((unsigned int)len > rr->length)
                         n = rr->length;
                 else
                         n = (unsigned int)len;
 
-                memcpy(buf, &(rr->data[rr->off]), n);
+                memcpy(buf, &rr->data[rr->off], n);
                 if (!peek) {
-                        memset(&(rr->data[rr->off]), 0, n);
+                        memset(&rr->data[rr->off], 0, n);
                         rr->length -= n;
                         rr->off += n;
                         if (rr->length == 0) {
@@ -1146,7 +1138,8 @@ ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
                                         ssl3_release_read_buffer(s);
                         }
                 }
-                return (n);
+
+                return n;
         }
 
         /*
@@ -1161,77 +1154,60 @@ ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
         }
 
         if (s->internal->shutdown & SSL_SENT_SHUTDOWN) {
-                /* but we have not received a shutdown */
                 s->internal->rwstate = SSL_NOTHING;
                 rr->length = 0;
-                return (0);
-        }
-
-        if (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC) {
-                if ((ret = ssl3_read_change_cipher_spec(s)) <= 0)
-                        return ret;
-                goto start;
-        }
-
-        if (rr->type == SSL3_RT_HANDSHAKE) {
-                if ((ret = ssl3_read_handshake_unexpected(s)) <= 0)
-                        return ret;
-                goto start;
+                return 0;
         }
 
-        switch (rr->type) {
-        default:
+        if (rr->type == SSL3_RT_APPLICATION_DATA) {
                 /*
-                 * TLS up to v1.1 just ignores unknown message types:
-                 * TLS v1.2 give an unexpected message alert.
+                 * At this point, we were expecting handshake data, but have
+                 * application data. If the library was running inside
+                 * ssl3_read() (i.e. in_read_app_data is set) and it makes
+                 * sense to read application data at this point (session
+                 * renegotiation not yet started), we will indulge it.
                  */
-                if (s->version >= TLS1_VERSION &&
-                    s->version <= TLS1_1_VERSION) {
-                        rr->length = 0;
-                        goto start;
-                }
-                al = SSL_AD_UNEXPECTED_MESSAGE;
-                SSLerror(s, SSL_R_UNEXPECTED_RECORD);
-                goto fatal_err;
-        case SSL3_RT_CHANGE_CIPHER_SPEC:
-        case SSL3_RT_ALERT:
-        case SSL3_RT_HANDSHAKE:
-                /* we already handled all of these, with the possible exception
-                 * of SSL3_RT_HANDSHAKE when s->internal->in_handshake is set, but that
-                 * should not happen when type != rr->type */
-                al = SSL_AD_UNEXPECTED_MESSAGE;
-                SSLerror(s, ERR_R_INTERNAL_ERROR);
-                goto fatal_err;
-        case SSL3_RT_APPLICATION_DATA:
-                /* At this point, we were expecting handshake data,
-                 * but have application data.  If the library was
-                 * running inside ssl3_read() (i.e. in_read_app_data
-                 * is set) and it makes sense to read application data
-                 * at this point (session renegotiation not yet started),
-                 * we will indulge it.
-                 */
-                if (s->s3->in_read_app_data &&
-                    (s->s3->total_renegotiations != 0) &&
+                if (s->s3->in_read_app_data != 0 &&
+                    s->s3->total_renegotiations != 0 &&
                     (((s->s3->hs.state & SSL_ST_CONNECT) &&
                     (s->s3->hs.state >= SSL3_ST_CW_CLNT_HELLO_A) &&
-                    (s->s3->hs.state <= SSL3_ST_CR_SRVR_HELLO_A)) ||
-                    ((s->s3->hs.state & SSL_ST_ACCEPT) &&
+                    (s->s3->hs.state <= SSL3_ST_CR_SRVR_HELLO_A)) || (
+                    (s->s3->hs.state & SSL_ST_ACCEPT) &&
                     (s->s3->hs.state <= SSL3_ST_SW_HELLO_REQ_A) &&
                     (s->s3->hs.state >= SSL3_ST_SR_CLNT_HELLO_A)))) {
                         s->s3->in_read_app_data = 2;
-                        return (-1);
+                        return -1;
                 } else {
-                        al = SSL_AD_UNEXPECTED_MESSAGE;
                         SSLerror(s, SSL_R_UNEXPECTED_RECORD);
-                        goto fatal_err;
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                            SSL_AD_UNEXPECTED_MESSAGE);
+                        return -1;
                 }
         }
-        /* not reached */
 
- fatal_err:
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
+        if (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC) {
+                if ((ret = ssl3_read_change_cipher_spec(s)) <= 0)
+                        return ret;
+                goto start;
+        }
 
-        return (-1);
+        if (rr->type == SSL3_RT_HANDSHAKE) {
+                if ((ret = ssl3_read_handshake_unexpected(s)) <= 0)
+                        return ret;
+                goto start;
+        }
+
+        /*
+         * Unknown record type - TLSv1.2 sends an unexpected message alert while
+         * earlier versions silently ignore the record.
+         */
+        if (ssl_effective_tls_version(s) <= TLS1_1_VERSION) {
+                rr->length = 0;
+                goto start;
+        }
+        SSLerror(s, SSL_R_UNEXPECTED_RECORD);
+        ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+        return -1;
 }
 
 int