Remove support for processing fragmented alerts
Prior to this change, BoringSSL maintained a 2-byte buffer for alerts,
and would support reassembly of fragmented alerts.
NSS does not support fragmented alerts, nor would any reasonable
implementation produce them. Remove fragmented alert handling and
produce an error if a fragmented alert has ever been encountered.
Change-Id: I31530ac372e8a90b47cf89404630c1c207cfb048
Reviewed-on: https://boringssl-review.googlesource.com/2125
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index bfe27cc..3ccb0a0d 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -944,6 +944,7 @@
unsigned int n;
SSL3_RECORD *rr;
void (*cb)(const SSL *ssl,int type2,int val)=NULL;
+ uint8_t alert_buffer[2];
if (s->s3->rbuf.buf == NULL) /* Not initialized yet */
if (!ssl3_setup_read_buffer(s))
@@ -1077,44 +1078,39 @@
/* In case of record types for which we have 'fragment' storage,
* fill that so that we can process the data at a fixed place.
*/
+
+ if (rr->type == SSL3_RT_HANDSHAKE)
{
- unsigned int dest_maxlen = 0;
- unsigned char *dest = NULL;
- unsigned int *dest_len = NULL;
-
- if (rr->type == SSL3_RT_HANDSHAKE)
+ const size_t size = sizeof(s->s3->handshake_fragment);
+ const size_t avail = size - s->s3->handshake_fragment_len;
+ const size_t len = (rr->length < avail) ? rr->length : avail;
+ memcpy(s->s3->handshake_fragment + s->s3->handshake_fragment_len,
+ &rr->data[rr->off], len);
+ rr->off += len;
+ rr->length -= len;
+ s->s3->handshake_fragment_len += len;
+ if (s->s3->handshake_fragment_len < size)
{
- dest_maxlen = sizeof s->s3->handshake_fragment;
- dest = s->s3->handshake_fragment;
- dest_len = &s->s3->handshake_fragment_len;
+ goto start; /* fragment was too small */
}
- else if (rr->type == SSL3_RT_ALERT)
+ }
+ else if (rr->type == SSL3_RT_ALERT)
+ {
+ const size_t len = sizeof(alert_buffer);
+ /* Note that this will still allow multiple alerts to
+ * be processed in the same record */
+ if (rr->length < sizeof(alert_buffer))
{
- dest_maxlen = sizeof s->s3->alert_fragment;
- dest = s->s3->alert_fragment;
- dest_len = &s->s3->alert_fragment_len;
+ al = SSL_AD_DECODE_ERROR;
+ OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_BAD_ALERT);
+ goto f_err;
}
-
- if (dest_maxlen > 0)
- {
- n = dest_maxlen - *dest_len; /* available space in 'dest' */
- if (rr->length < n)
- n = rr->length; /* available bytes */
-
- /* now move 'n' bytes: */
- while (n-- > 0)
- {
- dest[(*dest_len)++] = rr->data[rr->off++];
- rr->length--;
- }
-
- if (*dest_len < dest_maxlen)
- goto start; /* fragment was too small */
- }
+ memcpy(alert_buffer, &rr->data[rr->off], len);
+ rr->off += len;
+ rr->length -= len;
}
/* s->s3->handshake_fragment_len == 4 iff rr->type == SSL3_RT_HANDSHAKE;
- * s->s3->alert_fragment_len == 2 iff rr->type == SSL3_RT_ALERT.
* (Possibly rr is 'empty' now, i.e. rr->length may be 0.) */
/* If we are a client, check for an incoming 'Hello Request': */
@@ -1158,15 +1154,13 @@
goto start;
}
- if (s->s3->alert_fragment_len >= 2)
+ if (rr->type == SSL3_RT_ALERT)
{
- int alert_level = s->s3->alert_fragment[0];
- int alert_descr = s->s3->alert_fragment[1];
-
- s->s3->alert_fragment_len = 0;
+ uint8_t alert_level = alert_buffer[0];
+ uint8_t alert_descr = alert_buffer[1];
if (s->msg_callback)
- s->msg_callback(0, s->version, SSL3_RT_ALERT, s->s3->alert_fragment, 2, s, s->msg_callback_arg);
+ s->msg_callback(0, s->version, SSL3_RT_ALERT, alert_buffer, 2, s, s->msg_callback_arg);
if (s->info_callback != NULL)
cb=s->info_callback;
diff --git a/ssl/ssl_error.c b/ssl/ssl_error.c
index b070b5f..1737b1f 100644
--- a/ssl/ssl_error.c
+++ b/ssl/ssl_error.c
@@ -211,6 +211,7 @@
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_APP_DATA_IN_HANDSHAKE), "APP_DATA_IN_HANDSHAKE"},
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT), "ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT"},
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_AUTHZ_DATA_TOO_LARGE), "AUTHZ_DATA_TOO_LARGE"},
+ {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_ALERT), "BAD_ALERT"},
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_ALERT_RECORD), "BAD_ALERT_RECORD"},
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_AUTHENTICATION_TYPE), "BAD_AUTHENTICATION_TYPE"},
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_CHANGE_CIPHER_SPEC), "BAD_CHANGE_CIPHER_SPEC"},
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 6130343..9f79778 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -431,6 +431,13 @@
// the first 6 bytes of the ClientHello.
FragmentClientVersion bool
+ // FragmentAlert will cause all alerts to be fragmented across
+ // two records.
+ FragmentAlert bool
+
+ // SendSpuriousAlert will cause an spurious, unwanted alert to be sent.
+ SendSpuriousAlert bool
+
// RsaClientKeyExchangeVersion, if non-zero, causes the client to send a
// ClientKeyExchange with the specified version rather than the
// client_version when performing the RSA key exchange.
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index e1ccbb7..f4b4c36 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -769,7 +769,12 @@
c.tmp[0] = alertLevelError
}
c.tmp[1] = byte(err)
- c.writeRecord(recordTypeAlert, c.tmp[0:2])
+ if c.config.Bugs.FragmentAlert {
+ c.writeRecord(recordTypeAlert, c.tmp[0:1])
+ c.writeRecord(recordTypeAlert, c.tmp[1:2])
+ } else {
+ c.writeRecord(recordTypeAlert, c.tmp[0:2])
+ }
// closeNotify is a special case in that it isn't an error:
if err != alertCloseNotify {
return c.out.setErrorLocked(&net.OpError{Op: "local error", Err: err})
@@ -1001,6 +1006,10 @@
return 0, alertInternalError
}
+ if c.config.Bugs.SendSpuriousAlert {
+ c.sendAlertLocked(alertRecordOverflow)
+ }
+
// SSL 3.0 and TLS 1.0 are susceptible to a chosen-plaintext
// attack when using block mode ciphers due to predictable IVs.
// This can be prevented by splitting each Application Data
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 9172223..a302687 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -354,6 +354,18 @@
},
{
testType: serverTest,
+ name: "FragmentAlert",
+ config: Config{
+ Bugs: ProtocolBugs{
+ FragmentAlert: true,
+ SendSpuriousAlert: true,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":BAD_ALERT:",
+ },
+ {
+ testType: serverTest,
name: "EarlyChangeCipherSpec-server-1",
config: Config{
Bugs: ProtocolBugs{
