net/textproto: don't treat spaces as hyphens in header keys

This was originally done in https://codereview.appspot.com/5690059
(Feb 2012) to deal with bad response headers coming back from webcams,
but it presents a potential security problem with HTTP request
smuggling for request headers containing "Content Length" instead of
"Content-Length".

Part of overall HTTP hardening for request smuggling. See RFC 7230.

Thanks to Régis Leroy for the report.

Change-Id: I92b17fb637c9171c5774ea1437979ae2c17ca88a
Reviewed-on: https://go-review.googlesource.com/11772
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/net/http/header.go b/src/net/http/header.go
index 153b94370f856fd8ce23b8c626468b2b89590ead..d847b131184d3eac715b67de5ffb1d6f5fcaf6c7 100644 (file)
--- a/src/net/http/header.go
+++ b/src/net/http/header.go
@@ -168,6 +168,8 @@ func (h Header) WriteSubset(w io.Writer, exclude map[string]bool) error {
 // letter and any letter following a hyphen to upper case;
 // the rest are converted to lowercase.  For example, the
 // canonical key for "accept-encoding" is "Accept-Encoding".
+// If s contains a space or invalid header field bytes, it is
+// returned without modifications.
 func CanonicalHeaderKey(s string) string { return textproto.CanonicalMIMEHeaderKey(s) }
 
 // hasToken reports whether token appears with v, ASCII

diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go
index e4b8f6bb9125c58d4bf25e18bdea8c77c0c1701d..91303fec612dfcfc8e21edf9e653ec638ea180ff 100644 (file)
--- a/src/net/textproto/reader.go
+++ b/src/net/textproto/reader.go
@@ -547,11 +547,16 @@ func (r *Reader) upcomingHeaderNewlines() (n int) {
 // the rest are converted to lowercase.  For example, the
 // canonical key for "accept-encoding" is "Accept-Encoding".
 // MIME header keys are assumed to be ASCII only.
+// If s contains a space or invalid header field bytes, it is
+// returned without modifications.
 func CanonicalMIMEHeaderKey(s string) string {
        // Quick check for canonical encoding.
        upper := true
        for i := 0; i < len(s); i++ {
                c := s[i]
+               if !validHeaderFieldByte(c) {
+                       return s
+               }
                if upper && 'a' <= c && c <= 'z' {
                        return canonicalMIMEHeaderKey([]byte(s))
                }
@@ -565,19 +570,44 @@ func CanonicalMIMEHeaderKey(s string) string {
 
 const toLower = 'a' - 'A'
 
+// validHeaderFieldByte reports whether b is a valid byte in a header
+// field key. This is actually stricter than RFC 7230, which says:
+//   tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
+//           "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
+//   token = 1*tchar
+// TODO: revisit in Go 1.6+ and possibly expand this. But note that many
+// servers have historically dropped '_' to prevent ambiguities when mapping
+// to CGI environment variables.
+func validHeaderFieldByte(b byte) bool {
+       return ('A' <= b && b <= 'Z') ||
+               ('a' <= b && b <= 'z') ||
+               ('0' <= b && b <= '9') ||
+               b == '-'
+}
+
 // canonicalMIMEHeaderKey is like CanonicalMIMEHeaderKey but is
 // allowed to mutate the provided byte slice before returning the
 // string.
+//
+// For invalid inputs (if a contains spaces or non-token bytes), a
+// is unchanged and a string copy is returned.
 func canonicalMIMEHeaderKey(a []byte) string {
+       // See if a looks like a header key. If not, return it unchanged.
+       for _, c := range a {
+               if validHeaderFieldByte(c) {
+                       continue
+               }
+               // Don't canonicalize.
+               return string(a)
+       }
+
        upper := true
        for i, c := range a {
                // Canonicalize: first letter upper case
                // and upper case after each dash.
                // (Host, User-Agent, If-Modified-Since).
                // MIME headers are ASCII only, so no Unicode issues.
-               if c == ' ' {
-                       c = '-'
-               } else if upper && 'a' <= c && c <= 'z' {
+               if upper && 'a' <= c && c <= 'z' {
                        c -= toLower
                } else if !upper && 'A' <= c && c <= 'Z' {
                        c += toLower

diff --git a/src/net/textproto/reader_test.go b/src/net/textproto/reader_test.go
index 6bbd993b8c5bd3c9ae2491b14256fe80157c85e2..8fce7ddeb1e91f0998ad2b804062fb8279ab804b 100644 (file)
--- a/src/net/textproto/reader_test.go
+++ b/src/net/textproto/reader_test.go
@@ -24,11 +24,14 @@ var canonicalHeaderKeyTests = []canonicalHeaderKeyTest{
        {"uSER-aGENT", "User-Agent"},
        {"user-agent", "User-Agent"},
        {"USER-AGENT", "User-Agent"},
-       {"üser-agenT", "üser-Agent"}, // non-ASCII unchanged
+
+       // Non-ASCII or anything with spaces or non-token chars is unchanged:
+       {"üser-agenT", "üser-agenT"},
+       {"a B", "a B"},
 
        // This caused a panic due to mishandling of a space:
-       {"C Ontent-Transfer-Encoding", "C-Ontent-Transfer-Encoding"},
-       {"foo bar", "Foo-Bar"},
+       {"C Ontent-Transfer-Encoding", "C Ontent-Transfer-Encoding"},
+       {"foo bar", "foo bar"},
 }
 
 func TestCanonicalMIMEHeaderKey(t *testing.T) {
@@ -194,7 +197,7 @@ func TestReadMIMEHeaderNonCompliant(t *testing.T) {
                "Foo":              {"bar"},
                "Content-Language": {"en"},
                "Sid":              {"0"},
-               "Audio-Mode":       {"None"},
+               "Audio Mode":       {"None"},
                "Privilege":        {"127"},
        }
        if !reflect.DeepEqual(m, want) || err != nil {