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>

7 files changed, 62 insertions, 40 deletions

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index ab6e7f4..af3d55f 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2794,6 +2794,7 @@ OPENSSL_EXPORT void ERR_load_SSL_strings(void);
 #define SSL_R_HANDSHAKE_RECORD_BEFORE_CCS 441
 #define SSL_R_SESSION_MAY_NOT_BE_CREATED 442
 #define SSL_R_INVALID_SSL_SESSION 443
+#define SSL_R_BAD_ALERT 444
 #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 5083167..42e2154 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -372,10 +372,8 @@ typedef struct ssl3_state_st
 	SSL3_RECORD rrec;	/* each decoded record goes in here */
 	SSL3_RECORD wrec;	/* goes out from here */
 
-	/* storage for Alert/Handshake protocol data received but not
+	/* storage for Handshake protocol data received but not
 	 * yet processed by ssl3_read_bytes: */
-	unsigned char alert_fragment[2];
-	unsigned int alert_fragment_len;
 	unsigned char handshake_fragment[4];
 	unsigned int handshake_fragment_len;
 
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 @@ int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
 	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 @@ start:
 	/* In case of record types for which we have 'fragment' storage,
 	 * fill that so that we can process the data at a fixed place.
 	 */
-		{
-		unsigned int dest_maxlen = 0;
-		unsigned char *dest = NULL;
-		unsigned int *dest_len = NULL;
 
-		if (rr->type == SSL3_RT_HANDSHAKE)
-			{
-			dest_maxlen = sizeof s->s3->handshake_fragment;
-			dest = s->s3->handshake_fragment;
-			dest_len = &s->s3->handshake_fragment_len;
-			}
-		else if (rr->type == SSL3_RT_ALERT)
+	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->alert_fragment;
-			dest = s->s3->alert_fragment;
-			dest_len = &s->s3->alert_fragment_len;
+			goto start; /* fragment was too small */
 			}
-
-		if (dest_maxlen > 0)
+		}
+	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))
 			{
-			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 */
+			al = SSL_AD_DECODE_ERROR;
+			OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_BAD_ALERT);
+			goto f_err;
 			}
+		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 @@ start:
 		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 @@ const ERR_STRING_DATA SSL_error_string_data[] = {
   {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 @@ type ProtocolBugs struct {
 	// 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 @@ func (c *Conn) sendAlertLocked(err alert) error {
 		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 @@ func (c *Conn) Write(b []byte) (int, error) {
 		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 @@ var testCases = []testCase{
 	},
 	{
 		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{