From 3c6a1ea6747a026e74cf2c2dc179ccffa03f3e57 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 26 Sep 2016 18:30:05 -0400 Subject: [PATCH] Apply version/wire mapping at a higher layer in runner. This is in preparation for implementing the version extension and is probably what we should have done from the beginning as it makes intolerance bugs simpler. This means knobs like SendClientVersion and SendServerVersion deal with the wire values while knobs like NegotiateVersion and MaxVersion deal with logical versions. (This matches how the bugs have always worked. SendFoo is just a weird post-processing bit on the handshake messages while NegotiateVersion actually changes how BoGo behaves.) BUG=90 Change-Id: I7f359d798d0899fa2742107fb3d854be19e731a4 Reviewed-on: https://boringssl-review.googlesource.com/11300 Reviewed-by: Steven Valdez Reviewed-by: David Benjamin Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/test/runner/common.go | 6 ++--- ssl/test/runner/handshake_client.go | 21 +++++++++--------- ssl/test/runner/handshake_messages.go | 32 +++++++++++++-------------- ssl/test/runner/handshake_server.go | 15 +++++++------ ssl/test/runner/key_agreement.go | 5 ++--- ssl/test/runner/runner.go | 8 +++---- 6 files changed, 43 insertions(+), 44 deletions(-) diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 02ab23b049..7aa984e775 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -597,8 +597,8 @@ type ProtocolBugs struct { // send a NewSessionTicket message during an abbreviated handshake. RenewTicketOnResume bool - // SendClientVersion, if non-zero, causes the client to send a different - // TLS version in the ClientHello than the maximum supported version. + // SendClientVersion, if non-zero, causes the client to send the + // specified value in the ClientHello version field. SendClientVersion uint16 // NegotiateVersion, if non-zero, causes the server to negotiate the @@ -1040,7 +1040,7 @@ type ProtocolBugs struct { SecondHelloRetryRequest bool // SendServerHelloVersion, if non-zero, causes the server to send the - // specified version in ServerHello rather than the true version. + // specified value in ServerHello version field. SendServerHelloVersion uint16 // SkipHelloRetryRequest, if true, causes the TLS 1.3 server to not send diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 05cf0c2730..bdddfb425d 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -57,9 +57,10 @@ func (c *Conn) clientHandshake() error { return errors.New("tls: NextProtos values too large") } + maxVersion := c.config.maxVersion(c.isDTLS) hello := &clientHelloMsg{ isDTLS: c.isDTLS, - vers: c.config.maxVersion(c.isDTLS), + vers: versionToWire(maxVersion, c.isDTLS), compressionMethods: []uint8{compressionNone}, random: make([]byte, 32), ocspStapling: true, @@ -110,7 +111,7 @@ func (c *Conn) clientHandshake() error { } var keyShares map[CurveID]ecdhCurve - if hello.vers >= VersionTLS13 { + if maxVersion >= VersionTLS13 { keyShares = make(map[CurveID]ecdhCurve) hello.hasKeyShares = true hello.trailingKeyShareData = c.config.Bugs.TrailingKeyShareData @@ -163,7 +164,7 @@ NextCipherSuite: if !c.config.Bugs.EnableAllCiphers { // Don't advertise TLS 1.2-only cipher suites unless // we're attempting TLS 1.2. - if hello.vers < VersionTLS12 && suite.flags&suiteTLS12 != 0 { + if maxVersion < VersionTLS12 && suite.flags&suiteTLS12 != 0 { continue } // Don't advertise non-DTLS cipher suites in DTLS. @@ -190,7 +191,7 @@ NextCipherSuite: return errors.New("tls: short read from Rand: " + err.Error()) } - if hello.vers >= VersionTLS12 && !c.config.Bugs.NoSignatureAlgorithms { + if maxVersion >= VersionTLS12 && !c.config.Bugs.NoSignatureAlgorithms { hello.signatureAlgorithms = c.config.verifySignatureAlgorithms() } @@ -352,19 +353,19 @@ NextCipherSuite: } } - var serverVersion uint16 + var serverWireVersion uint16 switch m := msg.(type) { case *helloRetryRequestMsg: - serverVersion = m.vers + serverWireVersion = m.vers case *serverHelloMsg: - serverVersion = m.vers + serverWireVersion = m.vers default: c.sendAlert(alertUnexpectedMessage) return fmt.Errorf("tls: received unexpected message of type %T when waiting for HelloRetryRequest or ServerHello", msg) } var ok bool - c.vers, ok = c.config.mutualVersion(serverVersion, c.isDTLS) + c.vers, ok = c.config.mutualVersion(wireToVersion(serverWireVersion, c.isDTLS), c.isDTLS) if !ok { c.sendAlert(alertProtocolVersion) return fmt.Errorf("tls: server selected unsupported protocol version %x", c.vers) @@ -427,9 +428,9 @@ NextCipherSuite: return unexpectedMessageError(serverHello, msg) } - if c.vers != serverHello.vers { + if serverWireVersion != serverHello.vers { c.sendAlert(alertProtocolVersion) - return fmt.Errorf("tls: server sent non-matching version %x vs %x", serverHello.vers, c.vers) + return fmt.Errorf("tls: server sent non-matching version %x vs %x", serverWireVersion, serverHello.vers) } // Check for downgrade signals in the server random, per diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go index 8cec98b115..329be7f9d5 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go @@ -212,8 +212,7 @@ func (m *clientHelloMsg) marshal() []byte { handshakeMsg := newByteBuilder() handshakeMsg.addU8(typeClientHello) hello := handshakeMsg.addU24LengthPrefixed() - vers := versionToWire(m.vers, m.isDTLS) - hello.addU16(vers) + hello.addU16(m.vers) hello.addBytes(m.random) sessionId := hello.addU8LengthPrefixed() sessionId.addBytes(m.sessionId) @@ -420,7 +419,7 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool { return false } m.raw = data - m.vers = wireToVersion(uint16(data[4])<<8|uint16(data[5]), m.isDTLS) + m.vers = uint16(data[4])<<8 | uint16(data[5]) m.random = data[6:38] sessionIdLen := int(data[38]) if sessionIdLen > 32 || len(data) < 39+sessionIdLen { @@ -740,21 +739,21 @@ func (m *serverHelloMsg) marshal() []byte { handshakeMsg := newByteBuilder() handshakeMsg.addU8(typeServerHello) hello := handshakeMsg.addU24LengthPrefixed() - vers := versionToWire(m.vers, m.isDTLS) - hello.addU16(vers) + vers := wireToVersion(m.vers, m.isDTLS) + hello.addU16(m.vers) hello.addBytes(m.random) - if m.vers < VersionTLS13 { + if vers < VersionTLS13 { sessionId := hello.addU8LengthPrefixed() sessionId.addBytes(m.sessionId) } hello.addU16(m.cipherSuite) - if m.vers < VersionTLS13 { + if vers < VersionTLS13 { hello.addU8(m.compressionMethod) } extensions := hello.addU16LengthPrefixed() - if m.vers >= VersionTLS13 { + if vers >= VersionTLS13 { if m.hasKeyShare { extensions.addU16(extensionKeyShare) keyShare := extensions.addU16LengthPrefixed() @@ -772,7 +771,7 @@ func (m *serverHelloMsg) marshal() []byte { extensions.addU16(0) // Length } } else { - m.extensions.marshal(extensions, m.vers) + m.extensions.marshal(extensions, vers) if extensions.len() == 0 { hello.discardChild() } @@ -787,10 +786,11 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { return false } m.raw = data - m.vers = wireToVersion(uint16(data[4])<<8|uint16(data[5]), m.isDTLS) + m.vers = uint16(data[4])<<8 | uint16(data[5]) + vers := wireToVersion(m.vers, m.isDTLS) m.random = data[6:38] data = data[38:] - if m.vers < VersionTLS13 { + if vers < VersionTLS13 { sessionIdLen := int(data[0]) if sessionIdLen > 32 || len(data) < 1+sessionIdLen { return false @@ -803,7 +803,7 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { } m.cipherSuite = uint16(data[0])<<8 | uint16(data[1]) data = data[2:] - if m.vers < VersionTLS13 { + if vers < VersionTLS13 { if len(data) < 1 { return false } @@ -826,7 +826,7 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { return false } - if m.vers >= VersionTLS13 { + if vers >= VersionTLS13 { for len(data) != 0 { if len(data) < 4 { return false @@ -871,7 +871,7 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { return false } } - } else if !m.extensions.unmarshal(data, m.vers) { + } else if !m.extensions.unmarshal(data, vers) { return false } @@ -1873,7 +1873,7 @@ func (m *helloVerifyRequestMsg) marshal() []byte { x[1] = uint8(length >> 16) x[2] = uint8(length >> 8) x[3] = uint8(length) - vers := versionToWire(m.vers, true) + vers := m.vers x[4] = uint8(vers >> 8) x[5] = uint8(vers) x[6] = uint8(len(m.cookie)) @@ -1887,7 +1887,7 @@ func (m *helloVerifyRequestMsg) unmarshal(data []byte) bool { return false } m.raw = data - m.vers = wireToVersion(uint16(data[4])<<8|uint16(data[5]), true) + m.vers = uint16(data[4])<<8 | uint16(data[5]) cookieLen := int(data[6]) if cookieLen > 32 || len(data) != 7+cookieLen { return false diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 604fa140bb..55bc9cc1ce 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -205,14 +205,15 @@ func (hs *serverHandshakeState) readClientHello() error { } } c.clientVersion = hs.clientHello.vers + clientVersion := wireToVersion(c.clientVersion, c.isDTLS) // Reject < 1.2 ClientHellos with signature_algorithms. - if c.clientVersion < VersionTLS12 && len(hs.clientHello.signatureAlgorithms) > 0 { + if clientVersion < VersionTLS12 && len(hs.clientHello.signatureAlgorithms) > 0 { return fmt.Errorf("tls: client included signature_algorithms before TLS 1.2") } // Check the client cipher list is consistent with the version. - if hs.clientHello.vers < VersionTLS12 { + if clientVersion < VersionTLS12 { for _, id := range hs.clientHello.cipherSuites { if isTLS12Cipher(id) { return fmt.Errorf("tls: client offered TLS 1.2 cipher before TLS 1.2") @@ -238,10 +239,10 @@ func (hs *serverHandshakeState) readClientHello() error { } else if c.haveVers && config.Bugs.NegotiateVersionOnRenego != 0 { c.vers = config.Bugs.NegotiateVersionOnRenego } else { - c.vers, ok = config.mutualVersion(hs.clientHello.vers, c.isDTLS) + c.vers, ok = config.mutualVersion(clientVersion, c.isDTLS) if !ok { c.sendAlert(alertProtocolVersion) - return fmt.Errorf("tls: client offered an unsupported, maximum protocol version of %x", hs.clientHello.vers) + return fmt.Errorf("tls: client offered an unsupported, maximum protocol version of %x", clientVersion) } } c.haveVers = true @@ -311,7 +312,7 @@ func (hs *serverHandshakeState) doTLS13Handshake() error { hs.hello = &serverHelloMsg{ isDTLS: c.isDTLS, - vers: c.vers, + vers: versionToWire(c.vers, c.isDTLS), } if config.Bugs.SendServerHelloVersion != 0 { @@ -816,7 +817,7 @@ func (hs *serverHandshakeState) processClientHello() (isResume bool, err error) hs.hello = &serverHelloMsg{ isDTLS: c.isDTLS, - vers: c.vers, + vers: versionToWire(c.vers, c.isDTLS), compressionMethod: compressionNone, } @@ -1711,5 +1712,5 @@ func isTLS12Cipher(id uint16) bool { } func isGREASEValue(val uint16) bool { - return val&0x0f0f == 0x0a0a && val&0xff == val >> 8 + return val&0x0f0f == 0x0a0a && val&0xff == val>>8 } diff --git a/ssl/test/runner/key_agreement.go b/ssl/test/runner/key_agreement.go index 6327c1349f..271a9d17b3 100644 --- a/ssl/test/runner/key_agreement.go +++ b/ssl/test/runner/key_agreement.go @@ -40,7 +40,7 @@ type rsaKeyAgreement struct { func (ka *rsaKeyAgreement) generateServerKeyExchange(config *Config, cert *Certificate, clientHello *clientHelloMsg, hello *serverHelloMsg) (*serverKeyExchangeMsg, error) { // Save the client version for comparison later. - ka.clientVersion = versionToWire(clientHello.vers, clientHello.isDTLS) + ka.clientVersion = clientHello.vers if !config.Bugs.RSAEphemeralKey { return nil, nil @@ -128,7 +128,7 @@ func (ka *rsaKeyAgreement) processClientKeyExchange(config *Config, cert *Certif // RFC 4346. vers := uint16(preMasterSecret[0])<<8 | uint16(preMasterSecret[1]) if ka.clientVersion != vers { - return nil, errors.New("tls: invalid version in RSA premaster") + return nil, fmt.Errorf("tls: invalid version in RSA premaster (got %04x, wanted %04x)", vers, ka.clientVersion) } return preMasterSecret, nil } @@ -144,7 +144,6 @@ func (ka *rsaKeyAgreement) generateClientKeyExchange(config *Config, clientHello if bad == RSABadValueWrongVersion { vers ^= 1 } - vers = versionToWire(vers, clientHello.isDTLS) preMasterSecret[0] = byte(vers >> 8) preMasterSecret[1] = byte(vers) _, err := io.ReadFull(config.rand(), preMasterSecret[2:]) diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index ea660abe98..ea29ef5030 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -4194,7 +4194,7 @@ func addVersionNegotiationTests() { name: "MinorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0x03ff, + SendClientVersion: 0xfe00, }, }, expectedVersion: VersionTLS12, @@ -4205,7 +4205,7 @@ func addVersionNegotiationTests() { name: "MajorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0x0400, + SendClientVersion: 0xfdff, }, }, expectedVersion: VersionTLS12, @@ -4229,9 +4229,7 @@ func addVersionNegotiationTests() { name: "VersionTooLow-DTLS", config: Config{ Bugs: ProtocolBugs{ - // 0x0201 is the lowest version expressable in - // DTLS. - SendClientVersion: 0x0201, + SendClientVersion: 0xffff, }, }, shouldFail: true,