Skip to content

Commit

Permalink
Apply version/wire mapping at a higher layer in runner.
Browse files Browse the repository at this point in the history 
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 <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
  • Loading branch information
@davidben
davidben authored and CQ bot account: commit-bot@chromium.org committed Sep 27, 2016
1 parent 5ab4596 commit 3c6a1ea
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 44 deletions.
6 changes: 3 additions & 3 deletions ssl/test/runner/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
21 changes: 11 additions & 10 deletions ssl/test/runner/handshake_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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()
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
32 changes: 16 additions & 16 deletions ssl/test/runner/handshake_messages.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand All @@ -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()
}
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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))
Expand All @@ -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
Expand Down
15 changes: 8 additions & 7 deletions ssl/test/runner/handshake_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
}

Expand Down Expand Up @@ -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
}
5 changes: 2 additions & 3 deletions ssl/test/runner/key_agreement.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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:])
Expand Down
8 changes: 3 additions & 5 deletions ssl/test/runner/runner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4194,7 +4194,7 @@ func addVersionNegotiationTests() {
name: "MinorVersionTolerance-DTLS",
config: Config{
Bugs: ProtocolBugs{
SendClientVersion: 0x03ff,
SendClientVersion: 0xfe00,
},
},
expectedVersion: VersionTLS12,
Expand All @@ -4205,7 +4205,7 @@ func addVersionNegotiationTests() {
name: "MajorVersionTolerance-DTLS",
config: Config{
Bugs: ProtocolBugs{
SendClientVersion: 0x0400,
SendClientVersion: 0xfdff,
},
},
expectedVersion: VersionTLS12,
Expand All @@ -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,
Expand Down

0 comments on commit 3c6a1ea

Please sign in to comment.