From 5267ef7b4aa6385c0351ae10ce3092828cca4909 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 30 May 2018 10:18:09 -0400 Subject: [PATCH] Reject unexpected application data in bidirectional shutdown. Update-Note: This tweaks the SSL_shutdown behavior. OpenSSL's original SSL_shutdown behavior was an incoherent mix of discarding the record and rejecting it (it would return SSL_ERROR_SYSCALL but retrying the operation would discard it). SSLeay appears to have intended to discard it, so we previously "fixed" it actually discard. However, this behavior is somewhat bizarre and means we skip over unbounded data, which we typically try to avoid. If you are trying to cleanly shutdown the TLS portion of your protocol, surely it is at a point where additional data is a syntax error. I suspect I originally did not realize that, because the discarded record did not properly continue the loop, SSL_shutdown would appear as if it rejected the data, and so it's unlikely anyone was relying on that behavior. Discussion in https://github.com/openssl/openssl/pull/6340 suggests (some of) upstream also prefers rejecting. Change-Id: Icde419049306ed17eb06ce1a7e1ff587901166f3 Reviewed-on: https://boringssl-review.googlesource.com/28864 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Steven Valdez --- crypto/err/ssl.errordata | 1 + include/openssl/ssl.h | 28 ++++++++++++++++------------ ssl/ssl_lib.cc | 12 +++++------- ssl/test/runner/runner.go | 33 +++++++++++++++++++++++++++------ 4 files changed, 49 insertions(+), 25 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index 9ce3a317..45eadad4 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata @@ -1,5 +1,6 @@ SSL,277,ALPN_MISMATCH_ON_EARLY_DATA SSL,281,APPLICATION_DATA_INSTEAD_OF_HANDSHAKE +SSL,291,APPLICATION_DATA_ON_SHUTDOWN SSL,100,APP_DATA_IN_HANDSHAKE SSL,101,ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT SSL,102,BAD_ALERT diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 1218a46f..7f6295ed 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -394,20 +394,23 @@ OPENSSL_EXPORT int SSL_pending(const SSL *ssl); // https://crbug.com/466303. OPENSSL_EXPORT int SSL_write(SSL *ssl, const void *buf, int num); -// SSL_shutdown shuts down |ssl|. On success, it completes in two stages. First, -// it returns 0 if |ssl| completed uni-directional shutdown; close_notify has -// been sent, but the peer's close_notify has not been received. Most callers -// may stop at this point. For bi-directional shutdown, call |SSL_shutdown| -// again. It returns 1 if close_notify has been both sent and received. +// SSL_shutdown shuts down |ssl|. It runs in two stages. First, it sends +// close_notify and returns zero or one on success or -1 on failure. Zero +// indicates that close_notify was sent, but not received, and one additionally +// indicates that the peer's close_notify had already been received. // -// If the peer's close_notify arrived first, the first stage is skipped. -// |SSL_shutdown| will return 1 once close_notify is sent and skip 0. Callers -// only interested in uni-directional shutdown must therefore allow for the -// first stage returning either 0 or 1. +// To then wait for the peer's close_notify, run |SSL_shutdown| to completion a +// second time. This returns 1 on success and -1 on failure. Application data +// is considered a fatal error at this point. To process or discard it, read +// until close_notify with |SSL_read| instead. // -// |SSL_shutdown| returns -1 on failure. The caller should pass the return value -// into |SSL_get_error| to determine how to proceed. If the underlying |BIO| is -// non-blocking, both stages may require retry. +// In both cases, on failure, pass the return value into |SSL_get_error| to +// determine how to proceed. +// +// Most callers should stop at the first stage. Reading for close_notify is +// primarily used for uncommon protocols where the underlying transport is +// reused after TLS completes. Additionally, DTLS uses an unordered transport +// and is unordered, so the second stage is a no-op in DTLS. OPENSSL_EXPORT int SSL_shutdown(SSL *ssl); // SSL_CTX_set_quiet_shutdown sets quiet shutdown on |ctx| to |mode|. If @@ -4911,6 +4914,7 @@ OPENSSL_EXPORT bool SSL_apply_handback(SSL *ssl, Span handback); #define SSL_R_SECOND_SERVERHELLO_VERSION_MISMATCH 288 #define SSL_R_OCSP_CB_ERROR 289 #define SSL_R_SSL_SESSION_ID_TOO_LONG 290 +#define SSL_R_APPLICATION_DATA_ON_SHUTDOWN 291 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020 diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index c6799b6b..8f969c57 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1229,13 +1229,11 @@ int SSL_shutdown(SSL *ssl) { } 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(); - int ret = ssl_read_impl(ssl); - if (ret <= 0) { - break; - } + // Process records until an error, close_notify, or application data. + if (ssl_read_impl(ssl) > 0) { + // We received some unexpected application data. + OPENSSL_PUT_ERROR(SSL, SSL_R_APPLICATION_DATA_ON_SHUTDOWN); + return -1; } if (ssl->s3->read_shutdown != ssl_shutdown_close_notify) { return -1; diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index e217ed8b..41fc5e4c 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -414,7 +414,8 @@ type testCase struct { readWithUnfinishedWrite bool // shimShutsDown, if true, runs a test where the shim shuts down the // connection immediately after the handshake rather than echoing - // messages from the runner. + // messages from the runner. The runner will default to not sending + // application data. shimShutsDown bool // renegotiate indicates the number of times the connection should be // renegotiated during the exchange. @@ -825,7 +826,8 @@ func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool, tr } messageCount := test.messageCount - if messageCount == 0 { + // shimShutsDown sets the default message count to zero. + if messageCount == 0 && !test.shimShutsDown { messageCount = 1 } @@ -5318,6 +5320,25 @@ read alert 1 0 flags: []string{"-check-close-notify"}, }) + // The shim should reject unexpected application data + // when shutting down. + tests = append(tests, testCase{ + name: "Shutdown-Shim-ApplicationData", + config: Config{ + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + ExpectCloseNotify: true, + }, + }, + shimShutsDown: true, + messageCount: 1, + sendEmptyRecords: 1, + sendWarningAlerts: 1, + flags: []string{"-check-close-notify"}, + shouldFail: true, + expectedError: ":APPLICATION_DATA_ON_SHUTDOWN:", + }) + // Test that SSL_shutdown still processes KeyUpdate. tests = append(tests, testCase{ name: "Shutdown-Shim-KeyUpdate", @@ -5358,11 +5379,11 @@ read alert 1 0 MinVersion: VersionTLS12, MaxVersion: VersionTLS12, Bugs: ProtocolBugs{ - SendHelloRequestBeforeEveryAppDataRecord: true, - ExpectCloseNotify: true, + ExpectCloseNotify: true, }, }, shimShutsDown: true, + renegotiate: 1, shouldFail: true, expectedError: ":NO_RENEGOTIATION:", flags: []string{"-check-close-notify"}, @@ -5373,11 +5394,11 @@ read alert 1 0 MinVersion: VersionTLS12, MaxVersion: VersionTLS12, Bugs: ProtocolBugs{ - SendHelloRequestBeforeEveryAppDataRecord: true, - ExpectCloseNotify: true, + ExpectCloseNotify: true, }, }, shimShutsDown: true, + renegotiate: 1, shouldFail: true, expectedError: ":NO_RENEGOTIATION:", flags: []string{