[dev.boringcrypto] all: merge master into dev.boringcrypto

Change-Id: I083d1e4e997b30d9fab10940401eaf160e36f6c1

common.go

@@ -207,6 +207,10 @@ const (
 	downgradeCanaryTLS11 = "DOWNGRD\x00"
 )
 
+// testingOnlyForceDowngradeCanary is set in tests to force the server side to
+// include downgrade canaries even if it's using its highers supported version.
+var testingOnlyForceDowngradeCanary bool
+
 // ConnectionState records basic TLS details about the connection.
 type ConnectionState struct {
 	Version                     uint16                // TLS version used by the connection (e.g. VersionTLS12)

handshake_client.go

@@ -54,7 +54,7 @@ func (c *Conn) makeClientHello() (*clientHelloMsg, ecdheParameters, error) {
 		return nil, nil, errors.New("tls: no supported versions satisfy MinVersion and MaxVersion")
 	}
 
-	clientHelloVersion := supportedVersions[0]
+	clientHelloVersion := config.maxSupportedVersion()
 	// The version at the beginning of the ClientHello was capped at TLS 1.2
 	// for compatibility reasons. The supported_versions extension is used
 	// to negotiate versions now. See RFC 8446, Section 4.2.1.
@@ -184,6 +184,18 @@ func (c *Conn) clientHandshake() (err error) {
 		return err
 	}
 
+	// If we are negotiating a protocol version that's lower than what we
+	// support, check for the server downgrade canaries.
+	// See RFC 8446, Section 4.1.3.
+	maxVers := c.config.maxSupportedVersion()
+	tls12Downgrade := string(serverHello.random[24:]) == downgradeCanaryTLS12
+	tls11Downgrade := string(serverHello.random[24:]) == downgradeCanaryTLS11
+	if maxVers == VersionTLS13 && c.vers <= VersionTLS12 && (tls12Downgrade || tls11Downgrade) ||
+		maxVers == VersionTLS12 && c.vers <= VersionTLS11 && tls11Downgrade {
+		c.sendAlert(alertIllegalParameter)
+		return errors.New("tls: downgrade attempt detected, possibly due to a MitM attack or a broken middlebox")
+	}
+
 	if c.vers == VersionTLS13 {
 		hs := &clientHandshakeStateTLS13{
 			c:           c,

handshake_client_test.go

@@ -1984,3 +1984,48 @@ func TestCloseClientConnectionOnIdleServer(t *testing.T) {
 		t.Errorf("Error expected, but no error returned")
 	}
 }
+
+func testDowngradeCanary(t *testing.T, clientVersion, serverVersion uint16) error {
+	defer func() { testingOnlyForceDowngradeCanary = false }()
+	testingOnlyForceDowngradeCanary = true
+
+	clientConfig := testConfig.Clone()
+	clientConfig.MaxVersion = clientVersion
+	serverConfig := testConfig.Clone()
+	serverConfig.MaxVersion = serverVersion
+	_, _, err := testHandshake(t, clientConfig, serverConfig)
+	return err
+}
+
+func TestDowngradeCanary(t *testing.T) {
+	if err := testDowngradeCanary(t, VersionTLS13, VersionTLS12); err == nil {
+		t.Errorf("downgrade from TLS 1.3 to TLS 1.2 was not detected")
+	}
+	if testing.Short() {
+		t.Skip("skipping the rest of the checks in short mode")
+	}
+	if err := testDowngradeCanary(t, VersionTLS13, VersionTLS11); err == nil {
+		t.Errorf("downgrade from TLS 1.3 to TLS 1.1 was not detected")
+	}
+	if err := testDowngradeCanary(t, VersionTLS13, VersionTLS10); err == nil {
+		t.Errorf("downgrade from TLS 1.3 to TLS 1.0 was not detected")
+	}
+	if err := testDowngradeCanary(t, VersionTLS12, VersionTLS11); err == nil {
+		t.Errorf("downgrade from TLS 1.2 to TLS 1.1 was not detected")
+	}
+	if err := testDowngradeCanary(t, VersionTLS12, VersionTLS10); err == nil {
+		t.Errorf("downgrade from TLS 1.2 to TLS 1.0 was not detected")
+	}
+	if err := testDowngradeCanary(t, VersionTLS13, VersionTLS13); err != nil {
+		t.Errorf("server unexpectedly sent downgrade canary for TLS 1.3")
+	}
+	if err := testDowngradeCanary(t, VersionTLS12, VersionTLS12); err != nil {
+		t.Errorf("client didn't ignore expected TLS 1.2 canary")
+	}
+	if err := testDowngradeCanary(t, VersionTLS11, VersionTLS11); err != nil {
+		t.Errorf("client unexpectedly reacted to a canary in TLS 1.1")
+	}
+	if err := testDowngradeCanary(t, VersionTLS10, VersionTLS10); err != nil {
+		t.Errorf("client unexpectedly reacted to a canary in TLS 1.0")
+	}
+}

handshake_client_tls13.go

@@ -180,23 +180,35 @@ func (hs *clientHandshakeStateTLS13) processHelloRetryRequest() error {
 	c := hs.c
 
 	// The first ClientHello gets double-hashed into the transcript upon a
-	// HelloRetryRequest. See RFC 8446, Section 4.4.1.
+	// HelloRetryRequest. (The idea is that the server might offload transcript
+	// storage to the client in the cookie.) See RFC 8446, Section 4.4.1.
 	chHash := hs.transcript.Sum(nil)
 	hs.transcript.Reset()
 	hs.transcript.Write([]byte{typeMessageHash, 0, 0, uint8(len(chHash))})
 	hs.transcript.Write(chHash)
 	hs.transcript.Write(hs.serverHello.marshal())
 
+	// The only HelloRetryRequest extensions we support are key_share and
+	// cookie, and clients must abort the handshake if the HRR would not result
+	// in any change in the ClientHello.
+	if hs.serverHello.selectedGroup == 0 && hs.serverHello.cookie == nil {
+		c.sendAlert(alertIllegalParameter)
+		return errors.New("tls: server sent an unnecessary HelloRetryRequest message")
+	}
+
+	if hs.serverHello.cookie != nil {
+		hs.hello.cookie = hs.serverHello.cookie
+	}
+
 	if hs.serverHello.serverShare.group != 0 {
 		c.sendAlert(alertDecodeError)
 		return errors.New("tls: received malformed key_share extension")
 	}
 
-	curveID := hs.serverHello.selectedGroup
+	// If the server sent a key_share extension selecting a group, ensure it's
-	if curveID == 0 {
+	// a group we advertised but did not send a key share for, and send a key
-		c.sendAlert(alertMissingExtension)
+	// share for it this time.
-		return errors.New("tls: received HelloRetryRequest without selected group")
+	if curveID := hs.serverHello.selectedGroup; curveID != 0 {
-	}
 		curveOK := false
 		for _, id := range hs.hello.supportedCurves {
 			if id == curveID {
@@ -210,7 +222,7 @@ func (hs *clientHandshakeStateTLS13) processHelloRetryRequest() error {
 		}
 		if hs.ecdheParams.CurveID() == curveID {
 			c.sendAlert(alertIllegalParameter)
-		return errors.New("tls: server sent an unnecessary HelloRetryRequest message")
+			return errors.New("tls: server sent an unnecessary HelloRetryRequest key_share")
 		}
 		if _, ok := curveForCurveID(curveID); curveID != X25519 && !ok {
 			c.sendAlert(alertInternalError)
@@ -223,8 +235,7 @@ func (hs *clientHandshakeStateTLS13) processHelloRetryRequest() error {
 		}
 		hs.ecdheParams = params
 		hs.hello.keyShares = []keyShare{{group: curveID, data: params.PublicKey()}}
-
+	}
-	hs.hello.cookie = hs.serverHello.cookie
 
 	hs.hello.raw = nil
 	if len(hs.hello.pskIdentities) > 0 {

handshake_messages_test.go

@@ -308,11 +308,9 @@ func (*sessionState) Generate(rand *rand.Rand, size int) reflect.Value {
 	s := &sessionState{}
 	s.vers = uint16(rand.Intn(10000))
 	s.cipherSuite = uint16(rand.Intn(10000))
-	s.masterSecret = randomBytes(rand.Intn(100), rand)
+	s.masterSecret = randomBytes(rand.Intn(100)+1, rand)
-	numCerts := rand.Intn(20)
+	for i := 0; i < rand.Intn(20); i++ {
-	s.certificates = make([][]byte, numCerts)
+		s.certificates = append(s.certificates, randomBytes(rand.Intn(500)+1, rand))
-	for i := 0; i < numCerts; i++ {
-		s.certificates[i] = randomBytes(rand.Intn(10)+1, rand)
 	}
 	return reflect.ValueOf(s)
 }

handshake_server.go

@@ -193,7 +193,7 @@ func (hs *serverHandshakeState) processClientHello() error {
 	serverRandom := hs.hello.random
 	// Downgrade protection canaries. See RFC 8446, Section 4.1.3.
 	maxVers := c.config.maxSupportedVersion()
-	if maxVers >= VersionTLS12 && c.vers < maxVers {
+	if maxVers >= VersionTLS12 && c.vers < maxVers || testingOnlyForceDowngradeCanary {
 		if c.vers == VersionTLS12 {
 			copy(serverRandom[24:], downgradeCanaryTLS12)
 		} else {

ticket.go

@@ -12,8 +12,9 @@ import (
 	"crypto/sha256"
 	"crypto/subtle"
 	"errors"
-	"golang.org/x/crypto/cryptobyte"
 	"io"
+
+	"golang.org/x/crypto/cryptobyte"
 )
 
 // sessionState contains the information that is serialized into a session
@@ -21,88 +22,56 @@ import (
 type sessionState struct {
 	vers         uint16
 	cipherSuite  uint16
-	masterSecret []byte
+	masterSecret []byte // opaque master_secret<1..2^16-1>;
-	certificates [][]byte
+	// struct { opaque certificate<1..2^32-1> } Certificate;
+	certificates [][]byte // Certificate certificate_list<0..2^16-1>;
+
 	// usedOldKey is true if the ticket from which this session came from
 	// was encrypted with an older key and thus should be refreshed.
 	usedOldKey bool
 }
 
-func (s *sessionState) marshal() []byte {
+func (m *sessionState) marshal() []byte {
-	length := 2 + 2 + 2 + len(s.masterSecret) + 2
+	var b cryptobyte.Builder
-	for _, cert := range s.certificates {
+	b.AddUint16(m.vers)
-		length += 4 + len(cert)
+	b.AddUint16(m.cipherSuite)
+	b.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) {
+		b.AddBytes(m.masterSecret)
+	})
+	b.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) {
+		for _, cert := range m.certificates {
+			b.AddUint32LengthPrefixed(func(b *cryptobyte.Builder) {
+				b.AddBytes(cert)
+			})
+		}
+	})
+	return b.BytesOrPanic()
 }
 
-	ret := make([]byte, length)
+func (m *sessionState) unmarshal(data []byte) bool {
-	x := ret
+	*m = sessionState{usedOldKey: m.usedOldKey}
-	x[0] = byte(s.vers >> 8)
+	s := cryptobyte.String(data)
-	x[1] = byte(s.vers)
+	if ok := s.ReadUint16(&m.vers) &&
-	x[2] = byte(s.cipherSuite >> 8)
+		m.vers != VersionTLS13 &&
-	x[3] = byte(s.cipherSuite)
+		s.ReadUint16(&m.cipherSuite) &&
-	x[4] = byte(len(s.masterSecret) >> 8)
+		readUint16LengthPrefixed(&s, &m.masterSecret) &&
-	x[5] = byte(len(s.masterSecret))
+		len(m.masterSecret) != 0; !ok {
-	x = x[6:]
-	copy(x, s.masterSecret)
-	x = x[len(s.masterSecret):]
-
-	x[0] = byte(len(s.certificates) >> 8)
-	x[1] = byte(len(s.certificates))
-	x = x[2:]
-
-	for _, cert := range s.certificates {
-		x[0] = byte(len(cert) >> 24)
-		x[1] = byte(len(cert) >> 16)
-		x[2] = byte(len(cert) >> 8)
-		x[3] = byte(len(cert))
-		copy(x[4:], cert)
-		x = x[4+len(cert):]
-	}
-
-	return ret
-}
-
-func (s *sessionState) unmarshal(data []byte) bool {
-	if len(data) < 8 {
 		return false
 	}
-
+	var certList cryptobyte.String
-	s.vers = uint16(data[0])<<8 | uint16(data[1])
+	if !s.ReadUint16LengthPrefixed(&certList) {
-	s.cipherSuite = uint16(data[2])<<8 | uint16(data[3])
-	masterSecretLen := int(data[4])<<8 | int(data[5])
-	data = data[6:]
-	if len(data) < masterSecretLen {
 		return false
 	}
-
+	for !certList.Empty() {
-	s.masterSecret = data[:masterSecretLen]
+		var certLen uint32
-	data = data[masterSecretLen:]
+		certList.ReadUint32(&certLen)
-
+		var cert []byte
-	if len(data) < 2 {
+		if certLen == 0 || !certList.ReadBytes(&cert, int(certLen)) {
 			return false
 		}
-
+		m.certificates = append(m.certificates, cert)
-	numCerts := int(data[0])<<8 | int(data[1])
-	data = data[2:]
-
-	s.certificates = make([][]byte, numCerts)
-	for i := range s.certificates {
-		if len(data) < 4 {
-			return false
 	}
-		certLen := int(data[0])<<24 | int(data[1])<<16 | int(data[2])<<8 | int(data[3])
+	return s.Empty()
-		data = data[4:]
-		if certLen < 0 {
-			return false
-		}
-		if len(data) < certLen {
-			return false
-		}
-		s.certificates[i] = data[:certLen]
-		data = data[certLen:]
-	}
-
-	return len(data) == 0
 }
 
 // sessionStateTLS13 is the content of a TLS 1.3 session ticket. Its first