runner: Remove outdated FragmentClientVersion logic
A long time ago, OpenSSL negotiated versions by sniffing at the first
few bytes of the ClientHello. It couldn't handle ClientHellos that were
so fragmented that the version field was unreadable.
When it encountered a ClientHello it couldn't handle, it silently
assumed TLS 1.0, which led to CVE-2014-3511. The original fix for that
made this case an error instead. But this meant that
MaxHandshakeRecordLength had to take care never to trigger this error
case in other tests, otherwise it wouldn't exercise what we were trying
to exercise. So we addeed some funny logic in
https://boringssl-review.googlesource.com/1452.
Fast forward many years and BoringSSL no longer negotiates versions this
way. We read the whole ClientHello and then act on it. Now fragmenting
the first few bytes of the ClientHello behaves the same as any other,
and we no longer need to special case it in tests.
Change-Id: Id098f1e2066626661113ca4796250feb6cea421b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73247
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 0c54593..e691363 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -867,16 +867,11 @@
// MaxHandshakeRecordLength, if non-zero, is the maximum size of a
// handshake record. Handshake messages will be split into multiple
- // records at the specified size, except that the client_version will
- // never be fragmented. For DTLS, it is the maximum handshake fragment
- // size, not record size; DTLS allows multiple handshake fragments in a
- // single handshake record. See |PackHandshakeFragments|.
+ // records at the specified size. For DTLS, it is the maximum handshake
+ // fragment size, not record size; DTLS allows multiple handshake fragments
+ // in a single handshake record. See |PackHandshakeFragments|.
MaxHandshakeRecordLength int
- // FragmentClientVersion will allow MaxHandshakeRecordLength to apply to
- // the first 6 bytes of the ClientHello.
- FragmentClientVersion bool
-
// FragmentAlert will cause all alerts to be fragmented across
// two records.
FragmentAlert bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index e634449..5bfc91e 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1311,7 +1311,6 @@
func (c *Conn) doWriteRecord(typ recordType, data []byte) (n int, err error) {
first := true
- isClientHello := typ == recordTypeHandshake && len(data) > 0 && data[0] == typeClientHello
for len(data) > 0 || first {
m := len(data)
if m > maxPlaintext && !c.config.Bugs.SendLargeRecords {
@@ -1319,12 +1318,6 @@
}
if typ == recordTypeHandshake && c.config.Bugs.MaxHandshakeRecordLength > 0 && m > c.config.Bugs.MaxHandshakeRecordLength {
m = c.config.Bugs.MaxHandshakeRecordLength
- // By default, do not fragment the client_version or
- // server_version, which are located in the first 6
- // bytes.
- if first && isClientHello && !c.config.Bugs.FragmentClientVersion && m < 6 {
- m = 6
- }
}
first = false
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index d1a782b..6ee603f 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2306,13 +2306,14 @@
},
flags: []string{"-max-version", strconv.Itoa(VersionTLS12)},
},
+ // Regression test for CVE-2014-3511. Even when the ClientHello is
+ // maximally fragmented, version negotiation works correctly.
{
testType: serverTest,
name: "FragmentedClientVersion",
config: Config{
Bugs: ProtocolBugs{
MaxHandshakeRecordLength: 1,
- FragmentClientVersion: true,
},
},
expectations: connectionExpectations{
