src/net/http/internal/http2/frame.go | 48 +++++++++++++++++++++++++++++++----------------- src/net/http/internal/http2/frame_test.go | 32 ++++++++++++++++++++++++++++++++ src/net/http/serve_test.go | 18 ++++++++++++++++++ src/net/http/server.go | 9 ++++++--- src/net/http/transfer.go | 10 ++++++++-- diff --git a/src/net/http/internal/http2/frame.go b/src/net/http/internal/http2/frame.go index e3b371acec42a3b7364abafac6205c2a23bdb09f..d96472dcb7824d6c9c1ee5904a4842ad6206d615 100644 --- a/src/net/http/internal/http2/frame.go +++ b/src/net/http/internal/http2/frame.go @@ -1723,9 +1723,13 @@ } mh := &MetaHeadersFrame{ HeadersFrame: hf, } - var remainSize = fr.maxHeaderListSize() + type headerBudget struct { + remainSize uint32 + count int + } + headers := headerBudget{remainSize: fr.maxHeaderListSize()} + trailers := headerBudget{remainSize: fr.maxHeaderListSize()} var sawRegular bool - var headerCount int var invalid error // pseudo header field errors hdec := fr.ReadMetaHeaders @@ -1734,13 +1738,6 @@ hdec.SetMaxStringLength(fr.maxHeaderStringLen()) hdec.SetEmitFunc(func(hf hpack.HeaderField) { if VerboseLogs && fr.logReads { fr.debugReadLoggerf("http2: decoded hpack field %+v", hf) - } - headerCount++ - if limit := fr.maxHeaderValueCount(); limit > 0 && headerCount > limit { - hdec.SetEmitEnabled(false) - mh.Truncated = true - remainSize = 0 - return } if !httpguts.ValidHeaderFieldValue(hf.Value) { // Don't include the value in the error, because it may be sensitive. @@ -1763,14 +1760,31 @@ hdec.SetEmitEnabled(false) return } - size := hf.Size() - if size > remainSize { + var budget *headerBudget + var size uint32 + if hf.Name == "trailer" { + budget = &trailers + fieldCount := strings.Count(hf.Value, ",") + 1 + budget.count += fieldCount + // Rather than actually constructing hpack.HeaderField for each + // trailer field and cumulatively adding its Size, just do the math + // manually to avoid unnecessary work. This does make it so + // whitespaces after comma are counted against the budget, but that + // should be innocuous. + size = uint32(len(hf.Value)-fieldCount+1) + uint32(fieldCount)*hpack.HeaderField{}.Size() + } else { + budget = &headers + budget.count++ + size = hf.Size() + } + if countLimit := fr.maxHeaderValueCount(); (countLimit > 0 && budget.count > countLimit) || size > budget.remainSize { hdec.SetEmitEnabled(false) mh.Truncated = true - remainSize = 0 + headers.remainSize = 0 + trailers.remainSize = 0 return } - remainSize -= size + budget.remainSize -= size mh.Fields = append(mh.Fields, hf) }) @@ -1786,10 +1800,10 @@ // If the sender exceeds the max header list size by too much, // skip parsing the fragment and close the connection. // // "Too much" is either any CONTINUATION frame after we've already - // exceeded the max header list size (in which case remainSize is 0), - // or a frame whose encoded size is more than twice the remaining - // header list bytes we're willing to accept. - if int64(len(frag)) > int64(2*remainSize) { + // exceeded the max header list size (if so, both budgets are 0), or a + // frame whose encoded size is more than twice the remaining header + // list bytes we're willing to accept. + if int64(len(frag)) > 2*int64(headers.remainSize+trailers.remainSize) { if VerboseLogs { log.Printf("http2: header list too large") } diff --git a/src/net/http/internal/http2/frame_test.go b/src/net/http/internal/http2/frame_test.go index 43cd66bfc3bdc0920b50d1abcc57821228c6bd53..67436c6d1b1082718db2dd5e1812fed679f5d346 100644 --- a/src/net/http/internal/http2/frame_test.go +++ b/src/net/http/internal/http2/frame_test.go @@ -1129,6 +1129,15 @@ const noFlags Flags = 0 oneKBString := strings.Repeat("a", 1<<10) + // A Trailer declaration of 20 fields: ~250 bytes on the wire, but ~900 + // bytes by our accounting since we add 32 bytes for each entry in + // Request.Trailer. + var trailerNames []string + for i := range 20 { + trailerNames = append(trailerNames, fmt.Sprintf("x-trailer-%d", i)) + } + trailerDecl := strings.Join(trailerNames, ",") + tests := [...]struct { name string w func(*Framer) @@ -1265,6 +1274,29 @@ name: "invalid_field_value", w: func(f *Framer) { write(f, encodeHeaderRaw(t, "key", "bad_null\x00")) }, want: streamError(1, ErrCodeProtocol), wantErrReason: `invalid header field value for "key"`, + }, + 13: { + name: "trailer_declaration_okay", + w: func(f *Framer) { + write(f, encodeHeaderRaw(t, ":method", "GET", ":path", "/", "trailer", trailerDecl)) + }, + maxHeaderListSize: 1024, + want: want(FlagHeadersEndHeaders, 193, + ":method", "GET", + ":path", "/", + "trailer", trailerDecl, + ), + }, + 14: { + name: "trailer_declaration_truncated", + w: func(f *Framer) { + write(f, encodeHeaderRaw(t, ":method", "GET", ":path", "/", "trailer", trailerDecl)) + }, + maxHeaderListSize: 512, + want: truncated(want(FlagHeadersEndHeaders, 193, + ":method", "GET", + ":path", "/", + )), }, } for i, tt := range tests { diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 57325790cabeb6615f4e48cd9a7e5414253a35e3..9051d332e8984c010f8c2e21733699fbdf94ac67 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -3477,6 +3477,24 @@ } }, wantStatus: 431, }, + { + // Comma separated Trailer values are counted as multiple, because + // each value becomes its own field / a key in Request.Trailer. + // This is different from TestRequestTrailerHeaderValueCountLimit + // which tests the actual sending of the trailer, this just tests + // the Trailer header declaration. + name: "comma separated trailer values count as multiple", + limit: 15, + setup: func(req *Request) { + req.Body = NoBody + req.TransferEncoding = []string{"chunked"} + req.Trailer = make(Header) + for i := range 16 { + req.Trailer[fmt.Sprintf("X-Trailer-%d", i)] = nil + } + }, + wantStatus: 431, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/src/net/http/server.go b/src/net/http/server.go index f71266538eb06cc4ac72f82e531c51b5ddf2251e..96bb12b2f38bcfe1e0320e73a8f5f4046b1754a6 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -3120,9 +3120,12 @@ // MaxHeaderValueCount controls the maximum number of header // values that the server is willing to parse from a request. // If zero, DefaultMaxHeaderValueCount is used. - // Note that comma-separated values in a single header line are - // counted once, while values sent as multiple header lines are - // counted multiple times. + // Comma-separated values in a single header line are counted + // once, while values sent as multiple header lines are + // counted multiple times. An exception to this is the Trailer + // header, whose comma-separated values are counted separately, + // as each of them is expected to be received later as an + // individual trailer header line. MaxHeaderValueCount int // TLSNextProto optionally specifies a function to take over diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index e40edbccef4887afe2b1e5f4ea9133f6f643f7dd..997b9a00404364d54a7a73214e71e5ea0df80680 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go @@ -550,7 +550,7 @@ t.ContentLength = realLength } // Trailer - t.Trailer, err = fixTrailer(t.Header, t.Chunked) + t.Trailer, err = fixTrailer(t.Header, t.Chunked, maxTrailerHeaders) if err != nil { return err } @@ -772,7 +772,7 @@ return hasClose } // Parse the trailer header. -func fixTrailer(header Header, chunked bool) (Header, error) { +func fixTrailer(header Header, chunked bool, maxHeaders int64) (Header, error) { vv, ok := header["Trailer"] if !ok { return nil, nil @@ -788,6 +788,12 @@ // See issue #27197. return nil, nil } header.Del("Trailer") + for _, v := range vv { + maxHeaders -= int64(strings.Count(v, ",") + 1) + if maxHeaders < 0 { + return nil, errTooLarge + } + } trailer := make(Header) var err error