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{