Don't allow alert records with multiple alerts.
This is just kind of a silly thing to do. NSS doesn't allow them either. Fatal
alerts would kill the connection regardless and warning alerts are useless. We
previously stopped accepting fragmented alerts but still allowed them doubled
up.
This is in preparation for pulling the shared alert processing code between TLS
and DTLS out of read_bytes into some common place.
Change-Id: Idbef04e39ad135f9601f5686d41f54531981e0cf
Reviewed-on: https://boringssl-review.googlesource.com/7451
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 4a356b0..72ef8ac 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -326,11 +326,10 @@
/* If we get here, then type != rr->type. */
- /* If an alert record, process one alert out of the record. Note that we allow
- * a single record to contain multiple alerts. */
+ /* If an alert record, process the alert. */
if (rr->type == SSL3_RT_ALERT) {
- /* Alerts may not be fragmented. */
- if (rr->length < 2) {
+ /* Alerts records may not contain fragmented or multiple alerts. */
+ if (rr->length != 2) {
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ALERT);
goto f_err;
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index a1a59b2..56b0145 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -519,11 +519,10 @@
goto start;
}
- /* If an alert record, process one alert out of the record. Note that we allow
- * a single record to contain multiple alerts. */
+ /* If an alert record, process the alert. */
if (rr->type == SSL3_RT_ALERT) {
- /* Alerts may not be fragmented. */
- if (rr->length < 2) {
+ /* Alerts records may not contain fragmented or multiple alerts. */
+ if (rr->length != 2) {
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ALERT);
goto f_err;
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 74c552e..2e9ce04 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -525,6 +525,10 @@
// two records.
FragmentAlert bool
+ // DoubleAlert will cause all alerts to be sent as two copies packed
+ // within one record.
+ DoubleAlert bool
+
// SendSpuriousAlert, if non-zero, will cause an spurious, unwanted
// alert to be sent.
SendSpuriousAlert alert
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index fe0b139..43548e8 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -842,6 +842,9 @@
if c.config.Bugs.FragmentAlert {
c.writeRecord(recordTypeAlert, c.tmp[0:1])
c.writeRecord(recordTypeAlert, c.tmp[1:2])
+ } else if c.config.Bugs.DoubleAlert {
+ copy(c.tmp[2:4], c.tmp[0:2])
+ c.writeRecord(recordTypeAlert, c.tmp[0:4])
} else {
c.writeRecord(recordTypeAlert, c.tmp[0:2])
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 9100752..5b746c6 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1206,6 +1206,31 @@
},
{
testType: serverTest,
+ name: "DoubleAlert",
+ config: Config{
+ Bugs: ProtocolBugs{
+ DoubleAlert: true,
+ SendSpuriousAlert: alertRecordOverflow,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":BAD_ALERT:",
+ },
+ {
+ protocol: dtls,
+ testType: serverTest,
+ name: "DoubleAlert-DTLS",
+ config: Config{
+ Bugs: ProtocolBugs{
+ DoubleAlert: true,
+ SendSpuriousAlert: alertRecordOverflow,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":BAD_ALERT:",
+ },
+ {
+ testType: serverTest,
name: "EarlyChangeCipherSpec-server-1",
config: Config{
Bugs: ProtocolBugs{
