backport various security fixes:

- CVE-2015-5739, "Content Length" treated as valid header
- CVE-2015-5740, Double content-length headers does not return 400 error
- CVE-2015-5741, Additional hardening, not sending Content-Length w/Transfer-Encoding, Closing connections

from upstream git

ok jsing@ (MAINTAINER), czarkoff@
This commit is contained in:
jasper 2015-08-19 06:57:20 +00:00
parent 581ddd2f25
commit b7f5bf9dcf
6 changed files with 342 additions and 1 deletions

View File

@ -1,10 +1,11 @@
# $OpenBSD: Makefile,v 1.25 2015/05/06 08:28:26 jasper Exp $
# $OpenBSD: Makefile,v 1.26 2015/08/19 06:57:20 jasper Exp $
ONLY_FOR_ARCHS = ${GO_ARCHS}
COMMENT = Go programming language
VERSION = 1.4.2
REVISION = 0
EXTRACT_SUFX = .src.tar.gz
DISTNAME = go${VERSION}
PKGNAME = go-${VERSION}

View File

@ -0,0 +1,16 @@
$OpenBSD: patch-src_net_http_header_go,v 1.1 2015/08/19 06:57:20 jasper Exp $
Security fix for CVE-2015-5739, "Content Length" treated as valid header
https://github.com/golang/go/commit/117ddcb83d7f42d6aa72241240af99ded81118e9
--- src/net/http/header.go.orig Wed Feb 18 05:38:34 2015
+++ src/net/http/header.go Thu Aug 13 18:10:28 2015
@@ -168,6 +168,8 @@ func (h Header) WriteSubset(w io.Writer, exclude map[s
// 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

View File

@ -0,0 +1,89 @@
$OpenBSD: patch-src_net_http_readrequest_test_go,v 1.1 2015/08/19 06:57:20 jasper Exp $
Security fix for CVE-2015-5740, Double content-length headers does not return 400 error
Security fix for CVE-2015-5741, Additional hardening, not sending Content-Length w/Transfer-Encoding, Closing connections
https://github.com/golang/go/commit/300d9a21583e7cf0149a778a0611e76ff7c6680f
--- src/net/http/readrequest_test.go.orig Wed Feb 18 05:38:34 2015
+++ src/net/http/readrequest_test.go Thu Aug 13 18:13:02 2015
@@ -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)
+ }
+ }
+ }

View File

@ -0,0 +1,129 @@
$OpenBSD: patch-src_net_http_transfer_go,v 1.1 2015/08/19 06:57:20 jasper Exp $
Security fix for CVE-2015-5740, Double content-length headers does not return 400 error
Security fix for CVE-2015-5741, Additional hardening, not sending Content-Length w/Transfer-Encoding, Closing connections
https://github.com/golang/go/commit/300d9a21583e7cf0149a778a0611e76ff7c6680f
--- src/net/http/transfer.go.orig Wed Feb 18 05:38:34 2015
+++ src/net/http/transfer.go Thu Aug 13 18:11:38 2015
@@ -143,6 +143,9 @@ func (t *transferWriter) shouldSendContentLength() boo
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) (e
}
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) (e
}
// 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 &&
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
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
// 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, requestMet
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, requestMet
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
}

View File

@ -0,0 +1,72 @@
$OpenBSD: patch-src_net_textproto_reader_go,v 1.1 2015/08/19 06:57:20 jasper Exp $
Security fix for CVE-2015-5739, "Content Length" treated as valid header
https://github.com/golang/go/commit/117ddcb83d7f42d6aa72241240af99ded81118e9
--- src/net/textproto/reader.go.orig Wed Feb 18 05:38:34 2015
+++ src/net/textproto/reader.go Thu Aug 13 18:10:28 2015
@@ -540,11 +540,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))
}
@@ -558,19 +563,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

View File

@ -0,0 +1,34 @@
$OpenBSD: patch-src_net_textproto_reader_test_go,v 1.1 2015/08/19 06:57:20 jasper Exp $
Security fix for CVE-2015-5739, "Content Length" treated as valid header
https://github.com/golang/go/commit/117ddcb83d7f42d6aa72241240af99ded81118e9
--- src/net/textproto/reader_test.go.orig Wed Feb 18 05:38:34 2015
+++ src/net/textproto/reader_test.go Thu Aug 13 18:10:28 2015
@@ -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) {
@@ -185,7 +188,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 {