From: Brad Fitzpatrick Date: Wed, 20 Jan 2016 22:41:52 +0000 (+0000) Subject: net/http: update http2 to check header values, move from vendor to internal X-Git-Tag: go1.6rc1~84 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=e8e1928bd22339dcdbfde17778cfd976184bb377;p=gostls13.git net/http: update http2 to check header values, move from vendor to internal Updates x/net/http2 to git rev b2ed34f for https://golang.org/cl/18727 Updates #14029 (fixes it enough for Go 1.6) Fixes #13961 Change-Id: Id301247545507671f4e79df0e7c6ec9c421d5a7c Reviewed-on: https://go-review.googlesource.com/18728 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Andrew Gerrand --- diff --git a/src/go/build/build_test.go b/src/go/build/build_test.go index 61aac8fe5f..7312af08b5 100644 --- a/src/go/build/build_test.go +++ b/src/go/build/build_test.go @@ -300,6 +300,7 @@ func TestShellSafety(t *testing.T) { } func TestImportVendor(t *testing.T) { + t.Skip("skipping; hpack has moved to internal for now; golang.org/issue/14047") testenv.MustHaveGoBuild(t) // really must just have source ctxt := Default ctxt.GOPATH = "" diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go index 4603102526..376931e198 100644 --- a/src/go/build/deps_test.go +++ b/src/go/build/deps_test.go @@ -358,7 +358,7 @@ var pkgDeps = map[string][]string{ "L4", "NET", "OS", "compress/gzip", "crypto/tls", "mime/multipart", "runtime/debug", "net/http/internal", - "golang.org/x/net/http2/hpack", + "internal/golang.org/x/net/http2/hpack", }, "net/http/internal": {"L4"}, diff --git a/src/vendor/golang.org/x/net/http2/hpack/encode.go b/src/internal/golang.org/x/net/http2/hpack/encode.go similarity index 100% rename from src/vendor/golang.org/x/net/http2/hpack/encode.go rename to src/internal/golang.org/x/net/http2/hpack/encode.go diff --git a/src/vendor/golang.org/x/net/http2/hpack/encode_test.go b/src/internal/golang.org/x/net/http2/hpack/encode_test.go similarity index 100% rename from src/vendor/golang.org/x/net/http2/hpack/encode_test.go rename to src/internal/golang.org/x/net/http2/hpack/encode_test.go diff --git a/src/vendor/golang.org/x/net/http2/hpack/hpack.go b/src/internal/golang.org/x/net/http2/hpack/hpack.go similarity index 100% rename from src/vendor/golang.org/x/net/http2/hpack/hpack.go rename to src/internal/golang.org/x/net/http2/hpack/hpack.go diff --git a/src/vendor/golang.org/x/net/http2/hpack/hpack_test.go b/src/internal/golang.org/x/net/http2/hpack/hpack_test.go similarity index 100% rename from src/vendor/golang.org/x/net/http2/hpack/hpack_test.go rename to src/internal/golang.org/x/net/http2/hpack/hpack_test.go diff --git a/src/vendor/golang.org/x/net/http2/hpack/huffman.go b/src/internal/golang.org/x/net/http2/hpack/huffman.go similarity index 100% rename from src/vendor/golang.org/x/net/http2/hpack/huffman.go rename to src/internal/golang.org/x/net/http2/hpack/huffman.go diff --git a/src/vendor/golang.org/x/net/http2/hpack/tables.go b/src/internal/golang.org/x/net/http2/hpack/tables.go similarity index 100% rename from src/vendor/golang.org/x/net/http2/hpack/tables.go rename to src/internal/golang.org/x/net/http2/hpack/tables.go diff --git a/src/net/http/h2_bundle.go b/src/net/http/h2_bundle.go index cd530f16cd..bdbdadb5b2 100644 --- a/src/net/http/h2_bundle.go +++ b/src/net/http/h2_bundle.go @@ -24,6 +24,7 @@ import ( "encoding/binary" "errors" "fmt" + "internal/golang.org/x/net/http2/hpack" "io" "io/ioutil" "log" @@ -37,8 +38,6 @@ import ( "strings" "sync" "time" - - "golang.org/x/net/http2/hpack" ) // ClientConnPool manages a pool of HTTP/2 client connections. @@ -2065,13 +2064,60 @@ func (s http2SettingID) String() string { return fmt.Sprintf("UNKNOWN_SETTING_%d", uint16(s)) } -func http2validHeader(v string) bool { +var ( + http2errInvalidHeaderFieldName = errors.New("http2: invalid header field name") + http2errInvalidHeaderFieldValue = errors.New("http2: invalid header field value") +) + +// validHeaderFieldName reports whether v is a valid header field name (key). +// RFC 7230 says: +// header-field = field-name ":" OWS field-value OWS +// field-name = token +// tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / +// "^" / "_" / " +// Further, http2 says: +// "Just as in HTTP/1.x, header field names are strings of ASCII +// characters that are compared in a case-insensitive +// fashion. However, header field names MUST be converted to +// lowercase prior to their encoding in HTTP/2. " +func http2validHeaderFieldName(v string) bool { if len(v) == 0 { return false } for _, r := range v { + if int(r) >= len(http2isTokenTable) || ('A' <= r && r <= 'Z') { + return false + } + if !http2isTokenTable[byte(r)] { + return false + } + } + return true +} - if r >= 127 || ('A' <= r && r <= 'Z') { +// validHeaderFieldValue reports whether v is a valid header field value. +// +// RFC 7230 says: +// field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] +// field-vchar = VCHAR / obs-text +// obs-text = %x80-FF +// VCHAR = "any visible [USASCII] character" +// +// http2 further says: "Similarly, HTTP/2 allows header field values +// that are not valid. While most of the values that can be encoded +// will not alter header field parsing, carriage return (CR, ASCII +// 0xd), line feed (LF, ASCII 0xa), and the zero character (NUL, ASCII +// 0x0) might be exploited by an attacker if they are translated +// verbatim. Any request or response that contains a character not +// permitted in a header field value MUST be treated as malformed +// (Section 8.1.2.6). Valid characters are defined by the +// field-content ABNF rule in Section 3.2 of [RFC7230]." +// +// This function does not (yet?) properly handle the rejection of +// strings that begin or end with SP or HTAB. +func http2validHeaderFieldValue(v string) bool { + for i := 0; i < len(v); i++ { + if b := v[i]; b < ' ' && b != '\t' { return false } } @@ -2202,6 +2248,86 @@ func (e *http2httpError) Temporary() bool { return true } var http2errTimeout error = &http2httpError{msg: "http2: timeout awaiting response headers", timeout: true} +var http2isTokenTable = [127]bool{ + '!': true, + '#': true, + '$': true, + '%': true, + '&': true, + '\'': true, + '*': true, + '+': true, + '-': true, + '.': true, + '0': true, + '1': true, + '2': true, + '3': true, + '4': true, + '5': true, + '6': true, + '7': true, + '8': true, + '9': true, + 'A': true, + 'B': true, + 'C': true, + 'D': true, + 'E': true, + 'F': true, + 'G': true, + 'H': true, + 'I': true, + 'J': true, + 'K': true, + 'L': true, + 'M': true, + 'N': true, + 'O': true, + 'P': true, + 'Q': true, + 'R': true, + 'S': true, + 'T': true, + 'U': true, + 'W': true, + 'V': true, + 'X': true, + 'Y': true, + 'Z': true, + '^': true, + '_': true, + '`': true, + 'a': true, + 'b': true, + 'c': true, + 'd': true, + 'e': true, + 'f': true, + 'g': true, + 'h': true, + 'i': true, + 'j': true, + 'k': true, + 'l': true, + 'm': true, + 'n': true, + 'o': true, + 'p': true, + 'q': true, + 'r': true, + 's': true, + 't': true, + 'u': true, + 'v': true, + 'w': true, + 'x': true, + 'y': true, + 'z': true, + '|': true, + '~': true, +} + // pipe is a goroutine-safe io.Reader/io.Writer pair. It's like // io.Pipe except there are no PipeReader/PipeWriter halves, and the // underlying buffer is an interface. (io.Pipe is always unbuffered) @@ -2741,7 +2867,7 @@ func (sc *http2serverConn) onNewHeaderField(f hpack.HeaderField) { sc.vlogf("http2: server decoded %v", f) } switch { - case !http2validHeader(f.Name): + case !http2validHeaderFieldValue(f.Value): sc.req.invalidHeader = true case strings.HasPrefix(f.Name, ":"): if sc.req.sawRegularHeader { @@ -2771,6 +2897,8 @@ func (sc *http2serverConn) onNewHeaderField(f hpack.HeaderField) { return } *dst = f.Value + case !http2validHeaderFieldName(f.Name): + sc.req.invalidHeader = true default: sc.req.sawRegularHeader = true sc.req.header.Add(sc.canonicalHeader(f.Name), f.Value) @@ -2789,10 +2917,10 @@ func (st *http2stream) onNewTrailerField(f hpack.HeaderField) { sc.vlogf("http2: server decoded trailer %v", f) } switch { - case !http2validHeader(f.Name): + case strings.HasPrefix(f.Name, ":"): sc.req.invalidHeader = true return - case strings.HasPrefix(f.Name, ":"): + case !http2validHeaderFieldName(f.Name) || !http2validHeaderFieldValue(f.Value): sc.req.invalidHeader = true return default: @@ -5697,7 +5825,6 @@ func (cc *http2ClientConn) writeStreamReset(streamID uint32, code http2ErrCode, var ( http2errResponseHeaderListSize = errors.New("http2: response header list larger than advertised limit") - http2errInvalidHeaderKey = errors.New("http2: invalid header key") http2errPseudoTrailers = errors.New("http2: invalid pseudo header in trailers") ) @@ -5714,8 +5841,8 @@ func (rl *http2clientConnReadLoop) checkHeaderField(f hpack.HeaderField) bool { return false } - if !http2validHeader(f.Name) { - rl.reqMalformed = http2errInvalidHeaderKey + if !http2validHeaderFieldValue(f.Value) { + rl.reqMalformed = http2errInvalidHeaderFieldValue return false } @@ -5726,6 +5853,10 @@ func (rl *http2clientConnReadLoop) checkHeaderField(f hpack.HeaderField) bool { return false } } else { + if !http2validHeaderFieldName(f.Name) { + rl.reqMalformed = http2errInvalidHeaderFieldName + return false + } rl.sawRegHeader = true } diff --git a/src/vendor/README b/src/vendor/README new file mode 100644 index 0000000000..e540318bb2 --- /dev/null +++ b/src/vendor/README @@ -0,0 +1,8 @@ +This file needs to exist because the vendor directory needs +to exist for some go/build tests to pass, and git can't track +empty directories. + +In Go 1.7 we'll use this directory again. (In Go 1.6 we tried but +reverted). + +See http://golang.org/issue/14047 for details.