Limit the number of warning alerts silently consumed.

Per review comments on
https://boringssl-review.googlesource.com/#/c/4112/.

Change-Id: I82cacf67c6882e64f6637015ac41945522699797
Reviewed-on: https://boringssl-review.googlesource.com/5041
Reviewed-by: Adam Langley <agl@google.com>

crypto/err/ssl.errordata

@@ -349,6 +349,7 @@ SSL,reason,234,TLS_INVALID_ECPOINTFORMAT_LIST
 SSL,reason,235,TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST
 SSL,reason,236,TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG
 SSL,reason,237,TOO_MANY_EMPTY_FRAGMENTS
+SSL,reason,278,TOO_MANY_WARNING_ALERTS
 SSL,reason,238,UNABLE_TO_FIND_ECDH_PARAMETERS
 SSL,reason,239,UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS
 SSL,reason,240,UNEXPECTED_GROUP_CLOSE

include/openssl/ssl.h

@@ -2925,6 +2925,7 @@ OPENSSL_EXPORT const char *SSLeay_version(int unused);
 #define SSL_R_RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION 275
 #define SSL_R_EMS_STATE_INCONSISTENT 276
 #define SSL_R_RESUMED_NON_EMS_SESSION_WITH_EMS_EXTENSION 277
+#define SSL_R_TOO_MANY_WARNING_ALERTS 278
 #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

include/openssl/ssl3.h

@@ -423,6 +423,10 @@ typedef struct ssl3_state_st {
   /* empty_record_count is the number of consecutive empty records received. */
   uint8_t empty_record_count;
 
+  /* warning_alert_count is the number of consecutive warning alerts
+   * received. */
+  uint8_t warning_alert_count;
+
   /* State pertaining to the pending handshake.
    *
    * TODO(davidben): State is current spread all over the place. Move

ssl/s3_pkt.c

@@ -258,6 +258,10 @@ int ssl3_read_n(SSL *s, int n, int extend) {
  * forever. */
 static const uint8_t kMaxEmptyRecords = 32;
 
+/* kMaxWarningAlerts is the number of consecutive warning alerts that will be
+ * processed. */
+static const uint8_t kMaxWarningAlerts = 4;
+
 /* Call this to get a new input record. It will return <= 0 if more data is
  * needed, normally due to an error or non-blocking IO. When it finishes, one
  * packet has been decoded and can be found in
@@ -804,6 +808,8 @@ start:
   }
 
   if (type == rr->type) {
+    s->s3->warning_alert_count = 0;
+
     /* SSL3_RT_APPLICATION_DATA or SSL3_RT_HANDSHAKE */
     /* make sure that we are not getting application data when we are doing a
      * handshake for the first time */
@@ -963,6 +969,13 @@ start:
         OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_NO_RENEGOTIATION);
         goto f_err;
       }
+
+      s->s3->warning_alert_count++;
+      if (s->s3->warning_alert_count > kMaxWarningAlerts) {
+        al = SSL_AD_UNEXPECTED_MESSAGE;
+        OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_TOO_MANY_WARNING_ALERTS);
+        goto f_err;
+      }
     } else if (alert_level == SSL3_AL_FATAL) {
       char tmp[16];
 

ssl/test/runner/common.go

@@ -705,10 +705,6 @@ type ProtocolBugs struct {
 	// preferences to be ignored.
 	IgnorePeerCurvePreferences bool
 
-	// SendWarningAlerts, if non-zero, causes every record to be prefaced by
-	// a warning alert.
-	SendWarningAlerts alert
-
 	// BadFinished, if true, causes the Finished hash to be broken.
 	BadFinished bool
 

ssl/test/runner/conn.go

@@ -799,13 +799,8 @@ Again:
 
 // sendAlert sends a TLS alert message.
 // c.out.Mutex <= L.
-func (c *Conn) sendAlertLocked(err alert) error {
-	switch err {
-	case alertNoRenegotiation, alertCloseNotify:
-		c.tmp[0] = alertLevelWarning
-	default:
-		c.tmp[0] = alertLevelError
-	}
+func (c *Conn) sendAlertLocked(level byte, err alert) error {
+	c.tmp[0] = level
 	c.tmp[1] = byte(err)
 	if c.config.Bugs.FragmentAlert {
 		c.writeRecord(recordTypeAlert, c.tmp[0:1])
@@ -813,8 +808,8 @@ func (c *Conn) sendAlertLocked(err alert) error {
 	} else {
 		c.writeRecord(recordTypeAlert, c.tmp[0:2])
 	}
-	// closeNotify is a special case in that it isn't an error:
-	if err != alertCloseNotify {
+	// Error alerts are fatal to the connection.
+	if level == alertLevelError {
 		return c.out.setErrorLocked(&net.OpError{Op: "local error", Err: err})
 	}
 	return nil
@@ -823,9 +818,17 @@ func (c *Conn) sendAlertLocked(err alert) error {
 // sendAlert sends a TLS alert message.
 // L < c.out.Mutex.
 func (c *Conn) sendAlert(err alert) error {
+	level := byte(alertLevelError)
+	if err == alertNoRenegotiation || err == alertCloseNotify {
+		level = alertLevelWarning
+	}
+	return c.SendAlert(level, err)
+}
+
+func (c *Conn) SendAlert(level byte, err alert) error {
 	c.out.Lock()
 	defer c.out.Unlock()
-	return c.sendAlertLocked(err)
+	return c.sendAlertLocked(level, err)
 }
 
 // writeV2Record writes a record for a V2ClientHello.
@@ -841,13 +844,6 @@ func (c *Conn) writeV2Record(data []byte) (n int, err error) {
 // to the connection and updates the record layer state.
 // c.out.Mutex <= L.
 func (c *Conn) writeRecord(typ recordType, data []byte) (n int, err error) {
-	if typ != recordTypeAlert && c.config.Bugs.SendWarningAlerts != 0 {
-		alert := make([]byte, 2)
-		alert[0] = alertLevelWarning
-		alert[1] = byte(c.config.Bugs.SendWarningAlerts)
-		c.writeRecord(recordTypeAlert, alert)
-	}
-
 	if c.isDTLS {
 		return c.dtlsWriteRecord(typ, data)
 	}
@@ -1113,7 +1109,7 @@ func (c *Conn) Write(b []byte) (int, error) {
 	}
 
 	if c.config.Bugs.SendSpuriousAlert != 0 {
-		c.sendAlertLocked(c.config.Bugs.SendSpuriousAlert)
+		c.sendAlertLocked(alertLevelError, c.config.Bugs.SendSpuriousAlert)
 	}
 
 	// SSL 3.0 and TLS 1.0 are susceptible to a chosen-plaintext

ssl/test/runner/runner.go

@@ -207,6 +207,9 @@ type testCase struct {
 	// sendEmptyRecords is the number of consecutive empty records to send
 	// before and after the test message.
 	sendEmptyRecords int
+	// sendWarningAlerts is the number of consecutive warning alerts to send
+	// before and after the test message.
+	sendWarningAlerts int
 }
 
 var testCases = []testCase{
@@ -954,23 +957,6 @@ var testCases = []testCase{
 		shouldFail:    true,
 		expectedError: ":WRONG_CURVE:",
 	},
-	{
-		name: "SendWarningAlerts",
-		config: Config{
-			Bugs: ProtocolBugs{
-				SendWarningAlerts: alertAccessDenied,
-			},
-		},
-	},
-	{
-		protocol: dtls,
-		name:     "SendWarningAlerts-DTLS",
-		config: Config{
-			Bugs: ProtocolBugs{
-				SendWarningAlerts: alertAccessDenied,
-			},
-		},
-	},
 	{
 		name: "BadFinished",
 		config: Config{
@@ -1156,6 +1142,28 @@ var testCases = []testCase{
 		shouldFail:       true,
 		expectedError:    ":TOO_MANY_EMPTY_FRAGMENTS:",
 	},
+	{
+		name:              "SendWarningAlerts-Pass",
+		sendWarningAlerts: 4,
+	},
+	{
+		protocol:          dtls,
+		name:              "SendWarningAlerts-DTLS-Pass",
+		sendWarningAlerts: 4,
+	},
+	{
+		name:              "SendWarningAlerts",
+		sendWarningAlerts: 5,
+		shouldFail:        true,
+		expectedError:     ":TOO_MANY_WARNING_ALERTS:",
+	},
+	{
+		name:              "SendWarningAlerts-Async",
+		sendWarningAlerts: 5,
+		flags:             []string{"-async"},
+		shouldFail:        true,
+		expectedError:     ":TOO_MANY_WARNING_ALERTS:",
+	},
 }
 
 func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, isResume bool) error {
@@ -1295,6 +1303,10 @@ func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, i
 		tlsConn.Write(nil)
 	}
 
+	for i := 0; i < test.sendWarningAlerts; i++ {
+		tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage)
+	}
+
 	if test.renegotiate {
 		if test.renegotiateCiphers != nil {
 			config.CipherSuites = test.renegotiateCiphers
@@ -1334,6 +1346,10 @@ func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, i
 		tlsConn.Write(nil)
 	}
 
+	for i := 0; i < test.sendWarningAlerts; i++ {
+		tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage)
+	}
+
 	buf := make([]byte, len(testMessage))
 	if test.protocol == dtls {
 		bufTmp := make([]byte, len(buf)+1)