From: Matt Caswell Date: Tue, 7 Aug 2018 11:40:08 +0000 (+0100) Subject: Tolerate encrypted or plaintext alerts X-Git-Tag: OpenSSL_1_1_1-pre9~35 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=de9e884b2f43c59834c2b1c3cfde35fa2c797f2b;p=openssl Tolerate encrypted or plaintext alerts At certain points in the handshake we could receive either a plaintext or an encrypted alert from the client. We should tolerate both where appropriate. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/6887) --- diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 10ac0936ed..d208695b16 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -816,8 +816,8 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, /* Clear our SSL3_RECORD structures */ memset(wr, 0, sizeof(wr)); for (j = 0; j < numpipes; j++) { - unsigned int version = SSL_TREAT_AS_TLS13(s) ? TLS1_2_VERSION - : s->version; + unsigned int version = (s->version == TLS1_3_VERSION) ? TLS1_2_VERSION + : s->version; unsigned char *compressdata = NULL; size_t maxcomplen; unsigned int rectype; diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index ad478bf375..a616bf0409 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -342,7 +342,10 @@ int ssl3_get_record(SSL *s) if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) { if (thisrr->type != SSL3_RT_APPLICATION_DATA && (thisrr->type != SSL3_RT_CHANGE_CIPHER_SPEC - || !SSL_IS_FIRST_HANDSHAKE(s))) { + || !SSL_IS_FIRST_HANDSHAKE(s)) + && (thisrr->type != SSL3_RT_ALERT + || s->statem.enc_read_state + != ENC_READ_STATE_ALLOW_PLAIN_ALERTS)) { SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE); return -1; @@ -692,7 +695,9 @@ int ssl3_get_record(SSL *s) } } - if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) { + if (SSL_IS_TLS13(s) + && s->enc_read_ctx != NULL + && thisrr->type != SSL3_RT_ALERT) { size_t end; if (thisrr->length == 0 diff --git a/ssl/record/ssl3_record_tls13.c b/ssl/record/ssl3_record_tls13.c index cbf6a652e7..a11ed483e6 100644 --- a/ssl/record/ssl3_record_tls13.c +++ b/ssl/record/ssl3_record_tls13.c @@ -52,10 +52,13 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending) seq = RECORD_LAYER_get_read_sequence(&s->rlayer); } - if (ctx == NULL - || (rec->type == SSL3_RT_ALERT - && s->statem.enc_write_state - == ENC_WRITE_STATE_WRITE_PLAIN_ALERTS)) { + /* + * If we're sending an alert and ctx != NULL then we must be forcing + * plaintext alerts. If we're reading and ctx != NULL then we allow + * plaintext alerts at certain points in the handshake. If we've got this + * far then we have already validated that a plaintext alert is ok here. + */ + if (ctx == NULL || rec->type == SSL3_RT_ALERT) { memmove(rec->data, rec->input, rec->length); rec->input = rec->data; return 1; diff --git a/ssl/statem/statem.h b/ssl/statem/statem.h index 0799870178..144d930fc7 100644 --- a/ssl/statem/statem.h +++ b/ssl/statem/statem.h @@ -80,6 +80,13 @@ typedef enum { ENC_WRITE_STATE_WRITE_PLAIN_ALERTS } ENC_WRITE_STATES; +typedef enum { + /* The enc_read_ctx can be used normally */ + ENC_READ_STATE_VALID, + /* We may receive encrypted or plaintext alerts */ + ENC_READ_STATE_ALLOW_PLAIN_ALERTS +} ENC_READ_STATES; + /***************************************************************************** * * * This structure should be considered "opaque" to anything outside of the * @@ -110,6 +117,7 @@ struct ossl_statem_st { unsigned int no_cert_verify; int use_timer; ENC_WRITE_STATES enc_write_state; + ENC_READ_STATES enc_read_state; }; typedef struct ossl_statem_st OSSL_STATEM; diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index caed61aaac..8a7d178a51 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -747,6 +747,12 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt) /* This is a real handshake so make sure we clean it up at the end */ if (s->server) { + /* + * To get this far we must have read encrypted data from the client. We + * no longer tolerate unencrypted alerts. This value is ignored if less + * than TLSv1.3 + */ + s->statem.enc_read_state = ENC_READ_STATE_VALID; if (s->post_handshake_auth != SSL_PHA_REQUESTED) s->statem.cleanuphand = 1; if (SSL_IS_TLS13(s) && !tls13_save_handshake_digest_for_pha(s)) { diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index eb9070ecc4..db5aafe3be 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -848,12 +848,7 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) return WORK_MORE_A; break; } - /* - * TODO(TLS1.3): This actually causes a problem. We don't yet know - * whether the next record we are going to receive is an unencrypted - * alert, or an encrypted handshake message. We're going to need - * something clever in the record layer for this. - */ + if (SSL_IS_TLS13(s)) { if (!s->method->ssl3_enc->setup_key_block(s) || !s->method->ssl3_enc->change_cipher_state(s, @@ -868,6 +863,12 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) /* SSLfatal() already called */ return WORK_ERROR; } + /* + * We don't yet know whether the next record we are going to receive + * is an unencrypted alert, an encrypted alert, or an encrypted + * handshake message. We temporarily tolerate unencrypted alerts. + */ + s->statem.enc_read_state = ENC_READ_STATE_ALLOW_PLAIN_ALERTS; break; } @@ -3523,6 +3524,13 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt) size_t chainidx; SSL_SESSION *new_sess = NULL; + /* + * To get this far we must have read encrypted data from the client. We no + * longer tolerate unencrypted alerts. This value is ignored if less than + * TLSv1.3 + */ + s->statem.enc_read_state = ENC_READ_STATE_VALID; + if ((sk = sk_X509_new_null()) == NULL) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_CLIENT_CERTIFICATE, ERR_R_MALLOC_FAILURE);