Revert "Pack encrypted handshake messages together."

This reverts commit 75d43b5. Chatting
with EKR, there is some reason to believe that doing this might cause
more middlebox issues. Since we're still in the middle of working
towards viable deployment in the first place, revert this.

We can experiment with this later. I should have arranged for this to be
controlled more carefully anyway.

Change-Id: I0c8bf578f9d7364e913894e1bf3c2b8123dfd770
Reviewed-on: https://boringssl-review.googlesource.com/22204
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

ssl/internal.h

@@ -1010,9 +1010,6 @@ bool tls_has_unprocessed_handshake_data(const SSL *ssl);
 // |tls_has_unprocessed_handshake_data| for DTLS.
 bool dtls_has_unprocessed_handshake_data(const SSL *ssl);
 
-// tls_flush_pending_hs_data flushes any handshake plaintext data.
-bool tls_flush_pending_hs_data(SSL *ssl);
-
 struct DTLS_OUTGOING_MESSAGE {
   DTLS_OUTGOING_MESSAGE() {}
   DTLS_OUTGOING_MESSAGE(const DTLS_OUTGOING_MESSAGE &) = delete;
@@ -2276,11 +2273,6 @@ struct SSL3_STATE {
   // hs_buf is the buffer of handshake data to process.
   UniquePtr<BUF_MEM> hs_buf;
 
-  // pending_hs_data contains the pending handshake data that has not yet
-  // been encrypted to |pending_flight|. This allows packing the handshake into
-  // fewer records.
-  UniquePtr<BUF_MEM> pending_hs_data;
-
   // pending_flight is the pending outgoing flight. This is used to flush each
   // handshake flight in a single write. |write_buffer| must be written out
   // before this data.

ssl/s3_both.cc

@@ -134,8 +134,6 @@ namespace bssl {
 
 static bool add_record_to_flight(SSL *ssl, uint8_t type,
                                  Span<const uint8_t> in) {
-  // The caller should have flushed |pending_hs_data| first.
-  assert(!ssl->s3->pending_hs_data);
   // We'll never add a flight while in the process of writing it out.
   assert(ssl->s3->pending_flight_offset == 0);
 
@@ -184,49 +182,17 @@ bool ssl3_finish_message(SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg) {
 }
 
 bool ssl3_add_message(SSL *ssl, Array<uint8_t> msg) {
-  // Pack handshake data into the minimal number of records. This avoids
-  // unnecessary encryption overhead, notably in TLS 1.3 where we send several
-  // encrypted messages in a row. For now, we do not do this for the null
-  // cipher. The benefit is smaller and there is a risk of breaking buggy
-  // implementations.
-  //
-  // TODO(davidben): See if we can do this uniformly.
+  // Add the message to the current flight, splitting into several records if
+  // needed.
   Span<const uint8_t> rest = msg;
-  if (ssl->s3->aead_write_ctx->is_null_cipher()) {
-    while (!rest.empty()) {
-      Span<const uint8_t> chunk = rest.subspan(0, ssl->max_send_fragment);
-      rest = rest.subspan(chunk.size());
+  do {
+    Span<const uint8_t> chunk = rest.subspan(0, ssl->max_send_fragment);
+    rest = rest.subspan(chunk.size());
 
-      if (!add_record_to_flight(ssl, SSL3_RT_HANDSHAKE, chunk)) {
-        return false;
-      }
-    }
-  } else {
-    while (!rest.empty()) {
-      // Flush if |pending_hs_data| is full.
-      if (ssl->s3->pending_hs_data &&
-          ssl->s3->pending_hs_data->length >= ssl->max_send_fragment &&
-          !tls_flush_pending_hs_data(ssl)) {
-        return false;
-      }
-
-      size_t pending_len =
-          ssl->s3->pending_hs_data ? ssl->s3->pending_hs_data->length : 0;
-      Span<const uint8_t> chunk =
-          rest.subspan(0, ssl->max_send_fragment - pending_len);
-      assert(!chunk.empty());
-      rest = rest.subspan(chunk.size());
-
-      if (!ssl->s3->pending_hs_data) {
-        ssl->s3->pending_hs_data.reset(BUF_MEM_new());
-      }
-      if (!ssl->s3->pending_hs_data ||
-          !BUF_MEM_append(ssl->s3->pending_hs_data.get(), chunk.data(),
-                          chunk.size())) {
-        return false;
-      }
+    if (!add_record_to_flight(ssl, SSL3_RT_HANDSHAKE, chunk)) {
+      return false;
     }
-  }
+  } while (!rest.empty());
 
   ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HANDSHAKE, msg);
   // TODO(svaldez): Move this up a layer to fix abstraction for SSLTranscript on
@@ -238,23 +204,10 @@ bool ssl3_add_message(SSL *ssl, Array<uint8_t> msg) {
   return true;
 }
 
-bool tls_flush_pending_hs_data(SSL *ssl) {
-  if (!ssl->s3->pending_hs_data || ssl->s3->pending_hs_data->length == 0) {
-    return true;
-  }
-
-  UniquePtr<BUF_MEM> pending_hs_data = std::move(ssl->s3->pending_hs_data);
-  return add_record_to_flight(
-      ssl, SSL3_RT_HANDSHAKE,
-      MakeConstSpan(reinterpret_cast<const uint8_t *>(pending_hs_data->data),
-                    pending_hs_data->length));
-}
-
 bool ssl3_add_change_cipher_spec(SSL *ssl) {
   static const uint8_t kChangeCipherSpec[1] = {SSL3_MT_CCS};
 
-  if (!tls_flush_pending_hs_data(ssl) ||
-      !add_record_to_flight(ssl, SSL3_RT_CHANGE_CIPHER_SPEC,
+  if (!add_record_to_flight(ssl, SSL3_RT_CHANGE_CIPHER_SPEC,
                             kChangeCipherSpec)) {
     return false;
   }
@@ -266,8 +219,7 @@ bool ssl3_add_change_cipher_spec(SSL *ssl) {
 
 bool ssl3_add_alert(SSL *ssl, uint8_t level, uint8_t desc) {
   uint8_t alert[2] = {level, desc};
-  if (!tls_flush_pending_hs_data(ssl) ||
-      !add_record_to_flight(ssl, SSL3_RT_ALERT, alert)) {
+  if (!add_record_to_flight(ssl, SSL3_RT_ALERT, alert)) {
     return false;
   }
 
@@ -277,10 +229,6 @@ bool ssl3_add_alert(SSL *ssl, uint8_t level, uint8_t desc) {
 }
 
 int ssl3_flush_flight(SSL *ssl) {
-  if (!tls_flush_pending_hs_data(ssl)) {
-    return -1;
-  }
-
   if (ssl->s3->pending_flight == nullptr) {
     return 1;
   }

ssl/s3_pkt.cc

@@ -234,9 +234,6 @@ static int do_ssl3_write(SSL *ssl, int type, const uint8_t *in, unsigned len) {
     return 0;
   }
 
-  if (!tls_flush_pending_hs_data(ssl)) {
-    return -1;
-  }
   size_t flight_len = 0;
   if (ssl->s3->pending_flight != nullptr) {
     flight_len =

ssl/test/runner/common.go

@@ -1416,11 +1416,6 @@ type ProtocolBugs struct {
 	// length accepted from the peer.
 	MaxReceivePlaintext int
 
-	// ExpectPackedEncryptedHandshake, if non-zero, requires that the peer maximally
-	// pack their encrypted handshake messages, fitting at most the
-	// specified number of plaintext bytes per record.
-	ExpectPackedEncryptedHandshake int
-
 	// SendTicketLifetime, if non-zero, is the ticket lifetime to send in
 	// NewSessionTicket messages.
 	SendTicketLifetime time.Duration

ssl/test/runner/conn.go

@@ -101,11 +101,6 @@ type Conn struct {
 	keyUpdateRequested bool
 	seenOneByteRecord  bool
 
-	// seenHandshakePackEnd is whether the most recent handshake record was
-	// not full for ExpectPackedEncryptedHandshake. If true, no more
-	// handshake data may be received until the next flight or epoch change.
-	seenHandshakePackEnd bool
-
 	tmp [16]byte
 }
 
@@ -743,7 +738,6 @@ func (c *Conn) useInTrafficSecret(version uint16, suite *cipherSuite, secret []b
 		side = clientWrite
 	}
 	c.in.useTrafficSecret(version, suite, secret, side)
-	c.seenHandshakePackEnd = false
 	return nil
 }
 
@@ -919,13 +913,6 @@ Again:
 		return c.in.setErrorLocked(err)
 	}
 
-	if typ != recordTypeHandshake {
-		c.seenHandshakePackEnd = false
-	} else if c.seenHandshakePackEnd {
-		c.in.freeBlock(b)
-		return c.in.setErrorLocked(errors.New("tls: peer violated ExpectPackedEncryptedHandshake"))
-	}
-
 	switch typ {
 	default:
 		c.in.setErrorLocked(c.sendAlert(alertUnexpectedMessage))
@@ -990,9 +977,6 @@ Again:
 			return c.in.setErrorLocked(c.sendAlert(alertNoRenegotiation))
 		}
 		c.hand.Write(data)
-		if pack := c.config.Bugs.ExpectPackedEncryptedHandshake; pack > 0 && len(data) < pack && c.out.cipher != nil {
-			c.seenHandshakePackEnd = true
-		}
 	}
 
 	if b != nil {
@@ -1051,7 +1035,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) {
-	c.seenHandshakePackEnd = false
 	if typ == recordTypeHandshake {
 		msgType := data[0]
 		if c.config.Bugs.SendWrongMessageType != 0 && msgType == c.config.Bugs.SendWrongMessageType {

ssl/test/runner/runner.go

@@ -2806,8 +2806,7 @@ read alert 1 0
 			config: Config{
 				MaxVersion: VersionTLS13,
 				Bugs: ProtocolBugs{
-					MaxReceivePlaintext:            512,
-					ExpectPackedEncryptedHandshake: 512,
+					MaxReceivePlaintext: 512,
 				},
 			},
 			messageLen: 1024,
@@ -2837,18 +2836,6 @@ read alert 1 0
 			shouldFail:         true,
 			expectedLocalError: "local error: record overflow",
 		},
-		{
-			// Test that, by default, handshake data is tightly
-			// packed in TLS 1.3.
-			testType: serverTest,
-			name:     "PackedEncryptedHandshake-TLS13",
-			config: Config{
-				MaxVersion: VersionTLS13,
-				Bugs: ProtocolBugs{
-					ExpectPackedEncryptedHandshake: 16384,
-				},
-			},
-		},
 		{
 			// Test that DTLS can handle multiple application data
 			// records in a single packet.

ssl/tls_method.cc

@@ -97,10 +97,6 @@ static bool ssl3_set_read_state(SSL *ssl, UniquePtr<SSLAEADContext> aead_ctx) {
 }
 
 static bool ssl3_set_write_state(SSL *ssl, UniquePtr<SSLAEADContext> aead_ctx) {
-  if (!tls_flush_pending_hs_data(ssl)) {
-    return false;
-  }
-
   OPENSSL_memset(ssl->s3->write_sequence, 0, sizeof(ssl->s3->write_sequence));
   ssl->s3->aead_write_ctx = std::move(aead_ctx);
   return true;