From: Matt Caswell Date: Thu, 27 Oct 2022 14:38:32 +0000 (+0100) Subject: Resolve a TODO in ssl3_dispatch_alert X-Git-Tag: openssl-3.2.0-alpha1~1736 X-Git-Url: https://git.openssl.org/?p=openssl.git;a=commitdiff_plain;h=732435026b0141063084fb68c076bc1c9fd9bee8 Resolve a TODO in ssl3_dispatch_alert Properly handle the case where there is pending write data and we want to send an alert. Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/19550) --- diff --git a/ssl/d1_msg.c b/ssl/d1_msg.c index 279435ca03..eb84ed6470 100644 --- a/ssl/d1_msg.c +++ b/ssl/d1_msg.c @@ -48,7 +48,7 @@ int dtls1_dispatch_alert(SSL *ssl) if (s == NULL) return 0; - s->s3.alert_dispatch = 0; + s->s3.alert_dispatch = SSL_ALERT_DISPATCH_NONE; memset(buf, 0, sizeof(buf)); *ptr++ = s->s3.send_alert[0]; diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c index 88f596e239..2520288dd4 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c @@ -621,7 +621,7 @@ int do_dtls1_write(SSL_CONNECTION *sc, int type, const unsigned char *buf, int ret; /* If we have an alert to send, lets send it */ - if (sc->s3.alert_dispatch) { + if (sc->s3.alert_dispatch > 0) { i = s->method->ssl_dispatch_alert(s); if (i <= 0) return i; diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index dc0b8b3d9e..b4435bf020 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -315,7 +315,7 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, } /* If we have an alert to send, lets send it */ - if (s->s3.alert_dispatch) { + if (s->s3.alert_dispatch > 0) { i = ssl->method->ssl_dispatch_alert(ssl); if (i <= 0) { /* SSLfatal() already called if appropriate */ diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 0cf7954872..e7078efa6c 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -4443,11 +4443,11 @@ int ssl3_shutdown(SSL *s) ssl3_send_alert(sc, SSL3_AL_WARNING, SSL_AD_CLOSE_NOTIFY); /* * our shutdown alert has been sent now, and if it still needs to be - * written, s->s3.alert_dispatch will be true + * written, s->s3.alert_dispatch will be > 0 */ - if (sc->s3.alert_dispatch) + if (sc->s3.alert_dispatch > 0) return -1; /* return WANT_WRITE */ - } else if (sc->s3.alert_dispatch) { + } else if (sc->s3.alert_dispatch > 0) { /* resend it if not sent */ ret = s->method->ssl_dispatch_alert(s); if (ret == -1) { @@ -4469,8 +4469,8 @@ int ssl3_shutdown(SSL *s) } } - if ((sc->shutdown == (SSL_SENT_SHUTDOWN | SSL_RECEIVED_SHUTDOWN)) && - !sc->s3.alert_dispatch) + if ((sc->shutdown == (SSL_SENT_SHUTDOWN | SSL_RECEIVED_SHUTDOWN)) + && sc->s3.alert_dispatch == SSL_ALERT_DISPATCH_NONE) return 1; else return 0; diff --git a/ssl/s3_msg.c b/ssl/s3_msg.c index c656cd7670..3fcea15e27 100644 --- a/ssl/s3_msg.c +++ b/ssl/s3_msg.c @@ -61,7 +61,7 @@ int ssl3_send_alert(SSL_CONNECTION *s, int level, int desc) if ((level == SSL3_AL_FATAL) && (s->session != NULL)) SSL_CTX_remove_session(s->session_ctx, s->session); - s->s3.alert_dispatch = 1; + s->s3.alert_dispatch = SSL_ALERT_DISPATCH_PENDING; s->s3.send_alert[0] = level; s->s3.send_alert[1] = desc; if (!RECORD_LAYER_write_pending(&s->rlayer)) { @@ -85,10 +85,9 @@ int ssl3_dispatch_alert(SSL *s) if (sc == NULL) return -1; - sc->s3.alert_dispatch = 0; - if (sc->rlayer.wrlmethod == NULL) { /* No write record layer so we can't sent and alert. We just ignore it */ + sc->s3.alert_dispatch = SSL_ALERT_DISPATCH_NONE; return 1; } @@ -104,20 +103,43 @@ int ssl3_dispatch_alert(SSL *s) templ.buf = &sc->s3.send_alert[0]; templ.buflen = 2; - /* TODO(RECLAYER): What happens if there is already a write pending? */ - if (RECORD_LAYER_write_pending(&sc->rlayer)) - return -1; + if (RECORD_LAYER_write_pending(&sc->rlayer)) { + if (sc->s3.alert_dispatch != SSL_ALERT_DISPATCH_RETRY) { + /* + * We have a write pending but it wasn't from a previous call to + * this function! Can we ever get here? Maybe via API misuse?? + * Give up. + */ + sc->s3.alert_dispatch = SSL_ALERT_DISPATCH_NONE; + return -1; + } + /* Retry what we've already got pending */ + i = HANDLE_RLAYER_WRITE_RETURN(sc, + sc->rlayer.wrlmethod->retry_write_records(sc->rlayer.wrl)); + if (i <= 0) { + /* Could be NBIO. Keep alert_dispatch as SSL_ALERT_DISPATCH_RETRY */ + return -1; + } + sc->rlayer.wpend_tot = 0; + sc->s3.alert_dispatch = SSL_ALERT_DISPATCH_NONE; + return 1; + } i = HANDLE_RLAYER_WRITE_RETURN(sc, sc->rlayer.wrlmethod->write_records(sc->rlayer.wrl, &templ, 1)); + if (i <= 0) { - sc->s3.alert_dispatch = 1; + sc->s3.alert_dispatch = SSL_ALERT_DISPATCH_RETRY; + sc->rlayer.wpend_tot = templ.buflen; + sc->rlayer.wpend_type = templ.type; + sc->rlayer.wpend_buf = templ.buf; } else { /* * Alert sent to BIO - now flush. If the message does not get sent due * to non-blocking IO, we will not worry too much. */ (void)BIO_flush(sc->wbio); + sc->s3.alert_dispatch = SSL_ALERT_DISPATCH_NONE; if (sc->msg_callback) sc->msg_callback(1, sc->version, SSL3_RT_ALERT, sc->s3.send_alert, diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index e15bf29ffb..f8b8378ee9 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2418,7 +2418,7 @@ ossl_ssize_t SSL_sendfile(SSL *s, int fd, off_t offset, size_t size, int flags) } /* If we have an alert to send, lets send it */ - if (sc->s3.alert_dispatch) { + if (sc->s3.alert_dispatch > 0) { ret = (ossl_ssize_t)s->method->ssl_dispatch_alert(s); if (ret <= 0) { /* SSLfatal() already called if appropriate */ diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index 50e3b65372..da01e904a3 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -400,6 +400,15 @@ # define SSL_READ_ETM(s) (s->s3.flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_READ) # define SSL_WRITE_ETM(s) (s->s3.flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE) +/* alert_dispatch values */ + +/* No alert pending */ +# define SSL_ALERT_DISPATCH_NONE 0 +/* Alert pending */ +# define SSL_ALERT_DISPATCH_PENDING 1 +/* Pending alert write needs to be retried */ +# define SSL_ALERT_DISPATCH_RETRY 2 + /* Mostly for SSLv3 */ # define SSL_PKEY_RSA 0 # define SSL_PKEY_RSA_PSS_SIGN 1