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>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index 4ae0a51..e4c6e59 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -349,6 +349,7 @@
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
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 217dbaf..97485c7 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2925,6 +2925,7 @@
#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
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index 5cc0acc..f6c8972 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -423,6 +423,10 @@
/* 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
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 9599f59..bfe7f4a 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -258,6 +258,10 @@
* 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 @@
}
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 @@
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];
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index edebba1..d1b7db7 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -705,10 +705,6 @@
// 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
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index 84741fb..ea9f3bb 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -799,13 +799,8 @@
// 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 @@
} 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 @@
// 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 @@
// 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 @@
}
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
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index d7ae850..d17b048 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -207,6 +207,9 @@
// 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{
@@ -955,23 +958,6 @@
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{
Bugs: ProtocolBugs{
@@ -1156,6 +1142,28 @@
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 @@
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 @@
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)