upstream patches for net/http

Fixes: bz1250353 bz1250352 bz1250374
2015-08-05 16:45:13 -04:00 · 2015-08-05 16:45:13 -04:00 · aa501a2cd8
parent a8686d833c
commit aa501a2cd8
4 changed files with 487 additions and 1 deletions
--- a/117ddcb83d7f42d6aa72241240af99ded81118e9.patch
+++ b/117ddcb83d7f42d6aa72241240af99ded81118e9.patch
@ -0,0 +1,135 @@
+commit 117ddcb83d7f42d6aa72241240af99ded81118e9
+Author: Brad Fitzpatrick <bradfitz@golang.org>
+Date:   Tue Jun 30 09:22:41 2015 -0700
+
+    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 153b943..d847b13 100644
+--- 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 e4b8f6b..91303fe 100644
+--- 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 6bbd993..8fce7dd 100644
+--- 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 {
--- a/143822585e32449860e624cace9d2e521deee62e.patch
+++ b/143822585e32449860e624cace9d2e521deee62e.patch
@ -0,0 +1,112 @@
+commit 143822585e32449860e624cace9d2e521deee62e
+Author: Brad Fitzpatrick <bradfitz@golang.org>
+Date:   Tue Jul 7 13:19:44 2015 -0600
+
+    net/http: revert overly-strict part of earlier smuggling defense
+    
+    The recent https://golang.org/cl/11810 is reportedly a bit too
+    aggressive.
+    
+    Apparently some HTTP requests in the wild do contain both a
+    Transfer-Encoding along with a bogus Content-Length. Instead of
+    returning a 400 Bad Request error, we should just ignore the
+    Content-Length like we did before.
+    
+    Change-Id: I0001be90d09f8293a34f04691f608342875ff5c4
+    Reviewed-on: https://go-review.googlesource.com/11962
+    Reviewed-by: Andrew Gerrand <adg@golang.org>
+    Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
+    TryBot-Result: Gobot Gobot <gobot@golang.org>
+
+diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go
+index 1a3cf91..60e2be4 100644
+--- a/src/net/http/readrequest_test.go
+++ b/src/net/http/readrequest_test.go
+@@ -178,6 +178,36 @@ var reqTests = []reqTest{
+ 		noError,
+ 	},
+ 
+	// Tests chunked body and a bogus Content-Length which should be deleted.
+	{
+		"POST / HTTP/1.1\r\n" +
+			"Host: foo.com\r\n" +
+			"Transfer-Encoding: chunked\r\n" +
+			"Content-Length: 9999\r\n\r\n" + // to be removed.
+			"3\r\nfoo\r\n" +
+			"3\r\nbar\r\n" +
+			"0\r\n" +
+			"\r\n",
+		&Request{
+			Method: "POST",
+			URL: &url.URL{
+				Path: "/",
+			},
+			TransferEncoding: []string{"chunked"},
+			Proto:            "HTTP/1.1",
+			ProtoMajor:       1,
+			ProtoMinor:       1,
+			Header:           Header{},
+			ContentLength:    -1,
+			Host:             "foo.com",
+			RequestURI:       "/",
+		},
+
+		"foobar",
+		noTrailer,
+		noError,
+	},
+
+ 	// CONNECT request with domain name:
+ 	{
+ 		"CONNECT www.google.com:443 HTTP/1.1\r\n\r\n",
+@@ -400,11 +430,6 @@ Content-Length: 3
+ Content-Length: 4
+ 
+ abc`)},
+-	{"smuggle_chunked_and_len", reqBytes(`POST / HTTP/1.1
+-Transfer-Encoding: chunked
+-Content-Length: 3
+-
+-abc`)},
+ 	{"smuggle_content_len_head", reqBytes(`HEAD / HTTP/1.1
+ Host: foo
+ Content-Length: 5`)},
+diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go
+index 3c868bd..fbbbf24 100644
+--- a/src/net/http/transfer.go
+++ b/src/net/http/transfer.go
+@@ -430,7 +430,6 @@ func fixTransferEncoding(isResponse bool, requestMethod string, header Header) (
+ 	if !present {
+ 		return nil, nil
+ 	}
+-	isRequest := !isResponse
+ 	delete(header, "Transfer-Encoding")
+ 
+ 	encodings := strings.Split(raw[0], ",")
+@@ -458,12 +457,20 @@ func fixTransferEncoding(isResponse bool, requestMethod string, header Header) (
+ 		// RFC 7230 3.3.2 says "A sender MUST NOT send a
+ 		// Content-Length header field in any message that
+ 		// contains a Transfer-Encoding header field."
+-		if len(header["Content-Length"]) > 0 {
+-			if isRequest {
+-				return nil, errors.New("http: invalid Content-Length with Transfer-Encoding")
+-			}
+-			delete(header, "Content-Length")
+-		}
+		//
+		// but also:
+		// "If a message is received with both a
+		// Transfer-Encoding and a Content-Length header
+		// field, the Transfer-Encoding overrides the
+		// Content-Length. Such a message might indicate an
+		// attempt to perform request smuggling (Section 9.5)
+		// or response splitting (Section 9.4) and ought to be
+		// handled as an error. A sender MUST remove the
+		// received Content-Length field prior to forwarding
+		// such a message downstream."
+		//
+		// Reportedly, these appear in the wild.
+		delete(header, "Content-Length")
+ 		return te, nil
+ 	}
+ 
--- a/300d9a21583e7cf0149a778a0611e76ff7c6680f.patch
+++ b/300d9a21583e7cf0149a778a0611e76ff7c6680f.patch
@ -0,0 +1,225 @@
+commit 300d9a21583e7cf0149a778a0611e76ff7c6680f
+Author: Brad Fitzpatrick <bradfitz@golang.org>
+Date:   Tue Jun 30 14:21:15 2015 -0700
+
+    net/http: harden Server against request smuggling
+    
+    See RFC 7230.
+    
+    Thanks to Régis Leroy for the report.
+    
+    Change-Id: Ic1779bc2180900430d4d7a4938cac04ed73c304c
+    Reviewed-on: https://go-review.googlesource.com/11810
+    Reviewed-by: Russ Cox <rsc@golang.org>
+    Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
+
+diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go
+index e930d99..1a3cf91 100644
+--- a/src/net/http/readrequest_test.go
+++ b/src/net/http/readrequest_test.go
+@@ -9,6 +9,7 @@ import (
+ 	"bytes"
+ 	"fmt"
+ 	"io"
+	"io/ioutil"
+ 	"net/url"
+ 	"reflect"
+ 	"strings"
+@@ -323,6 +324,32 @@ var reqTests = []reqTest{
+ 		noTrailer,
+ 		noError,
+ 	},
+
+	// HEAD with Content-Length 0. Make sure this is permitted,
+	// since I think we used to send it.
+	{
+		"HEAD / HTTP/1.1\r\nHost: issue8261.com\r\nConnection: close\r\nContent-Length: 0\r\n\r\n",
+		&Request{
+			Method: "HEAD",
+			URL: &url.URL{
+				Path: "/",
+			},
+			Header: Header{
+				"Connection":     []string{"close"},
+				"Content-Length": []string{"0"},
+			},
+			Host:       "issue8261.com",
+			Proto:      "HTTP/1.1",
+			ProtoMajor: 1,
+			ProtoMinor: 1,
+			Close:      true,
+			RequestURI: "/",
+		},
+
+		noBody,
+		noTrailer,
+		noError,
+	},
+ }
+ 
+ func TestReadRequest(t *testing.T) {
+@@ -356,3 +383,39 @@ func TestReadRequest(t *testing.T) {
+ 		}
+ 	}
+ }
+
+// reqBytes treats req as a request (with \n delimiters) and returns it with \r\n delimiters,
+// ending in \r\n\r\n
+func reqBytes(req string) []byte {
+	return []byte(strings.Replace(strings.TrimSpace(req), "\n", "\r\n", -1) + "\r\n\r\n")
+}
+
+var badRequestTests = []struct {
+	name string
+	req  []byte
+}{
+	{"bad_connect_host", reqBytes("CONNECT []%20%48%54%54%50%2f%31%2e%31%0a%4d%79%48%65%61%64%65%72%3a%20%31%32%33%0a%0a HTTP/1.0")},
+	{"smuggle_two_contentlen", reqBytes(`POST / HTTP/1.1
+Content-Length: 3
+Content-Length: 4
+
+abc`)},
+	{"smuggle_chunked_and_len", reqBytes(`POST / HTTP/1.1
+Transfer-Encoding: chunked
+Content-Length: 3
+
+abc`)},
+	{"smuggle_content_len_head", reqBytes(`HEAD / HTTP/1.1
+Host: foo
+Content-Length: 5`)},
+}
+
+func TestReadRequest_Bad(t *testing.T) {
+	for _, tt := range badRequestTests {
+		got, err := ReadRequest(bufio.NewReader(bytes.NewReader(tt.req)))
+		if err == nil {
+			all, err := ioutil.ReadAll(got.Body)
+			t.Errorf("%s: got unexpected request = %#v\n  Body = %q, %v", tt.name, got, all, err)
+		}
+	}
+}
+diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go
+index 5205003..3887604 100644
+--- a/src/net/http/transfer.go
+++ b/src/net/http/transfer.go
+@@ -143,6 +143,9 @@ func (t *transferWriter) shouldSendContentLength() bool {
+ 		return true
+ 	}
+ 	if t.ContentLength == 0 && isIdentity(t.TransferEncoding) {
+		if t.Method == "GET" || t.Method == "HEAD" {
+			return false
+		}
+ 		return true
+ 	}
+ 
+@@ -310,6 +313,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
+ 		}
+ 	case *Request:
+ 		t.Header = rr.Header
+		t.RequestMethod = rr.Method
+ 		t.ProtoMajor = rr.ProtoMajor
+ 		t.ProtoMinor = rr.ProtoMinor
+ 		// Transfer semantics for Requests are exactly like those for
+@@ -325,7 +329,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
+ 	}
+ 
+ 	// Transfer encoding, content length
+-	t.TransferEncoding, err = fixTransferEncoding(t.RequestMethod, t.Header)
+	t.TransferEncoding, err = fixTransferEncoding(isResponse, t.RequestMethod, t.Header)
+ 	if err != nil {
+ 		return err
+ 	}
+@@ -413,12 +417,12 @@ func chunked(te []string) bool { return len(te) > 0 && te[0] == "chunked" }
+ func isIdentity(te []string) bool { return len(te) == 1 && te[0] == "identity" }
+ 
+ // Sanitize transfer encoding
+-func fixTransferEncoding(requestMethod string, header Header) ([]string, error) {
+func fixTransferEncoding(isResponse bool, requestMethod string, header Header) ([]string, error) {
+ 	raw, present := header["Transfer-Encoding"]
+ 	if !present {
+ 		return nil, nil
+ 	}
+-
+	isRequest := !isResponse
+ 	delete(header, "Transfer-Encoding")
+ 
+ 	encodings := strings.Split(raw[0], ",")
+@@ -443,10 +447,15 @@ func fixTransferEncoding(requestMethod string, header Header) ([]string, error)
+ 		return nil, &badStringError{"too many transfer encodings", strings.Join(te, ",")}
+ 	}
+ 	if len(te) > 0 {
+-		// Chunked encoding trumps Content-Length. See RFC 2616
+-		// Section 4.4. Currently len(te) > 0 implies chunked
+-		// encoding.
+-		delete(header, "Content-Length")
+		// RFC 7230 3.3.2 says "A sender MUST NOT send a
+		// Content-Length header field in any message that
+		// contains a Transfer-Encoding header field."
+		if len(header["Content-Length"]) > 0 {
+			if isRequest {
+				return nil, errors.New("http: invalid Content-Length with Transfer-Encoding")
+			}
+			delete(header, "Content-Length")
+		}
+ 		return te, nil
+ 	}
+ 
+@@ -457,9 +466,17 @@ func fixTransferEncoding(requestMethod string, header Header) ([]string, error)
+ // function is not a method, because ultimately it should be shared by
+ // ReadResponse and ReadRequest.
+ func fixLength(isResponse bool, status int, requestMethod string, header Header, te []string) (int64, error) {
+-
+	contentLens := header["Content-Length"]
+	isRequest := !isResponse
+ 	// Logic based on response type or status
+ 	if noBodyExpected(requestMethod) {
+		// For HTTP requests, as part of hardening against request
+		// smuggling (RFC 7230), don't allow a Content-Length header for
+		// methods which don't permit bodies. As an exception, allow
+		// exactly one Content-Length header if its value is "0".
+		if isRequest && len(contentLens) > 0 && !(len(contentLens) == 1 && contentLens[0] == "0") {
+			return 0, fmt.Errorf("http: method cannot contain a Content-Length; got %q", contentLens)
+		}
+ 		return 0, nil
+ 	}
+ 	if status/100 == 1 {
+@@ -470,13 +487,21 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header,
+ 		return 0, nil
+ 	}
+ 
+	if len(contentLens) > 1 {
+		// harden against HTTP request smuggling. See RFC 7230.
+		return 0, errors.New("http: message cannot contain multiple Content-Length headers")
+	}
+
+ 	// Logic based on Transfer-Encoding
+ 	if chunked(te) {
+ 		return -1, nil
+ 	}
+ 
+ 	// Logic based on Content-Length
+-	cl := strings.TrimSpace(header.get("Content-Length"))
+	var cl string
+	if len(contentLens) == 1 {
+		cl = strings.TrimSpace(contentLens[0])
+	}
+ 	if cl != "" {
+ 		n, err := parseContentLength(cl)
+ 		if err != nil {
+@@ -487,11 +512,14 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header,
+ 		header.Del("Content-Length")
+ 	}
+ 
+-	if !isResponse && requestMethod == "GET" {
+-		// RFC 2616 doesn't explicitly permit nor forbid an
+	if !isResponse {
+		// RFC 2616 neither explicitly permits nor forbids an
+ 		// entity-body on a GET request so we permit one if
+ 		// declared, but we default to 0 here (not -1 below)
+ 		// if there's no mention of a body.
+		// Likewise, all other request methods are assumed to have
+		// no body if neither Transfer-Encoding chunked nor a
+		// Content-Length are set.
+ 		return 0, nil
+ 	}
+ 
--- a/golang.spec
+++ b/golang.spec
@ -40,7 +40,7 @@

 Name:           golang
 Version:        1.4.2
-Release:        2%{?dist}
+Release:        3%{?dist}
 Summary:        The Go Programming Language

 License:        BSD
@ -68,6 +68,12 @@ Patch0:         golang-1.2-verbose-build.patch
 # https://bugzilla.redhat.com/show_bug.cgi?id=1038683
 Patch1:         golang-1.2-remove-ECC-p224.patch

+# TODO this should be removed with go1.4.3
+# https://bugzilla.redhat.com/show_bug.cgi?id=1250352
+Patch100:	300d9a21583e7cf0149a778a0611e76ff7c6680f.patch
+Patch101:	117ddcb83d7f42d6aa72241240af99ded81118e9.patch
+Patch102:	143822585e32449860e624cace9d2e521deee62e.patch
+
 # Having documentation separate was broken
 Obsoletes:      %{name}-docs < 1.1-4

@ -324,6 +330,11 @@ end
 # remove the P224 curve
 %patch1 -p1

+# bz1250352
+%patch100 -p1
+%patch101 -p1
+%patch102 -p1
+
 %build
 # set up final install location
 export GOROOT_FINAL=%{goroot}
@ -739,6 +750,9 @@ fi


 %changelog
+* Wed Aug 05 2015 Vincent Batts <vbatts@fedoraproject.org> - 1.4.2-3
+- bz1250352
+
 * Wed Mar 18 2015 Vincent Batts <vbatts@fedoraproject.org> - 1.4.2-2
 - obsoleting deprecated packages