Fix DTLS handling of multiple records in a packet.

9a41d1b946 broke handling of multiple records in
a single packet. If |extend| is true, not all of the previous packet should be
consumed, only up to the record length.

Add a test which stresses the DTLS stack's handling of multiple handshake
fragments in a handshake record and multiple handshake records in a packet.

Change-Id: I96571098ad9001e96440501c4730325227b155b8
Reviewed-on: https://boringssl-review.googlesource.com/4950
Reviewed-by: Adam Langley <agl@google.com>

ssl/s3_pkt.c

@@ -177,11 +177,10 @@ int ssl3_read_n(SSL *s, int n, int extend) {
     /* ... now we can act as if 'extend' was set */
   }
 
-  /* For DTLS/UDP reads should not span multiple packets because the read
-   * operation returns the whole packet at once (as long as it fits into the
-   * buffer). Moreover, if |extend| is true, we must not read another packet,
-   * even if the entire packet was consumed. */
-  if (SSL_IS_DTLS(s) && ((left > 0 && n > left) || extend)) {
+  /* In DTLS, if there is leftover data from the previous packet or |extend| is
+   * true, clamp to the previous read. DTLS records may not span packet
+   * boundaries. */
+  if (SSL_IS_DTLS(s) && n > left && (left > 0 || extend)) {
     n = left;
   }
 

ssl/test/runner/common.go

@@ -478,7 +478,9 @@ type ProtocolBugs struct {
 	// MaxHandshakeRecordLength, if non-zero, is the maximum size of a
 	// handshake record. Handshake messages will be split into multiple
 	// records at the specified size, except that the client_version will
-	// never be fragmented.
+	// never be fragmented. For DTLS, it is the maximum handshake fragment
+	// size, not record size; DTLS allows multiple handshake fragments in a
+	// single handshake record. See |PackHandshakeFragments|.
 	MaxHandshakeRecordLength int
 
 	// FragmentClientVersion will allow MaxHandshakeRecordLength to apply to
@@ -712,6 +714,15 @@ type ProtocolBugs struct {
 	// DHGroupPrime, if not nil, is used to define the (finite field)
 	// Diffie-Hellman group. The generator used is always two.
 	DHGroupPrime *big.Int
+
+	// PackHandshakeFragments, if true, causes handshake fragments to be
+	// packed into individual handshake records, up to the specified record
+	// size.
+	PackHandshakeFragments int
+
+	// PackHandshakeRecords, if true, causes handshake records to be packed
+	// into individual packets, up to the specified packet size.
+	PackHandshakeRecords int
 }
 
 func (c *Config) serverInit() {

ssl/test/runner/dtls.go

@@ -196,6 +196,8 @@ func (c *Conn) dtlsFlushHandshake() error {
 		return nil
 	}
 
+	// This is a test-only DTLS implementation, so there is no need to
+	// retain |c.pendingFragments| for a future retransmit.
 	var fragments [][]byte
 	fragments, c.pendingFragments = c.pendingFragments, fragments
 
@@ -208,38 +210,66 @@ func (c *Conn) dtlsFlushHandshake() error {
 		fragments = tmp
 	}
 
-	// Send them all.
+	maxRecordLen := c.config.Bugs.PackHandshakeFragments
+	maxPacketLen := c.config.Bugs.PackHandshakeRecords
+
+	// Pack handshake fragments into records.
+	var records [][]byte
 	for _, fragment := range fragments {
 		if c.config.Bugs.SplitFragmentHeader {
-			if _, err := c.dtlsWriteRawRecord(recordTypeHandshake, fragment[:2]); err != nil {
-				return err
-			}
-			fragment = fragment[2:]
-		} else if c.config.Bugs.SplitFragmentBody && len(fragment) > 12 {
-			if _, err := c.dtlsWriteRawRecord(recordTypeHandshake, fragment[:13]); err != nil {
-				return err
+			records = append(records, fragment[:2])
+			records = append(records, fragment[2:])
+		} else if c.config.Bugs.SplitFragmentBody {
+			if len(fragment) > 12 {
+				records = append(records, fragment[:13])
+				records = append(records, fragment[13:])
+			} else {
+				records = append(records, fragment)
 			}
-			fragment = fragment[13:]
+		} else if i := len(records) - 1; len(records) > 0 && len(records[i])+len(fragment) <= maxRecordLen {
+			records[i] = append(records[i], fragment...)
+		} else {
+			// The fragment will be appended to, so copy it.
+			records = append(records, append([]byte{}, fragment...))
+		}
+	}
+
+	// Format them into packets.
+	var packets [][]byte
+	for _, record := range records {
+		b, err := c.dtlsSealRecord(recordTypeHandshake, record)
+		if err != nil {
+			return err
+		}
+
+		if i := len(packets) - 1; len(packets) > 0 && len(packets[i])+len(b.data) <= maxPacketLen {
+			packets[i] = append(packets[i], b.data...)
+		} else {
+			// The sealed record will be appended to and reused by
+			// |c.out|, so copy it.
+			packets = append(packets, append([]byte{}, b.data...))
 		}
+		c.out.freeBlock(b)
+	}
 
-		// TODO(davidben): A real DTLS implementation needs to
-		// retransmit handshake messages. For testing purposes, we don't
-		// actually care.
-		if _, err := c.dtlsWriteRawRecord(recordTypeHandshake, fragment); err != nil {
+	// Send all the packets.
+	for _, packet := range packets {
+		if _, err := c.conn.Write(packet); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func (c *Conn) dtlsWriteRawRecord(typ recordType, data []byte) (n int, err error) {
+// dtlsSealRecord seals a record into a block from |c.out|'s pool.
+func (c *Conn) dtlsSealRecord(typ recordType, data []byte) (b *block, err error) {
 	recordHeaderLen := dtlsRecordHeaderLen
 	maxLen := c.config.Bugs.MaxHandshakeRecordLength
 	if maxLen <= 0 {
 		maxLen = 1024
 	}
 
-	b := c.out.newBlock()
+	b = c.out.newBlock()
 
 	explicitIVLen := 0
 	explicitIVIsSeq := false
@@ -286,6 +316,14 @@ func (c *Conn) dtlsWriteRawRecord(typ recordType, data []byte) (n int, err error
 	}
 	copy(b.data[recordHeaderLen+explicitIVLen:], data)
 	c.out.encrypt(b, explicitIVLen)
+	return
+}
+
+func (c *Conn) dtlsWriteRawRecord(typ recordType, data []byte) (n int, err error) {
+	b, err := c.dtlsSealRecord(typ, data)
+	if err != nil {
+		return
+	}
 
 	_, err = c.conn.Write(b.data)
 	if err != nil {

ssl/test/runner/runner.go

@@ -1105,6 +1105,17 @@ var testCases = []testCase{
 		},
 		flags: []string{"-async"},
 	},
+	{
+		protocol: dtls,
+		name:     "PackDTLSHandshake",
+		config: Config{
+			Bugs: ProtocolBugs{
+				MaxHandshakeRecordLength: 2,
+				PackHandshakeFragments:   20,
+				PackHandshakeRecords:     200,
+			},
+		},
+	},
 }
 
 func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, isResume bool) error {