From 824388bb1103ce457573af9cbc3c59508cc42974 Mon Sep 17 00:00:00 2001 From: jsing Date: Sat, 26 Mar 2022 15:05:53 +0000 Subject: [PATCH] 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@ --- lib/libssl/d1_pkt.c | 176 +++++++++++++++++++-------------------- lib/libssl/ssl_pkt.c | 190 +++++++++++++++++++------------------------ 2 files changed, 166 insertions(+), 200 deletions(-) diff --git a/lib/libssl/d1_pkt.c b/lib/libssl/d1_pkt.c index f17608608e5..456f871a436 100644 --- a/lib/libssl/d1_pkt.c +++ b/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/lib/libssl/ssl_pkt.c b/lib/libssl/ssl_pkt.c index 57230f8faeb..3dd0269540f 100644 --- a/lib/libssl/ssl_pkt.c +++ b/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 -- 2.20.1