Clear the counters for empty or warning records only on actual data.

Fixes a minor issue where a peer can alternate between empty and
warning-only alert records, which applications might find confusing if
they try to account for activity of the connection at an upper layer.

Change-Id: I3103e0f4b8979c075eb7f3b5591ec0e66a6a6964
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/94607
Auto-Submit: Rudolf Polzer <rpolzer@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/test/runner/basic_tests.go

diff --git a/ssl/test/runner/basic_tests.go b/ssl/test/runner/basic_tests.go
index dfd17d5..54d4963 100644
--- a/ssl/test/runner/basic_tests.go
+++ b/ssl/test/runner/basic_tests.go
@@ -1116,6 +1116,34 @@
 			expectedError:     ":TOO_MANY_WARNING_ALERTS:",
 		},
 		{
+			name: "AlternateEmptyRecordsAndWarningAlerts",
+			config: Config{
+				MaxVersion: VersionTLS12,
+			},
+			sendEmptyRecords:  32,
+			sendWarningAlerts: 4,
+		},
+		{
+			name: "AlternateTooManyEmptyRecordsAndWarningAlerts",
+			config: Config{
+				MaxVersion: VersionTLS12,
+			},
+			sendEmptyRecords:  33,
+			sendWarningAlerts: 4,
+			shouldFail:        true,
+			expectedError:     ":TOO_MANY_EMPTY_FRAGMENTS:",
+		},
+		{
+			name: "AlternateEmptyRecordsAndTooManyWarningAlerts",
+			config: Config{
+				MaxVersion: VersionTLS12,
+			},
+			sendEmptyRecords:  32,
+			sendWarningAlerts: 5,
+			shouldFail:        true,
+			expectedError:     ":TOO_MANY_WARNING_ALERTS:",
+		},
+		{
 			name:               "SendBogusAlertType",
 			sendBogusAlertType: true,
 			shouldFail:         true,

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 43b3227..d10b8f1 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -632,9 +632,11 @@
 	testTLSUnique bool
 	// sendEmptyRecords is the number of consecutive empty records to send
 	// before each test message.
+	// If both this and `sendWarningAlerts` are set, they alternate at the end.
 	sendEmptyRecords int
 	// sendWarningAlerts is the number of consecutive warning alerts to send
 	// before each test message.
+	// If both this and `sendWarningAlerts` are set, they alternate at the end.
 	sendWarningAlerts int
 	// sendUserCanceledAlerts is the number of consecutive user_canceled alerts to
 	// send before each test message.
@@ -1193,15 +1195,17 @@
 			}
 		}
 
-		for i := 0; i < test.sendEmptyRecords; i++ {
-			if _, err := tlsConn.Write(nil); err != nil {
-				return err
+		// Count _down_, so that in case of differing values for the two counters, the alternating takes place at the _end_.
+		for i := max(test.sendEmptyRecords, test.sendWarningAlerts); i > 0; i-- {
+			if i <= test.sendEmptyRecords {
+				if _, err := tlsConn.Write(nil); err != nil {
+					return err
+				}
 			}
-		}
-
-		for i := 0; i < test.sendWarningAlerts; i++ {
-			if err := tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage); err != nil {
-				return err
+			if i <= test.sendWarningAlerts {
+				if err := tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage); err != nil {
+					return err
+				}
 			}
 		}
 

ssl/tls_record.cc

diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc
index ba0c4c1..83f5260 100644
--- a/ssl/tls_record.cc
+++ b/ssl/tls_record.cc
@@ -238,8 +238,6 @@
     }
     // Apart from the limit, empty records are returned up to the caller. This
     // allows the caller to reject records of the wrong type.
-  } else {
-    ssl->s3->empty_record_count = 0;
   }
 
   if (type == SSL3_RT_ALERT) {
@@ -254,7 +252,12 @@
     return ssl_open_record_error;
   }
 
-  ssl->s3->warning_alert_count = 0;
+  // Only when at least one byte is returned, clear the counters for empty
+  // records and warnings.
+  if (!out->empty()) {
+    ssl->s3->empty_record_count = 0;
+    ssl->s3->warning_alert_count = 0;
+  }
 
   *out_type = type;
   return ssl_open_record_success;