Reject compressed ECDH coordinates in TLS.
We don't advertise compressed coordinates (and point format negotiation
was deprecated in TLS 1.3), so reject them. Both Internet Explorer and
Firefox appear to reject them already.
Later I hope to add an easier to use ECDH API that acts on bytes, not
EC_POINT. This clears the way for that API to only accept uncompressed
coordinates. Compressed coordinates never got deployed over NIST curves,
for better or worse. At this point, there is no sense in changing that
as new protocols should use curve25519.
Change-Id: Id2f1be791ddcf155d596f4eb0b79351766c5cdab
Reviewed-on: https://boringssl-review.googlesource.com/26024
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index a5ae578..4d76bb2 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -97,8 +97,10 @@
return false;
}
- if (!EC_POINT_oct2point(group.get(), peer_point.get(), peer_key.data(),
+ if (peer_key.empty() || peer_key[0] != POINT_CONVERSION_UNCOMPRESSED ||
+ !EC_POINT_oct2point(group.get(), peer_point.get(), peer_key.data(),
peer_key.size(), bn_ctx.get())) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT);
*out_alert = SSL_AD_DECODE_ERROR;
return false;
}
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index fef5129..1192948 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1551,6 +1551,10 @@
// require that the client sent a dummy PQ padding extension of this
// length.
ExpectDummyPQPaddingLength int
+
+ // SendCompressedCoordinates, if true, causes ECDH key shares over NIST
+ // curves to use compressed coordinates.
+ SendCompressedCoordinates bool
}
func (c *Config) serverInit() {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 1140269..c8b1f37 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -168,7 +168,7 @@
if !curvesToSend[curveID] {
continue
}
- curve, ok := curveForCurveID(curveID)
+ curve, ok := curveForCurveID(curveID, c.config)
if !ok {
continue
}
@@ -523,7 +523,7 @@
c.sendAlert(alertHandshakeFailure)
return errors.New("tls: received invalid HelloRetryRequest")
}
- curve, ok := curveForCurveID(group)
+ curve, ok := curveForCurveID(group, c.config)
if !ok {
return errors.New("tls: Unable to get curve requested in HelloRetryRequest")
}
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index caa66ed..329cfd6 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -735,7 +735,7 @@
// Once a curve has been selected and a key share identified,
// the server needs to generate a public value and send it in
// the ServerHello.
- curve, ok := curveForCurveID(selectedCurve)
+ curve, ok := curveForCurveID(selectedCurve, config)
if !ok {
panic("tls: server failed to look up curve ID")
}
@@ -745,7 +745,7 @@
if config.Bugs.SkipHelloRetryRequest {
// If skipping HelloRetryRequest, use a random key to
// avoid crashing.
- curve2, _ := curveForCurveID(selectedCurve)
+ curve2, _ := curveForCurveID(selectedCurve, config)
var err error
peerKey, err = curve2.offer(config.rand())
if err != nil {
diff --git a/ssl/test/runner/key_agreement.go b/ssl/test/runner/key_agreement.go
index 5071985..1b4dfc4 100644
--- a/ssl/test/runner/key_agreement.go
+++ b/ssl/test/runner/key_agreement.go
@@ -252,8 +252,9 @@
// ellipticECDHCurve implements ecdhCurve with an elliptic.Curve.
type ellipticECDHCurve struct {
- curve elliptic.Curve
- privateKey []byte
+ curve elliptic.Curve
+ privateKey []byte
+ sendCompressed bool
}
func (e *ellipticECDHCurve) offer(rand io.Reader) (publicKey []byte, err error) {
@@ -262,7 +263,15 @@
if err != nil {
return nil, err
}
- return elliptic.Marshal(e.curve, x, y), nil
+ ret := elliptic.Marshal(e.curve, x, y)
+ if e.sendCompressed {
+ l := (len(ret) - 1) / 2
+ tmp := make([]byte, 1+l)
+ tmp[0] = byte(2 | y.Bit(0))
+ copy(tmp[1:], ret[1:1+l])
+ ret = tmp
+ }
+ return ret, nil
}
func (e *ellipticECDHCurve) accept(rand io.Reader, peerKey []byte) (publicKey []byte, preMasterSecret []byte, err error) {
@@ -334,16 +343,16 @@
return out[:], nil
}
-func curveForCurveID(id CurveID) (ecdhCurve, bool) {
+func curveForCurveID(id CurveID, config *Config) (ecdhCurve, bool) {
switch id {
case CurveP224:
- return &ellipticECDHCurve{curve: elliptic.P224()}, true
+ return &ellipticECDHCurve{curve: elliptic.P224(), sendCompressed: config.Bugs.SendCompressedCoordinates}, true
case CurveP256:
- return &ellipticECDHCurve{curve: elliptic.P256()}, true
+ return &ellipticECDHCurve{curve: elliptic.P256(), sendCompressed: config.Bugs.SendCompressedCoordinates}, true
case CurveP384:
- return &ellipticECDHCurve{curve: elliptic.P384()}, true
+ return &ellipticECDHCurve{curve: elliptic.P384(), sendCompressed: config.Bugs.SendCompressedCoordinates}, true
case CurveP521:
- return &ellipticECDHCurve{curve: elliptic.P521()}, true
+ return &ellipticECDHCurve{curve: elliptic.P521(), sendCompressed: config.Bugs.SendCompressedCoordinates}, true
case CurveX25519:
return &x25519ECDHCurve{}, true
default:
@@ -507,7 +516,7 @@
}
var ok bool
- if ka.curve, ok = curveForCurveID(curveid); !ok {
+ if ka.curve, ok = curveForCurveID(curveid, config); !ok {
return nil, errors.New("tls: preferredCurves includes unsupported curve")
}
ka.curveID = curveid
@@ -552,7 +561,7 @@
ka.curveID = curveid
var ok bool
- if ka.curve, ok = curveForCurveID(curveid); !ok {
+ if ka.curve, ok = curveForCurveID(curveid, config); !ok {
return errors.New("tls: server selected unsupported curve")
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 430e3d9..9e4817d 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -10443,58 +10443,94 @@
func addCurveTests() {
for _, curve := range testCurves {
- testCases = append(testCases, testCase{
- name: "CurveTest-Client-" + curve.name,
- config: Config{
- MaxVersion: VersionTLS12,
- CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
- CurvePreferences: []CurveID{curve.id},
- },
- flags: []string{
- "-enable-all-curves",
- "-expect-curve-id", strconv.Itoa(int(curve.id)),
- },
- expectedCurveID: curve.id,
- })
- testCases = append(testCases, testCase{
- name: "CurveTest-Client-" + curve.name + "-TLS13",
- config: Config{
- MaxVersion: VersionTLS13,
- CurvePreferences: []CurveID{curve.id},
- },
- flags: []string{
- "-enable-all-curves",
- "-expect-curve-id", strconv.Itoa(int(curve.id)),
- },
- expectedCurveID: curve.id,
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "CurveTest-Server-" + curve.name,
- config: Config{
- MaxVersion: VersionTLS12,
- CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
- CurvePreferences: []CurveID{curve.id},
- },
- flags: []string{
- "-enable-all-curves",
- "-expect-curve-id", strconv.Itoa(int(curve.id)),
- },
- expectedCurveID: curve.id,
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "CurveTest-Server-" + curve.name + "-TLS13",
- config: Config{
- MaxVersion: VersionTLS13,
- CurvePreferences: []CurveID{curve.id},
- },
- flags: []string{
- "-enable-all-curves",
- "-expect-curve-id", strconv.Itoa(int(curve.id)),
- },
- expectedCurveID: curve.id,
- })
+ for _, ver := range tlsVersions {
+ // SSL 3.0 cannot reliably negotiate curves.
+ if ver.version == VersionSSL30 {
+ continue
+ }
+
+ suffix := curve.name + "-" + ver.name
+
+ testCases = append(testCases, testCase{
+ name: "CurveTest-Client-" + suffix,
+ config: Config{
+ MaxVersion: ver.version,
+ CipherSuites: []uint16{
+ TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+ TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+ TLS_AES_128_GCM_SHA256,
+ },
+ CurvePreferences: []CurveID{curve.id},
+ },
+ tls13Variant: ver.tls13Variant,
+ flags: []string{
+ "-enable-all-curves",
+ "-expect-curve-id", strconv.Itoa(int(curve.id)),
+ },
+ expectedCurveID: curve.id,
+ })
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "CurveTest-Server-" + suffix,
+ config: Config{
+ MaxVersion: ver.version,
+ CipherSuites: []uint16{
+ TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+ TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+ TLS_AES_128_GCM_SHA256,
+ },
+ CurvePreferences: []CurveID{curve.id},
+ },
+ tls13Variant: ver.tls13Variant,
+ flags: []string{
+ "-enable-all-curves",
+ "-expect-curve-id", strconv.Itoa(int(curve.id)),
+ },
+ expectedCurveID: curve.id,
+ })
+
+ if curve.id != CurveX25519 {
+ testCases = append(testCases, testCase{
+ name: "CurveTest-Client-Compressed-" + suffix,
+ config: Config{
+ MaxVersion: ver.version,
+ CipherSuites: []uint16{
+ TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+ TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+ TLS_AES_128_GCM_SHA256,
+ },
+ CurvePreferences: []CurveID{curve.id},
+ Bugs: ProtocolBugs{
+ SendCompressedCoordinates: true,
+ },
+ },
+ tls13Variant: ver.tls13Variant,
+ flags: []string{"-enable-all-curves"},
+ shouldFail: true,
+ expectedError: ":BAD_ECPOINT:",
+ })
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "CurveTest-Server-Compressed-" + suffix,
+ config: Config{
+ MaxVersion: ver.version,
+ CipherSuites: []uint16{
+ TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+ TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+ TLS_AES_128_GCM_SHA256,
+ },
+ CurvePreferences: []CurveID{curve.id},
+ Bugs: ProtocolBugs{
+ SendCompressedCoordinates: true,
+ },
+ },
+ tls13Variant: ver.tls13Variant,
+ flags: []string{"-enable-all-curves"},
+ shouldFail: true,
+ expectedError: ":BAD_ECPOINT:",
+ })
+ }
+ }
}
// The server must be tolerant to bogus curves.
@@ -10630,7 +10666,7 @@
},
},
shouldFail: true,
- expectedError: ":INVALID_ENCODING:",
+ expectedError: ":BAD_ECPOINT:",
})
testCases = append(testCases, testCase{
name: "InvalidECDHPoint-Client-TLS13",
@@ -10642,7 +10678,7 @@
},
},
shouldFail: true,
- expectedError: ":INVALID_ENCODING:",
+ expectedError: ":BAD_ECPOINT:",
})
testCases = append(testCases, testCase{
testType: serverTest,
@@ -10656,7 +10692,7 @@
},
},
shouldFail: true,
- expectedError: ":INVALID_ENCODING:",
+ expectedError: ":BAD_ECPOINT:",
})
testCases = append(testCases, testCase{
testType: serverTest,
@@ -10669,7 +10705,7 @@
},
},
shouldFail: true,
- expectedError: ":INVALID_ENCODING:",
+ expectedError: ":BAD_ECPOINT:",
})
// The previous curve ID should be reported on TLS 1.2 resumption.
