Replace open_close_notify with open_app_data.
While a fairly small hook, open_close_notify is pretty weird. It
processes things at the record level and not above. Notably, this will
break if it skips past a TLS 1.3 KeyUpdate.
Instead, it can share the core part of SSL_read/SSL_peek, with slight
tweaks to post-handshake processing. Note this does require some tweaks
to that code. Notably, to retain the current semantics that SSL_shutdown
does not call funny callbacks, we suppress tickets.
Change-Id: Ia0cbd0b9f4527f1b091dd2083a5d8c7efb2bac65
Reviewed-on: https://boringssl-review.googlesource.com/21885
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc
index e2a8e6a..c6be93d 100644
--- a/ssl/d1_pkt.cc
+++ b/ssl/d1_pkt.cc
@@ -187,27 +187,6 @@
return ssl_open_record_success;
}
-ssl_open_record_t dtls1_open_close_notify(SSL *ssl, size_t *out_consumed,
- uint8_t *out_alert,
- bssl::Span<uint8_t> in) {
- switch (ssl->s3->read_shutdown) {
- // Bidirectional shutdown doesn't make sense for an unordered transport.
- // DTLS alerts also aren't delivered reliably, so we may even time out
- // because the peer never received our close_notify. Report to the caller
- // that the channel has fully shut down.
- case ssl_shutdown_none:
- case ssl_shutdown_close_notify:
- ssl->s3->read_shutdown = ssl_shutdown_close_notify;
- return ssl_open_record_close_notify;
- case ssl_shutdown_error:
- ERR_restore_state(ssl->s3->read_error);
- *out_alert = 0;
- return ssl_open_record_error;
- }
- assert(0);
- return ssl_open_record_error;
-}
-
int dtls1_write_app_data(SSL *ssl, bool *out_needs_handshake,
const uint8_t *buf, int len) {
assert(!SSL_in_init(ssl));
diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc
index ac06842..fab19be 100644
--- a/ssl/dtls_method.cc
+++ b/ssl/dtls_method.cc
@@ -121,7 +121,6 @@
dtls1_open_handshake,
dtls1_open_change_cipher_spec,
dtls1_open_app_data,
- dtls1_open_close_notify,
dtls1_write_app_data,
dtls1_dispatch_alert,
dtls1_supports_cipher,
diff --git a/ssl/internal.h b/ssl/internal.h
index 6bbde76..7801cc5 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1754,11 +1754,6 @@
ssl_open_record_t (*open_app_data)(SSL *ssl, Span<uint8_t> *out,
size_t *out_consumed, uint8_t *out_alert,
Span<uint8_t> in);
- // open_close_notify processes a record from |in| for reading close_notify.
- // It discards all records and returns |ssl_open_record_close_notify| when it
- // receives one.
- ssl_open_record_t (*open_close_notify)(SSL *ssl, size_t *out_consumed,
- uint8_t *out_alert, Span<uint8_t> in);
int (*write_app_data)(SSL *ssl, bool *out_needs_handshake, const uint8_t *buf,
int len);
int (*dispatch_alert)(SSL *ssl);
@@ -2711,8 +2706,6 @@
ssl_open_record_t ssl3_open_change_cipher_spec(SSL *ssl, size_t *out_consumed,
uint8_t *out_alert,
Span<uint8_t> in);
-ssl_open_record_t ssl3_open_close_notify(SSL *ssl, size_t *out_consumed,
- uint8_t *out_alert, Span<uint8_t> in);
int ssl3_write_app_data(SSL *ssl, bool *out_needs_handshake, const uint8_t *buf,
int len);
@@ -2747,8 +2740,6 @@
ssl_open_record_t dtls1_open_change_cipher_spec(SSL *ssl, size_t *out_consumed,
uint8_t *out_alert,
Span<uint8_t> in);
-ssl_open_record_t dtls1_open_close_notify(SSL *ssl, size_t *out_consumed,
- uint8_t *out_alert, Span<uint8_t> in);
int dtls1_write_app_data(SSL *ssl, bool *out_needs_handshake,
const uint8_t *buf, int len);
diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc
index e647d06..7966e6f 100644
--- a/ssl/s3_pkt.cc
+++ b/ssl/s3_pkt.cc
@@ -390,19 +390,6 @@
return ssl_open_record_success;
}
-ssl_open_record_t ssl3_open_close_notify(SSL *ssl, size_t *out_consumed,
- uint8_t *out_alert, Span<uint8_t> in) {
- // TODO(davidben): Replace this with open_app_data so we actually process
- // various bad behaviors.
- uint8_t type;
- Span<uint8_t> body;
- auto ret = tls_open_record(ssl, &type, &body, out_consumed, out_alert, in);
- if (ret == ssl_open_record_success) {
- return ssl_open_record_discard;
- }
- return ret;
-}
-
int ssl_send_alert(SSL *ssl, int level, int desc) {
// It is illegal to send an alert when we've already sent a closing one.
if (ssl->s3->write_shutdown != ssl_shutdown_none) {
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 8e06c49..ee7406d 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -884,7 +884,8 @@
// protocol, namely in HTTPS, just before reading the HTTP response. Require
// the record-layer be idle and avoid complexities of sending a handshake
// record while an application_data record is being written.
- if (ssl_write_buffer_is_pending(ssl)) {
+ if (ssl_write_buffer_is_pending(ssl) ||
+ ssl->s3->write_shutdown != ssl_shutdown_none) {
goto no_renegotiation;
}
@@ -902,12 +903,12 @@
return 1;
no_renegotiation:
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
return 0;
}
-static int ssl_read_impl(SSL *ssl, void *buf, int num, bool peek) {
+static int ssl_read_impl(SSL *ssl) {
ssl_reset_error_state(ssl);
if (ssl->do_handshake == NULL) {
@@ -915,7 +916,7 @@
return -1;
}
- for (;;) {
+ while (ssl->s3->pending_app_data.empty()) {
// Complete the current handshake, if any. False Start will cause
// |SSL_do_handshake| to return mid-handshake, so this may require multiple
// iterations.
@@ -941,48 +942,52 @@
continue; // Loop again. We may have begun a new handshake.
}
- if (ssl->s3->pending_app_data.empty()) {
- uint8_t alert = SSL_AD_DECODE_ERROR;
- size_t consumed = 0;
- auto ret =
- ssl->method->open_app_data(ssl, &ssl->s3->pending_app_data, &consumed,
- &alert, ssl_read_buffer(ssl));
- bool retry;
- int bio_ret = ssl_handle_open_record(ssl, &retry, ret, consumed, alert);
- if (bio_ret <= 0) {
- return bio_ret;
- }
- if (retry) {
- continue;
- }
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ size_t consumed = 0;
+ auto ret =
+ ssl->method->open_app_data(ssl, &ssl->s3->pending_app_data, &consumed,
+ &alert, ssl_read_buffer(ssl));
+ bool retry;
+ int bio_ret = ssl_handle_open_record(ssl, &retry, ret, consumed, alert);
+ if (bio_ret <= 0) {
+ return bio_ret;
+ }
+ if (!retry) {
+ assert(!ssl->s3->pending_app_data.empty());
ssl->s3->key_update_count = 0;
}
-
- if (num <= 0) {
- return num;
- }
-
- size_t todo =
- std::min(ssl->s3->pending_app_data.size(), static_cast<size_t>(num));
- OPENSSL_memcpy(buf, ssl->s3->pending_app_data.data(), todo);
- if (!peek) {
- // TODO(davidben): In DTLS, should the rest of the record be discarded?
- // DTLS is not a stream. See https://crbug.com/boringssl/65.
- ssl->s3->pending_app_data = ssl->s3->pending_app_data.subspan(todo);
- if (ssl->s3->pending_app_data.empty()) {
- ssl_read_buffer_discard(ssl);
- }
- }
- return static_cast<int>(todo);
}
+
+ return 1;
}
int SSL_read(SSL *ssl, void *buf, int num) {
- return ssl_read_impl(ssl, buf, num, false /* consume bytes */);
+ int ret = SSL_peek(ssl, buf, num);
+ if (ret <= 0) {
+ return ret;
+ }
+ // TODO(davidben): In DTLS, should the rest of the record be discarded? DTLS
+ // is not a stream. See https://crbug.com/boringssl/65.
+ ssl->s3->pending_app_data =
+ ssl->s3->pending_app_data.subspan(static_cast<size_t>(ret));
+ if (ssl->s3->pending_app_data.empty()) {
+ ssl_read_buffer_discard(ssl);
+ }
+ return ret;
}
int SSL_peek(SSL *ssl, void *buf, int num) {
- return ssl_read_impl(ssl, buf, num, true /* peek */);
+ int ret = ssl_read_impl(ssl);
+ if (ret <= 0) {
+ return ret;
+ }
+ if (num <= 0) {
+ return num;
+ }
+ size_t todo =
+ std::min(ssl->s3->pending_app_data.size(), static_cast<size_t>(num));
+ OPENSSL_memcpy(buf, ssl->s3->pending_app_data.data(), todo);
+ return static_cast<int>(todo);
}
int SSL_write(SSL *ssl, const void *buf, int num) {
@@ -1056,21 +1061,28 @@
return -1;
}
} else if (ssl->s3->read_shutdown != ssl_shutdown_close_notify) {
- ssl->s3->pending_app_data = Span<uint8_t>();
- for (;;) {
- uint8_t alert = SSL_AD_DECODE_ERROR;
- size_t consumed = 0;
- auto ret = ssl->method->open_close_notify(ssl, &consumed, &alert,
- ssl_read_buffer(ssl));
- bool retry;
- int bio_ret = ssl_handle_open_record(ssl, &retry, ret, consumed, alert);
- if (bio_ret <= 0) {
- break;
+ if (SSL_is_dtls(ssl)) {
+ // Bidirectional shutdown doesn't make sense for an unordered
+ // transport. DTLS alerts also aren't delivered reliably, so we may even
+ // time out because the peer never received our close_notify. Report to
+ // the caller that the channel has fully shut down.
+ if (ssl->s3->read_shutdown == ssl_shutdown_error) {
+ ERR_restore_state(ssl->s3->read_error);
+ return -1;
}
- assert(retry); // open_close_notify never reports success.
- }
- if (ssl->s3->read_shutdown != ssl_shutdown_close_notify) {
- return -1;
+ ssl->s3->read_shutdown = ssl_shutdown_close_notify;
+ } else {
+ // Keep discarding data until we see a close_notify.
+ for (;;) {
+ ssl->s3->pending_app_data = Span<uint8_t>();
+ int ret = ssl_read_impl(ssl);
+ if (ret <= 0) {
+ break;
+ }
+ }
+ if (ssl->s3->read_shutdown != ssl_shutdown_close_notify) {
+ return -1;
+ }
}
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index f475b73..434dfdd 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -3930,6 +3930,39 @@
EXPECT_EQ(3, SSL_pending(client_.get()));
}
+// Test that post-handshake tickets consumed by |SSL_shutdown| are ignored.
+TEST(SSLTest, ShutdownIgnoresTickets) {
+ bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(ctx);
+ ASSERT_TRUE(SSL_CTX_set_min_proto_version(ctx.get(), TLS1_3_VERSION));
+ ASSERT_TRUE(SSL_CTX_set_max_proto_version(ctx.get(), TLS1_3_VERSION));
+
+ bssl::UniquePtr<X509> cert = GetTestCertificate();
+ bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+ ASSERT_TRUE(cert);
+ ASSERT_TRUE(key);
+ ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), cert.get()));
+ ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key.get()));
+
+ SSL_CTX_set_session_cache_mode(ctx.get(), SSL_SESS_CACHE_BOTH);
+
+ bssl::UniquePtr<SSL> client, server;
+ ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get()));
+
+ SSL_CTX_sess_set_new_cb(ctx.get(), [](SSL *ssl, SSL_SESSION *session) -> int {
+ ADD_FAILURE() << "New session callback called during SSL_shutdown";
+ return 0;
+ });
+
+ // Send close_notify.
+ EXPECT_EQ(0, SSL_shutdown(server.get()));
+ EXPECT_EQ(0, SSL_shutdown(client.get()));
+
+ // Receive close_notify.
+ EXPECT_EQ(1, SSL_shutdown(server.get()));
+ EXPECT_EQ(1, SSL_shutdown(client.get()));
+}
+
// TODO(davidben): Convert this file to GTest properly.
TEST(SSLTest, AllTests) {
if (!TestSSL_SESSIONEncoding(kOpenSSLSession) ||
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index a57362d..065dee8 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -4894,6 +4894,92 @@
sendWarningAlerts: 1,
flags: []string{"-check-close-notify"},
})
+
+ // Test that SSL_shutdown still processes KeyUpdate.
+ tests = append(tests, testCase{
+ name: "Shutdown-Shim-KeyUpdate",
+ config: Config{
+ MinVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ ExpectCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ sendKeyUpdates: 1,
+ keyUpdateRequest: keyUpdateRequested,
+ flags: []string{"-check-close-notify"},
+ })
+
+ // Test that SSL_shutdown processes HelloRequest
+ // correctly.
+ tests = append(tests, testCase{
+ name: "Shutdown-Shim-HelloRequest-Ignore",
+ config: Config{
+ MinVersion: VersionTLS12,
+ MaxVersion: VersionTLS12,
+ Bugs: ProtocolBugs{
+ SendHelloRequestBeforeEveryAppDataRecord: true,
+ ExpectCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ flags: []string{
+ "-renegotiate-ignore",
+ "-check-close-notify",
+ },
+ })
+ tests = append(tests, testCase{
+ name: "Shutdown-Shim-HelloRequest-Reject",
+ config: Config{
+ MinVersion: VersionTLS12,
+ MaxVersion: VersionTLS12,
+ Bugs: ProtocolBugs{
+ SendHelloRequestBeforeEveryAppDataRecord: true,
+ ExpectCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ shouldFail: true,
+ expectedError: ":NO_RENEGOTIATION:",
+ flags: []string{"-check-close-notify"},
+ })
+ tests = append(tests, testCase{
+ name: "Shutdown-Shim-HelloRequest-CannotHandshake",
+ config: Config{
+ MinVersion: VersionTLS12,
+ MaxVersion: VersionTLS12,
+ Bugs: ProtocolBugs{
+ SendHelloRequestBeforeEveryAppDataRecord: true,
+ ExpectCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ shouldFail: true,
+ expectedError: ":NO_RENEGOTIATION:",
+ flags: []string{
+ "-check-close-notify",
+ "-renegotiate-freely",
+ },
+ })
+
+ tests = append(tests, testCase{
+ testType: serverTest,
+ name: "Shutdown-Shim-Renegotiate-Server-Forbidden",
+ config: Config{
+ MaxVersion: VersionTLS12,
+ Bugs: ProtocolBugs{
+ ExpectCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ renegotiate: 1,
+ shouldFail: true,
+ expectedError: ":NO_RENEGOTIATION:",
+ flags: []string{
+ "-check-close-notify",
+ },
+ })
}
} else {
// TODO(davidben): DTLS 1.3 will want a similar thing for
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index e75d976..a03c581 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -774,6 +774,13 @@
}
int tls13_process_new_session_ticket(SSL *ssl, const SSLMessage &msg) {
+ if (ssl->s3->write_shutdown != ssl_shutdown_none) {
+ // Ignore tickets on shutdown. Callers tend to indiscriminately call
+ // |SSL_shutdown| before destroying an |SSL|, at which point calling the new
+ // session callback may be confusing.
+ return 1;
+ }
+
UniquePtr<SSL_SESSION> session(SSL_SESSION_dup(ssl->s3->established_session,
SSL_SESSION_INCLUDE_NONAUTH));
if (!session) {
diff --git a/ssl/tls_method.cc b/ssl/tls_method.cc
index c7352ce..94b9e20 100644
--- a/ssl/tls_method.cc
+++ b/ssl/tls_method.cc
@@ -116,7 +116,6 @@
ssl3_open_handshake,
ssl3_open_change_cipher_spec,
ssl3_open_app_data,
- ssl3_open_close_notify,
ssl3_write_app_data,
ssl3_dispatch_alert,
ssl3_supports_cipher,
