From 3fbacf810de490214fc4dba0b34a532906eadfac Mon Sep 17 00:00:00 2001 From: jsing <> Date: Thu, 10 Nov 2022 18:06:37 +0000 Subject: Use tls_buffer for alert and handshake fragments in the legacy stack. This avoids a bunch of pointer munging and a handrolled memmove. ok tb@ --- src/lib/libssl/s3_lib.c | 10 +++- src/lib/libssl/ssl_locl.h | 11 ++--- src/lib/libssl/ssl_pkt.c | 108 +++++++++++++++++++++++++++--------------- src/lib/libssl/tls_buffer.c | 11 ++++- src/lib/libssl/tls_internal.h | 3 +- 5 files changed, 94 insertions(+), 49 deletions(-) diff --git a/src/lib/libssl/s3_lib.c b/src/lib/libssl/s3_lib.c index 52ad16a697..68c6fc6324 100644 --- a/src/lib/libssl/s3_lib.c +++ b/src/lib/libssl/s3_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: s3_lib.c,v 1.239 2022/10/02 16:36:41 jsing Exp $ */ +/* $OpenBSD: s3_lib.c,v 1.240 2022/11/10 18:06:37 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1560,6 +1560,9 @@ ssl3_free(SSL *s) ssl3_release_read_buffer(s); ssl3_release_write_buffer(s); + tls_buffer_free(s->s3->alert_fragment); + tls_buffer_free(s->s3->handshake_fragment); + freezero(s->s3->hs.sigalgs, s->s3->hs.sigalgs_len); sk_X509_pop_free(s->s3->hs.peer_certs, X509_free); sk_X509_pop_free(s->s3->hs.peer_certs_no_leaf, X509_free); @@ -1598,6 +1601,11 @@ ssl3_clear(SSL *s) sk_X509_pop_free(s->verified_chain, X509_free); s->verified_chain = NULL; + tls_buffer_free(s->s3->alert_fragment); + s->s3->alert_fragment = NULL; + tls_buffer_free(s->s3->handshake_fragment); + s->s3->handshake_fragment = NULL; + freezero(s->s3->hs.sigalgs, s->s3->hs.sigalgs_len); s->s3->hs.sigalgs = NULL; s->s3->hs.sigalgs_len = 0; diff --git a/src/lib/libssl/ssl_locl.h b/src/lib/libssl/ssl_locl.h index 64cf6a60f9..69546c0962 100644 --- a/src/lib/libssl/ssl_locl.h +++ b/src/lib/libssl/ssl_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_locl.h,v 1.430 2022/11/07 11:58:45 jsing Exp $ */ +/* $OpenBSD: ssl_locl.h,v 1.431 2022/11/10 18:06:37 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1168,12 +1168,9 @@ typedef struct ssl3_state_st { SSL3_RECORD_INTERNAL rrec; /* each decoded record goes in here */ - /* storage for Alert/Handshake protocol data received but not - * yet processed by ssl3_read_bytes: */ - unsigned char alert_fragment[2]; - unsigned int alert_fragment_len; - unsigned char handshake_fragment[4]; - unsigned int handshake_fragment_len; + /* Unprocessed Alert/Handshake protocol data. */ + struct tls_buffer *alert_fragment; + struct tls_buffer *handshake_fragment; /* partial write - check the numbers match */ unsigned int wnum; /* number of bytes sent so far */ diff --git a/src/lib/libssl/ssl_pkt.c b/src/lib/libssl/ssl_pkt.c index ddb2ce0935..58d3ee1db2 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.62 2022/10/21 15:48:14 tb Exp $ */ +/* $OpenBSD: ssl_pkt.c,v 1.63 2022/11/10 18:06:37 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -688,11 +688,32 @@ ssl3_write_pending(SSL *s, int type, const unsigned char *buf, unsigned int len) } } +static ssize_t +ssl3_read_cb(void *buf, size_t n, void *cb_arg) +{ + SSL3_RECORD_INTERNAL *rr; + SSL *s = cb_arg; + + rr = &s->s3->rrec; + + if (n > rr->length) + n = rr->length; + + memcpy(buf, &rr->data[rr->off], n); + + rr->off += n; + rr->length -= n; + + return n; +} + +#define SSL3_ALERT_LENGTH 2 + int ssl3_read_alert(SSL *s) { - SSL3_RECORD_INTERNAL *rr = &s->s3->rrec; uint8_t alert_level, alert_descr; + ssize_t ret; CBS cbs; /* @@ -702,13 +723,17 @@ ssl3_read_alert(SSL *s) * fragmented across multiple records, hence a full alert must be * available in the record. */ - while (rr->length > 0 && - s->s3->alert_fragment_len < sizeof(s->s3->alert_fragment)) { - s->s3->alert_fragment[s->s3->alert_fragment_len++] = - rr->data[rr->off++]; - rr->length--; + if (s->s3->alert_fragment == NULL) { + if ((s->s3->alert_fragment = tls_buffer_new(0)) == NULL) + return -1; + tls_buffer_set_capacity_limit(s->s3->alert_fragment, + SSL3_ALERT_LENGTH); } - if (s->s3->alert_fragment_len < sizeof(s->s3->alert_fragment)) { + ret = tls_buffer_extend(s->s3->alert_fragment, SSL3_ALERT_LENGTH, + ssl3_read_cb, s); + if (ret <= 0 && ret != TLS_IO_WANT_POLLIN) + return -1; + if (ret != SSL3_ALERT_LENGTH) { if (SSL_is_dtls(s)) { SSLerror(s, SSL_R_BAD_LENGTH); ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); @@ -717,7 +742,8 @@ ssl3_read_alert(SSL *s) return 1; } - CBS_init(&cbs, s->s3->alert_fragment, sizeof(s->s3->alert_fragment)); + if (!tls_buffer_data(s->s3->alert_fragment, &cbs)) + return -1; ssl_msg_callback_cbs(s, 0, SSL3_RT_ALERT, &cbs); @@ -726,7 +752,8 @@ ssl3_read_alert(SSL *s) if (!CBS_get_u8(&cbs, &alert_descr)) return -1; - s->s3->alert_fragment_len = 0; + tls_buffer_free(s->s3->alert_fragment); + s->s3->alert_fragment = NULL; ssl_info_callback(s, SSL_CB_READ_ALERT, (alert_level << 8) | alert_descr); @@ -829,9 +856,9 @@ ssl3_read_change_cipher_spec(SSL *s) static int ssl3_read_handshake_unexpected(SSL *s) { - SSL3_RECORD_INTERNAL *rr = &s->s3->rrec; uint32_t hs_msg_length; uint8_t hs_msg_type; + ssize_t ssret; CBS cbs; int ret; @@ -840,14 +867,17 @@ ssl3_read_handshake_unexpected(SSL *s) * header - this may be in the same record or fragmented across multiple * records. */ - while (rr->length > 0 && - s->s3->handshake_fragment_len < sizeof(s->s3->handshake_fragment)) { - s->s3->handshake_fragment[s->s3->handshake_fragment_len++] = - rr->data[rr->off++]; - rr->length--; + if (s->s3->handshake_fragment == NULL) { + if ((s->s3->handshake_fragment = tls_buffer_new(0)) == NULL) + return -1; + tls_buffer_set_capacity_limit(s->s3->handshake_fragment, + SSL3_HM_HEADER_LENGTH); } - - if (s->s3->handshake_fragment_len < sizeof(s->s3->handshake_fragment)) + ssret = tls_buffer_extend(s->s3->handshake_fragment, SSL3_HM_HEADER_LENGTH, + ssl3_read_cb, s); + if (ssret <= 0 && ssret != TLS_IO_WANT_POLLIN) + return -1; + if (ssret != SSL3_HM_HEADER_LENGTH) return 1; if (s->in_handshake) { @@ -862,7 +892,8 @@ ssl3_read_handshake_unexpected(SSL *s) */ /* Parse handshake message header. */ - CBS_init(&cbs, s->s3->handshake_fragment, s->s3->handshake_fragment_len); + if (!tls_buffer_data(s->s3->handshake_fragment, &cbs)) + return -1; if (!CBS_get_u8(&cbs, &hs_msg_type)) return -1; if (!CBS_get_u24(&cbs, &hs_msg_length)) @@ -888,10 +919,12 @@ ssl3_read_handshake_unexpected(SSL *s) return -1; } - ssl_msg_callback(s, 0, SSL3_RT_HANDSHAKE, - s->s3->handshake_fragment, s->s3->handshake_fragment_len); + if (!tls_buffer_data(s->s3->handshake_fragment, &cbs)) + return -1; + ssl_msg_callback_cbs(s, 0, SSL3_RT_HANDSHAKE, &cbs); - s->s3->handshake_fragment_len = 0; + tls_buffer_free(s->s3->handshake_fragment); + s->s3->handshake_fragment = NULL; /* * It should be impossible to hit this, but keep the safety @@ -1045,24 +1078,21 @@ ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) return -1; } - 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) { - *dst++ = *src++; - len--; - s->s3->handshake_fragment_len--; - n++; + if (type == SSL3_RT_HANDSHAKE && + s->s3->handshake_fragment != NULL && + tls_buffer_remaining(s->s3->handshake_fragment) > 0) { + ssize_t ssn; + + if ((ssn = tls_buffer_read(s->s3->handshake_fragment, buf, + len)) <= 0) + return -1; + + if (tls_buffer_remaining(s->s3->handshake_fragment) == 0) { + tls_buffer_free(s->s3->handshake_fragment); + s->s3->handshake_fragment = NULL; } - /* move any remaining fragment bytes: */ - for (k = 0; k < s->s3->handshake_fragment_len; k++) - s->s3->handshake_fragment[k] = *src++; - return n; + + return (int)ssn; } if (SSL_in_init(s) && !s->in_handshake) { diff --git a/src/lib/libssl/tls_buffer.c b/src/lib/libssl/tls_buffer.c index f70cfbc1a0..517d66d685 100644 --- a/src/lib/libssl/tls_buffer.c +++ b/src/lib/libssl/tls_buffer.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls_buffer.c,v 1.3 2022/07/22 19:33:53 jsing Exp $ */ +/* $OpenBSD: tls_buffer.c,v 1.4 2022/11/10 18:06:37 jsing Exp $ */ /* * Copyright (c) 2018, 2019, 2022 Joel Sing * @@ -155,6 +155,15 @@ tls_buffer_extend(struct tls_buffer *buf, size_t len, } } +size_t +tls_buffer_remaining(struct tls_buffer *buf) +{ + if (buf->offset > buf->len) + return 0; + + return buf->len - buf->offset; +} + ssize_t tls_buffer_read(struct tls_buffer *buf, uint8_t *rbuf, size_t n) { diff --git a/src/lib/libssl/tls_internal.h b/src/lib/libssl/tls_internal.h index 1d3a8133cd..84edde8474 100644 --- a/src/lib/libssl/tls_internal.h +++ b/src/lib/libssl/tls_internal.h @@ -1,4 +1,4 @@ -/* $OpenBSD: tls_internal.h,v 1.9 2022/07/24 14:28:16 jsing Exp $ */ +/* $OpenBSD: tls_internal.h,v 1.10 2022/11/10 18:06:37 jsing Exp $ */ /* * Copyright (c) 2018, 2019, 2021 Joel Sing * @@ -64,6 +64,7 @@ void tls_buffer_free(struct tls_buffer *buf); void tls_buffer_set_capacity_limit(struct tls_buffer *buf, size_t limit); ssize_t tls_buffer_extend(struct tls_buffer *buf, size_t len, tls_read_cb read_cb, void *cb_arg); +size_t tls_buffer_remaining(struct tls_buffer *buf); ssize_t tls_buffer_read(struct tls_buffer *buf, uint8_t *rbuf, size_t n); ssize_t tls_buffer_write(struct tls_buffer *buf, const uint8_t *wbuf, size_t n); int tls_buffer_append(struct tls_buffer *buf, const uint8_t *wbuf, size_t n); -- cgit v1.2.3-55-g6feb