Fix empty fragment handling in DTLS message reassembly.
Found with libFuzzer.
Bug: chromium:763097
Change-Id: I806bcfc714c0629ff7f725e37f4c0045d4ec7ac6
Reviewed-on: https://boringssl-review.googlesource.com/20105
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index 321e01d..ab499df 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -228,6 +228,10 @@
// A zero-length message will never have a pending reassembly.
assert(msg_len > 0);
+ if (start == end) {
+ return;
+ }
+
if ((start >> 3) == (end >> 3)) {
frag->reassembly[start >> 3] |= bit_range(start & 7, end & 7);
} else {
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 7244527..d5b017f 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -119,6 +119,7 @@
extensionUseSRTP uint16 = 14
extensionALPN uint16 = 16
extensionSignedCertificateTimestamp uint16 = 18
+ extensionPadding uint16 = 21
extensionExtendedMasterSecret uint16 = 23
extensionSessionTicket uint16 = 35
extensionKeyShare uint16 = 40 // draft-ietf-tls-tls13-16
@@ -1434,6 +1435,10 @@
// ExpectRecordSplitting, if true, causes application records to only be
// accepted if they follow a 1/n-1 record split.
ExpectRecordSplitting bool
+
+ // PadClientHello, if non-zero, pads the ClientHello to a multiple of
+ // that many bytes.
+ PadClientHello int
}
func (c *Config) serverInit() {
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go
index c81955e..619710c 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -209,8 +209,8 @@
isFinished := header[0] == typeFinished
if c.config.Bugs.SendEmptyFragments {
- fragment := c.makeFragment(header, data, 0, 0)
- c.pendingFragments = append(c.pendingFragments, fragment)
+ c.pendingFragments = append(c.pendingFragments, c.makeFragment(header, data, 0, 0))
+ c.pendingFragments = append(c.pendingFragments, c.makeFragment(header, data, len(data), 0))
}
firstRun := true
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index 7da08d8..d65119f 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -175,6 +175,7 @@
pskBinderFirst bool
omitExtensions bool
emptyExtensions bool
+ pad int
}
func (m *clientHelloMsg) equal(i interface{}) bool {
@@ -222,7 +223,8 @@
m.hasGREASEExtension == m1.hasGREASEExtension &&
m.pskBinderFirst == m1.pskBinderFirst &&
m.omitExtensions == m1.omitExtensions &&
- m.emptyExtensions == m1.emptyExtensions
+ m.emptyExtensions == m1.emptyExtensions &&
+ m.pad == m1.pad
}
func (m *clientHelloMsg) marshal() []byte {
@@ -454,6 +456,16 @@
}
}
+ if m.pad != 0 && hello.len()%m.pad != 0 {
+ extensions.addU16(extensionPadding)
+ padding := extensions.addU16LengthPrefixed()
+ // Note hello.len() has changed at this point from the length
+ // prefix.
+ if l := hello.len() % m.pad; l != 0 {
+ padding.addBytes(make([]byte, m.pad-l))
+ }
+ }
+
if m.omitExtensions || m.emptyExtensions {
// Silently erase any extensions which were sent.
hello.discardChild()
@@ -463,6 +475,10 @@
}
m.raw = handshakeMsg.finish()
+ // Sanity-check padding.
+ if m.pad != 0 && (len(m.raw)-4)%m.pad != 0 {
+ panic(fmt.Sprintf("%d is not a multiple of %d", len(m.raw)-4, m.pad))
+ }
return m.raw
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 89f0713..2ffe795 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2123,6 +2123,19 @@
},
},
{
+ testType: serverTest,
+ protocol: dtls,
+ name: "SendEmptyFragments-Padded-DTLS",
+ config: Config{
+ Bugs: ProtocolBugs{
+ // Test empty fragments for a message with a
+ // nice power-of-two length.
+ PadClientHello: 64,
+ SendEmptyFragments: true,
+ },
+ },
+ },
+ {
name: "BadFinished-Client",
config: Config{
MaxVersion: VersionTLS12,
