Add tests for packed handshake records in TLS. I'm surprised we'd never tested this. In addition to splitting handshake records up, one may pack multiple handshakes into a single record, as they fit. Generalize the DTLS handshake flush hook to do this in TLS as well. Change-Id: Ia546d18c7c56ba45e50f489c5b53e1fcd6404f51 Reviewed-on: https://boringssl-review.googlesource.com/8650 Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 0e5d9e4..eabd0c6 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -767,15 +767,19 @@ // 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, if true, causes handshake fragments in DTLS + // 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, if true, causes handshake records in DTLS to be + // packed into individual packets, up to the specified packet size. PackHandshakeRecords int + // PackHandshakeFlight, if true, causes each handshake flight in TLS to + // be packed into records, up to the largest size record available. + PackHandshakeFlight bool + // EnableAllCiphers, if true, causes all configured ciphers to be // enabled. EnableAllCiphers bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index 551c6bc..c33ac0c 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go
@@ -77,6 +77,10 @@ input *block // application record waiting to be read hand bytes.Buffer // handshake record waiting to be read + // pendingFlight, if PackHandshakeFlight is enabled, is the buffer of + // handshake data to be split into records at the end of the flight. + pendingFlight bytes.Buffer + // DTLS state sendHandshakeSeq uint16 recvHandshakeSeq uint16 @@ -934,6 +938,15 @@ return c.dtlsWriteRecord(typ, data) } + if c.config.Bugs.PackHandshakeFlight && typ == recordTypeHandshake { + c.pendingFlight.Write(data) + return len(data), nil + } + + return c.doWriteRecord(typ, data) +} + +func (c *Conn) doWriteRecord(typ recordType, data []byte) (n int, err error) { recordHeaderLen := tlsRecordHeaderLen b := c.out.newBlock() first := true @@ -1031,6 +1044,23 @@ return } +func (c *Conn) flushHandshake() error { + if c.isDTLS { + return c.dtlsFlushHandshake() + } + + for c.pendingFlight.Len() > 0 { + var buf [maxPlaintext]byte + n, _ := c.pendingFlight.Read(buf[:]) + if _, err := c.doWriteRecord(recordTypeHandshake, buf[:n]); err != nil { + return err + } + } + + c.pendingFlight.Reset() + return nil +} + func (c *Conn) doReadHandshake() ([]byte, error) { if c.isDTLS { return c.dtlsDoReadHandshake() @@ -1217,6 +1247,7 @@ if c.config.Bugs.SendHelloRequestBeforeEveryAppDataRecord { c.writeRecord(recordTypeHandshake, []byte{typeHelloRequest, 0, 0, 0}) + c.flushHandshake() } // SSL 3.0 and TLS 1.0 are susceptible to a chosen-plaintext @@ -1269,6 +1300,7 @@ helloReq = c.config.Bugs.BadHelloRequest } c.writeRecord(recordTypeHandshake, helloReq) + c.flushHandshake() } c.handshakeComplete = false
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go index f4c5f71..48b8a1b 100644 --- a/ssl/test/runner/dtls.go +++ b/ssl/test/runner/dtls.go
@@ -242,10 +242,6 @@ } func (c *Conn) dtlsFlushHandshake() error { - if !c.isDTLS { - 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
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 8e8f7d7..a2f6f65 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -225,7 +225,7 @@ helloBytes = hello.marshal() c.writeRecord(recordTypeHandshake, helloBytes) } - c.dtlsFlushHandshake() + c.flushHandshake() if err := c.simulatePacketLoss(nil); err != nil { return err @@ -249,7 +249,7 @@ hello.cookie = helloVerifyRequest.cookie helloBytes = hello.marshal() c.writeRecord(recordTypeHandshake, helloBytes) - c.dtlsFlushHandshake() + c.flushHandshake() if err := c.simulatePacketLoss(nil); err != nil { return err @@ -342,7 +342,7 @@ // Finished. if err := c.simulatePacketLoss(func() { c.writeRecord(recordTypeHandshake, hs.finishedBytes) - c.dtlsFlushHandshake() + c.flushHandshake() }); err != nil { return err } @@ -549,7 +549,7 @@ hs.writeClientHash(certVerify.marshal()) c.writeRecord(recordTypeHandshake, certVerify.marshal()) } - c.dtlsFlushHandshake() + c.flushHandshake() hs.finishedHash.discardHandshakeBuffer() @@ -893,7 +893,7 @@ c.writeRecord(recordTypeHandshake, postCCSBytes[:5]) postCCSBytes = postCCSBytes[5:] } - c.dtlsFlushHandshake() + c.flushHandshake() if !c.config.Bugs.SkipChangeCipherSpec && c.config.Bugs.EarlyChangeCipherSpec == 0 { @@ -914,7 +914,7 @@ if !c.config.Bugs.SkipFinished { c.writeRecord(recordTypeHandshake, postCCSBytes) - c.dtlsFlushHandshake() + c.flushHandshake() } return nil }
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 94f5923..771beea 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go
@@ -76,7 +76,7 @@ // Finished. if err := c.simulatePacketLoss(func() { c.writeRecord(recordTypeHandshake, hs.finishedBytes) - c.dtlsFlushHandshake() + c.flushHandshake() }); err != nil { return err } @@ -154,7 +154,7 @@ return false, errors.New("dtls: short read from Rand: " + err.Error()) } c.writeRecord(recordTypeHandshake, helloVerifyRequest.marshal()) - c.dtlsFlushHandshake() + c.flushHandshake() if err := c.simulatePacketLoss(nil); err != nil { return false, err @@ -614,7 +614,7 @@ helloDone := new(serverHelloDoneMsg) hs.writeServerHash(helloDone.marshal()) c.writeRecord(recordTypeHandshake, helloDone.marshal()) - c.dtlsFlushHandshake() + c.flushHandshake() var pub crypto.PublicKey // public key for client auth, if any @@ -894,7 +894,7 @@ c.writeRecord(recordTypeHandshake, postCCSBytes[:5]) postCCSBytes = postCCSBytes[5:] } - c.dtlsFlushHandshake() + c.flushHandshake() if !c.config.Bugs.SkipChangeCipherSpec { ccs := []byte{1} @@ -914,7 +914,7 @@ if !c.config.Bugs.SkipFinished { c.writeRecord(recordTypeHandshake, postCCSBytes) - c.dtlsFlushHandshake() + c.flushHandshake() } c.cipherSuite = hs.suite
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 7596485..b8f7088 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -2918,10 +2918,39 @@ } } +type stateMachineTestConfig struct { + protocol protocol + async bool + splitHandshake, packHandshakeFlight bool +} + // Adds tests that try to cover the range of the handshake state machine, under // various conditions. Some of these are redundant with other tests, but they // only cover the synchronous case. -func addStateMachineCoverageTests(async, splitHandshake bool, protocol protocol) { +func addAllStateMachineCoverageTests() { + for _, async := range []bool{false, true} { + for _, protocol := range []protocol{tls, dtls} { + addStateMachineCoverageTests(stateMachineTestConfig{ + protocol: protocol, + async: async, + }) + addStateMachineCoverageTests(stateMachineTestConfig{ + protocol: protocol, + async: async, + splitHandshake: true, + }) + if protocol == tls { + addStateMachineCoverageTests(stateMachineTestConfig{ + protocol: protocol, + async: async, + packHandshakeFlight: true, + }) + } + } + } +} + +func addStateMachineCoverageTests(config stateMachineTestConfig) { var tests []testCase // Basic handshake, with resumption. Client and server, @@ -3024,7 +3053,7 @@ // Setting SSL_VERIFY_PEER allows anonymous clients. flags: []string{"-verify-peer"}, }) - if protocol == tls { + if config.protocol == tls { tests = append(tests, testCase{ testType: clientTest, name: "ClientAuth-NoCertificate-Client-SSL3", @@ -3247,7 +3276,7 @@ }, }) - if protocol == tls { + if config.protocol == tls { tests = append(tests, testCase{ name: "Renegotiate-Client", config: Config{ @@ -3486,24 +3515,28 @@ } for _, test := range tests { - test.protocol = protocol - if protocol == dtls { + test.protocol = config.protocol + if config.protocol == dtls { test.name += "-DTLS" } - if async { + if config.async { test.name += "-Async" test.flags = append(test.flags, "-async") } else { test.name += "-Sync" } - if splitHandshake { + if config.splitHandshake { test.name += "-SplitHandshakeRecords" test.config.Bugs.MaxHandshakeRecordLength = 1 - if protocol == dtls { + if config.protocol == dtls { test.config.Bugs.MaxPacketLength = 256 test.flags = append(test.flags, "-mtu", "256") } } + if config.packHandshakeFlight { + test.name += "-PackHandshakeFlight" + test.config.Bugs.PackHandshakeFlight = true + } testCases = append(testCases, test) } } @@ -5856,13 +5889,7 @@ addCECPQ1Tests() addKeyExchangeInfoTests() addTLS13RecordTests() - for _, async := range []bool{false, true} { - for _, splitHandshake := range []bool{false, true} { - for _, protocol := range []protocol{tls, dtls} { - addStateMachineCoverageTests(async, splitHandshake, protocol) - } - } - } + addAllStateMachineCoverageTests() var wg sync.WaitGroup