From 4792110b2bb1a5833e761e49f41b6c1fd83066aa Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 28 Jul 2016 11:29:18 -0400 Subject: [PATCH] Forbid interleaving app data in a HelloRequest. We already forbid renego/app-data interleave. Forbid it within a HelloRequest too because that's nonsense. No one would ever send: [hs:HelloReq-] [app:Hello world] [hs:-uest] Add tests for this case. This is in preparation for our more complex TLS 1.3 post-handshake logic which is going to go through the usual handshake reassembly logic and, for sanity, will want to enforce this anyway. BUG=83 Change-Id: I80eb9f3333da3d751f98f25d9469860d1993a97a Reviewed-on: https://boringssl-review.googlesource.com/9000 Reviewed-by: David Benjamin Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/s3_pkt.c | 3 ++- ssl/test/runner/conn.go | 14 ++++++++++++++ ssl/test/runner/runner.go | 21 +++++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index 060ef691..3e674e6f 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c @@ -411,7 +411,8 @@ start: /* we now have a packet which can be read and processed */ - if (type == rr->type) { + /* Do not allow interleaving application data and HelloRequest. */ + if (type == rr->type && ssl->s3->hello_request_len == 0) { /* Discard empty records. */ if (rr->length == 0) { goto start; diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index cefdde33..d01643c9 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go @@ -1274,6 +1274,20 @@ func (c *Conn) simulatePacketLoss(resendFunc func()) error { return nil } +func (c *Conn) SendHalfHelloRequest() error { + if err := c.Handshake(); err != nil { + return err + } + + c.out.Lock() + defer c.out.Unlock() + + if _, err := c.writeRecord(recordTypeHandshake, []byte{typeHelloRequest, 0}); err != nil { + return err + } + return c.flushHandshake() +} + // Write writes data to the connection. func (c *Conn) Write(b []byte) (int, error) { if err := c.Handshake(); err != nil { diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index bfd9faf3..35fe585e 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -301,6 +301,9 @@ type testCase struct { // renegotiate indicates the number of times the connection should be // renegotiated during the exchange. renegotiate int + // sendHalfHelloRequest, if true, causes the server to send half a + // HelloRequest when the handshake completes. + sendHalfHelloRequest bool // renegotiateCiphers is a list of ciphersuite ids that will be // switched in just before renegotiation. renegotiateCiphers []uint16 @@ -572,6 +575,10 @@ func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool) er tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage) } + if test.sendHalfHelloRequest { + tlsConn.SendHalfHelloRequest() + } + if test.renegotiate > 0 { if test.renegotiateCiphers != nil { config.CipherSuites = test.renegotiateCiphers @@ -3383,6 +3390,20 @@ func addStateMachineCoverageTests(config stateMachineTestConfig) { }, }) + tests = append(tests, testCase{ + name: "SendHalfHelloRequest", + config: Config{ + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + PackHelloRequestWithFinished: config.packHandshakeFlight, + }, + }, + sendHalfHelloRequest: true, + flags: []string{"-renegotiate-ignore"}, + shouldFail: true, + expectedError: ":UNEXPECTED_RECORD:", + }) + // NPN on client and server; results in post-handshake message. tests = append(tests, testCase{ name: "NPN-Client",