From 751d1a1c22631b506b3e9bab8ca7fd8bcc1f8edb Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 25 Sep 2017 13:37:30 -0400 Subject: [PATCH] Fold ssl_open_record_fatal_alert into ssl_open_record_error. The only difference is whether there's an alert to send back, but we'll need to allow an "error without alert" in several cases anyway: 1. If the server sees an HTTP request or garbage instead of a ClientHello, it shouldn't send an alert. 2. Resurfaced errors. Just make zero signal no alert for now. Later on, I'm thinking we might just want to put the alert into the outgoing buffer and make it further uniform. This also gives us only one error state to keep track of rather than two. Bug: 206 Change-Id: Ia821d9f89abd2ca6010e8851220d4e070bc42fa1 Reviewed-on: https://boringssl-review.googlesource.com/21286 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Steven Valdez --- include/openssl/ssl.h | 4 +--- ssl/d1_pkt.cc | 7 +++---- ssl/internal.h | 9 ++++----- ssl/s3_pkt.cc | 8 ++++---- ssl/tls_record.cc | 5 ++--- 5 files changed, 14 insertions(+), 19 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 6b4bf818..032ec7e9 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4651,7 +4651,6 @@ enum class OpenRecordResult { kDiscard, kIncompleteRecord, kAlertCloseNotify, - kAlertFatal, kError, }; @@ -4665,9 +4664,8 @@ enum class OpenRecordResult { // - kIncompleteRecord if |in| did not contain a complete record. // - kAlertCloseNotify if a record was successfully processed but is a // close_notify alert. -// - kAlertFatal if a record was successfully processed but is a fatal alert. // - kError if an error occurred or the record is invalid. |*out_alert| will be -// set to an alert to emit. +// set to an alert to emit, or zero if no alert should be emitted. OPENSSL_EXPORT OpenRecordResult OpenRecord(SSL *ssl, Span *out, size_t *out_record_len, uint8_t *out_alert, diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc index a3095e18..91fc647e 100644 --- a/ssl/d1_pkt.cc +++ b/ssl/d1_pkt.cc @@ -192,11 +192,10 @@ again: case ssl_open_record_close_notify: return 0; - case ssl_open_record_fatal_alert: - return -1; - case ssl_open_record_error: - ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); + if (alert != 0) { + ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); + } return -1; } diff --git a/ssl/internal.h b/ssl/internal.h index 32199aa5..b9a597c5 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -764,7 +764,6 @@ enum ssl_open_record_t { ssl_open_record_discard, ssl_open_record_partial, ssl_open_record_close_notify, - ssl_open_record_fatal_alert, ssl_open_record_error, }; @@ -786,11 +785,11 @@ enum ssl_open_record_t { // If a record was successfully processed but should be discarded, it returns // |ssl_open_record_discard|. // -// If a record was successfully processed but is a close_notify or fatal alert, -// it returns |ssl_open_record_close_notify| or |ssl_open_record_fatal_alert|. +// If a record was successfully processed but is a close_notify, it returns +// |ssl_open_record_close_notify|. // -// On failure, it returns |ssl_open_record_error| and sets |*out_alert| to an -// alert to emit. +// On failure or fatal alert, it returns |ssl_open_record_error| and sets +// |*out_alert| to an alert to emit, or zero if no alert should be emitted. enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, CBS *out, size_t *out_consumed, uint8_t *out_alert, uint8_t *in, size_t in_len); diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc index f7470aeb..5334145f 100644 --- a/ssl/s3_pkt.cc +++ b/ssl/s3_pkt.cc @@ -178,11 +178,10 @@ again: case ssl_open_record_close_notify: return 0; - case ssl_open_record_fatal_alert: - return -1; - case ssl_open_record_error: - ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); + if (alert != 0) { + ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); + } return -1; } @@ -547,6 +546,7 @@ int ssl3_send_alert(SSL *ssl, int level, int desc) { ssl->s3->send_shutdown = ssl_shutdown_close_notify; } else { assert(level == SSL3_AL_FATAL); + assert(desc != SSL_AD_CLOSE_NOTIFY); ssl->s3->send_shutdown = ssl_shutdown_fatal_alert; } diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc index 5eeff3c5..f9033e6c 100644 --- a/ssl/tls_record.cc +++ b/ssl/tls_record.cc @@ -562,7 +562,8 @@ enum ssl_open_record_t ssl_process_alert(SSL *ssl, uint8_t *out_alert, OPENSSL_PUT_ERROR(SSL, SSL_AD_REASON_OFFSET + alert_descr); BIO_snprintf(tmp, sizeof(tmp), "%d", alert_descr); ERR_add_error_data(2, "SSL alert number ", tmp); - return ssl_open_record_fatal_alert; + *out_alert = 0; // No alert to send back to the peer. + return ssl_open_record_error; } *out_alert = SSL_AD_ILLEGAL_PARAMETER; @@ -603,8 +604,6 @@ OpenRecordResult OpenRecord(SSL *ssl, Span *out, return OpenRecordResult::kIncompleteRecord; case ssl_open_record_close_notify: return OpenRecordResult::kAlertCloseNotify; - case ssl_open_record_fatal_alert: - return OpenRecordResult::kAlertFatal; case ssl_open_record_error: return OpenRecordResult::kError; }