Reject empty ALPN protocols.
https://tools.ietf.org/html/rfc7301#section-3.1 specifies that a
ProtocolName may not be empty. This change enforces this in ClientHello
and ServerHello messages.
Thanks to Doug Hogan for reporting this.
Change-Id: Iab879c83145007799b94d2725201ede1a39e4596
Reviewed-on: https://boringssl-review.googlesource.com/5390
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 7005704..1a5594d 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1674,7 +1674,9 @@
while (CBS_len(&protocol_name_list_copy) > 0) {
CBS protocol_name;
- if (!CBS_get_u8_length_prefixed(&protocol_name_list_copy, &protocol_name)) {
+ if (!CBS_get_u8_length_prefixed(&protocol_name_list_copy, &protocol_name) ||
+ /* Empty protocol names are forbidden. */
+ CBS_len(&protocol_name) == 0) {
goto parse_error;
}
}
@@ -2118,6 +2120,8 @@
if (!CBS_get_u16_length_prefixed(&extension, &protocol_name_list) ||
CBS_len(&extension) != 0 ||
!CBS_get_u8_length_prefixed(&protocol_name_list, &protocol_name) ||
+ /* Empty protocol names are forbidden. */
+ CBS_len(&protocol_name) == 0 ||
CBS_len(&protocol_name_list) != 0) {
*out_alert = SSL_AD_DECODE_ERROR;
return 0;
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 2e30208..bad3ebe 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -544,6 +544,10 @@
// of ALPN works regardless of their relative order.
SwapNPNAndALPN bool
+ // ALPNProtocol, if not nil, sets the ALPN protocol that a server will
+ // return.
+ ALPNProtocol *string
+
// AllowSessionVersionMismatch causes the server to resume sessions
// regardless of the version associated with the session.
AllowSessionVersionMismatch bool
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 951b956..bc10fe7 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -45,7 +45,7 @@
nextProtosLength := 0
for _, proto := range c.config.NextProtos {
- if l := len(proto); l == 0 || l > 255 {
+ if l := len(proto); l > 255 {
return errors.New("tls: invalid NextProtos value")
} else {
nextProtosLength += 1 + l
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index ce214fd..4fee9d4 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -119,7 +119,7 @@
if len(m.alpnProtocols) > 0 {
extensionsLength += 2
for _, s := range m.alpnProtocols {
- if l := len(s); l == 0 || l > 255 {
+ if l := len(s); l > 255 {
panic("invalid ALPN protocol")
}
extensionsLength++
@@ -625,6 +625,7 @@
ticketSupported bool
secureRenegotiation []byte
alpnProtocol string
+ alpnProtocolEmpty bool
duplicateExtension bool
channelIDRequested bool
extendedMasterSecret bool
@@ -653,6 +654,7 @@
bytes.Equal(m.secureRenegotiation, m1.secureRenegotiation) &&
(m.secureRenegotiation == nil) == (m1.secureRenegotiation == nil) &&
m.alpnProtocol == m1.alpnProtocol &&
+ m.alpnProtocolEmpty == m1.alpnProtocolEmpty &&
m.duplicateExtension == m1.duplicateExtension &&
m.channelIDRequested == m1.channelIDRequested &&
m.extendedMasterSecret == m1.extendedMasterSecret &&
@@ -695,7 +697,7 @@
if m.channelIDRequested {
numExtensions++
}
- if alpnLen := len(m.alpnProtocol); alpnLen > 0 {
+ if alpnLen := len(m.alpnProtocol); alpnLen > 0 || m.alpnProtocolEmpty {
if alpnLen >= 256 {
panic("invalid ALPN protocol")
}
@@ -784,7 +786,7 @@
copy(z, m.secureRenegotiation)
z = z[len(m.secureRenegotiation):]
}
- if alpnLen := len(m.alpnProtocol); alpnLen > 0 {
+ if alpnLen := len(m.alpnProtocol); alpnLen > 0 || m.alpnProtocolEmpty {
z[0] = byte(extensionALPN >> 8)
z[1] = byte(extensionALPN & 0xff)
l := 2 + 1 + alpnLen
@@ -869,6 +871,7 @@
m.ocspStapling = false
m.ticketSupported = false
m.alpnProtocol = ""
+ m.alpnProtocolEmpty = false
m.extendedMasterSecret = false
if len(data) == 0 {
@@ -940,6 +943,7 @@
}
d = d[1:]
m.alpnProtocol = string(d)
+ m.alpnProtocolEmpty = len(d) == 0
case extensionChannelID:
if length > 0 {
return false
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 220e30c..3902ed3 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -285,7 +285,12 @@
}
if len(hs.clientHello.alpnProtocols) > 0 {
- if selectedProto, fallback := mutualProtocol(hs.clientHello.alpnProtocols, c.config.NextProtos); !fallback {
+ if proto := c.config.Bugs.ALPNProtocol; proto != nil {
+ hs.hello.alpnProtocol = *proto
+ hs.hello.alpnProtocolEmpty = len(*proto) == 0
+ c.clientProtocol = *proto
+ c.usedALPN = true
+ } else if selectedProto, fallback := mutualProtocol(hs.clientHello.alpnProtocols, c.config.NextProtos); !fallback {
hs.hello.alpnProtocol = selectedProto
c.clientProtocol = selectedProto
c.usedALPN = true
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 2b56462..9dee3b3 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2829,6 +2829,38 @@
expectedNextProtoType: alpn,
resumeSession: true,
})
+ var emptyString string
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "ALPNClient-EmptyProtocolName",
+ config: Config{
+ NextProtos: []string{""},
+ Bugs: ProtocolBugs{
+ // A server returning an empty ALPN protocol
+ // should be rejected.
+ ALPNProtocol: &emptyString,
+ },
+ },
+ flags: []string{
+ "-advertise-alpn", "\x03foo",
+ },
+ shouldFail: true,
+ expectedError: ":PARSE_TLSEXT:",
+ })
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "ALPNServer-EmptyProtocolName",
+ config: Config{
+ // A ClientHello containing an empty ALPN protocol
+ // should be rejected.
+ NextProtos: []string{"foo", "", "baz"},
+ },
+ flags: []string{
+ "-select-alpn", "foo",
+ },
+ shouldFail: true,
+ expectedError: ":PARSE_TLSEXT:",
+ })
// Resume with a corrupt ticket.
testCases = append(testCases, testCase{
testType: serverTest,
