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 <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>

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

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,
+// SSL_shutdown shuts down |ssl|. It runs in two stages. First, it sends
-// it returns 0 if |ssl| completed uni-directional shutdown; close_notify has
+// close_notify and returns zero or one on success or -1 on failure. Zero
-// been sent, but the peer's close_notify has not been received. Most callers
+// indicates that close_notify was sent, but not received, and one additionally
-// may stop at this point. For bi-directional shutdown, call |SSL_shutdown|
+// indicates that the peer's close_notify had already been received.
-// again. It returns 1 if close_notify has been both sent and received.
 //
-// If the peer's close_notify arrived first, the first stage is skipped.
+// To then wait for the peer's close_notify, run |SSL_shutdown| to completion a
-// |SSL_shutdown| will return 1 once close_notify is sent and skip 0. Callers
+// second time. This returns 1 on success and -1 on failure. Application data
-// only interested in uni-directional shutdown must therefore allow for the
+// is considered a fatal error at this point. To process or discard it, read
-// first stage returning either 0 or 1.
+// until close_notify with |SSL_read| instead.
 //
-// |SSL_shutdown| returns -1 on failure. The caller should pass the return value
+// In both cases, on failure, pass the return value into |SSL_get_error| to
-// into |SSL_get_error| to determine how to proceed. If the underlying |BIO| is
+// determine how to proceed.
-// non-blocking, both stages may require retry.
+//
+// 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<const uint8_t> 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

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.
+      // Process records until an error, close_notify, or application data.
-      for (;;) {
+      if (ssl_read_impl(ssl) > 0) {
-        ssl->s3->pending_app_data = Span<uint8_t>();
+        // We received some unexpected application data.
-        int ret = ssl_read_impl(ssl);
+        OPENSSL_PUT_ERROR(SSL, SSL_R_APPLICATION_DATA_ON_SHUTDOWN);
-        if (ret <= 0) {
+        return -1;
-          break;
-        }
       }
       if (ssl->s3->read_shutdown != ssl_shutdown_close_notify) {
         return -1;

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,
 					},
 				},
 				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,
 					},
 				},
 				shimShutsDown: true,
+				renegotiate:   1,
 				shouldFail:    true,
 				expectedError: ":NO_RENEGOTIATION:",
 				flags: []string{