Use http.MaxBytesReader on the raw body and on the gzip decode output (#42265)
- [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually --------- Co-authored-by: Juan Fernandez <juan-fdz-hawa@users.noreply.github.com>
This commit is contained in:
Lucas Manuel Rodriguez
co-authored by
Juan Fernandez
Juan Fernandez
parent
968ac3a9d7
commit
ccd50ed791
@@ -0,0 +1 @@
|
||||
* Improved body parsing validation by using http.MaxBytesReader and wrapping gzip decode output too.
|
||||
@@ -32,17 +32,6 @@ import (
|
||||
"github.com/gorilla/mux"
|
||||
)
|
||||
|
||||
// We use our own wrapper here, to preserve the Close method of the original io.ReadCloser
|
||||
// But also allows us to modify the limit at a laterp oint.
|
||||
type LimitedReadCloser struct {
|
||||
*io.LimitedReader
|
||||
Closer io.Closer
|
||||
}
|
||||
|
||||
func (lrc *LimitedReadCloser) Close() error {
|
||||
return lrc.Closer.Close()
|
||||
}
|
||||
|
||||
type HandlerRoutesFunc func(r *mux.Router, opts []kithttp.ServerOption)
|
||||
|
||||
// ParseTag parses a `url` tag and whether it's optional or not, which is an optional part of the tag
|
||||
@@ -501,23 +490,7 @@ type requestValidator interface {
|
||||
// The customQueryDecoder parameter allows services to inject domain-specific
|
||||
// query parameter decoding logic.
|
||||
//
|
||||
// If adding a new way to parse/decode the requset, make sure to wrap the body in a limited reader with the maxRequestBodySize
|
||||
|
||||
// limitExhaustedBody returns true when the LimitedReader was the cause of an
|
||||
// unexpected EOF — i.e. the underlying body actually had more data beyond the
|
||||
// limit. It does this by attempting a single-byte read from the underlying
|
||||
// reader (limitedReader.R) which bypasses the limit wrapper. A successful read
|
||||
// means the body exceeded the limit; an immediate EOF means the body ended
|
||||
// exactly at the limit (malformed JSON, not an oversized payload).
|
||||
//
|
||||
// This is used to avoid false-positive PayloadTooLargeError responses for
|
||||
// bodies whose JSON is malformed and happen to be exactly maxRequestBodySize
|
||||
// bytes long.
|
||||
func limitExhaustedBody(limitedReader *io.LimitedReader) bool {
|
||||
var peek [1]byte
|
||||
n, _ := limitedReader.R.Read(peek[:])
|
||||
return n > 0
|
||||
}
|
||||
// If adding a new way to parse/decode the request, make sure to wrap the body with http.MaxBytesReader using the maxRequestBodySize
|
||||
|
||||
func MakeDecoder(
|
||||
iface interface{},
|
||||
@@ -537,18 +510,48 @@ func MakeDecoder(
|
||||
}
|
||||
if rd, ok := iface.(RequestDecoder); ok {
|
||||
return func(ctx context.Context, r *http.Request) (interface{}, error) {
|
||||
var limitedReader *io.LimitedReader
|
||||
if maxRequestBodySize != -1 {
|
||||
limitedReader = io.LimitReader(r.Body, maxRequestBodySize).(*io.LimitedReader)
|
||||
|
||||
r.Body = &LimitedReadCloser{
|
||||
LimitedReader: limitedReader,
|
||||
Closer: r.Body,
|
||||
r.Body = http.MaxBytesReader(nil, r.Body, maxRequestBodySize)
|
||||
}
|
||||
//
|
||||
// We take care of gzip encoding here to prevent any future DecodeRequest
|
||||
// implementations from missing gzip bomb checks.
|
||||
//
|
||||
gzipped := false
|
||||
if strings.EqualFold(r.Header.Get("content-encoding"), "gzip") {
|
||||
gzipped = true
|
||||
gzr, err := gzip.NewReader(r.Body)
|
||||
if err != nil {
|
||||
return nil, BadRequestErr("gzip decoder error", err)
|
||||
}
|
||||
defer gzr.Close()
|
||||
if maxRequestBodySize != -1 {
|
||||
// Limit decompressed bytes to prevent gzip bombs from bypassing
|
||||
// the raw body size limit applied above.
|
||||
r.Body = http.MaxBytesReader(nil, gzr, maxRequestBodySize)
|
||||
} else {
|
||||
r.Body = io.NopCloser(gzr)
|
||||
}
|
||||
// Clear so implementations don't try to decompress again.
|
||||
r.Header.Del("Content-Encoding")
|
||||
}
|
||||
ret, err := rd.DecodeRequest(ctx, r)
|
||||
if err != nil && errors.Is(err, io.ErrUnexpectedEOF) && limitedReader != nil && limitedReader.N == 0 && limitExhaustedBody(limitedReader) {
|
||||
return nil, platform_http.PayloadTooLargeError{ContentLength: r.Header.Get("Content-Length"), MaxRequestSize: maxRequestBodySize}
|
||||
|
||||
// Some DecodeRequest implementations (like getHostSoftwareRequest)
|
||||
// themselves return platform_http.PayloadTooLargeError.
|
||||
if inner, isPayloadTooLargeError := errors.AsType[platform_http.PayloadTooLargeError](err); isPayloadTooLargeError {
|
||||
// Preserve the inner error's MaxRequestSize and ContentLength
|
||||
// (it knows the actual limit that was hit), only add Gzipped.
|
||||
inner.Gzipped = gzipped
|
||||
return nil, inner
|
||||
}
|
||||
|
||||
if _, isMaxBytesError := errors.AsType[*http.MaxBytesError](err); isMaxBytesError {
|
||||
return nil, platform_http.PayloadTooLargeError{
|
||||
ContentLength: r.Header.Get("Content-Length"),
|
||||
MaxRequestSize: maxRequestBodySize,
|
||||
Gzipped: gzipped,
|
||||
}
|
||||
}
|
||||
return ret, err
|
||||
}
|
||||
@@ -564,28 +567,30 @@ func MakeDecoder(
|
||||
nilBody := false
|
||||
var rewriter *JSONKeyRewriteReader
|
||||
|
||||
var limitedReader *io.LimitedReader
|
||||
if maxRequestBodySize != -1 {
|
||||
limitedReader = io.LimitReader(r.Body, maxRequestBodySize).(*io.LimitedReader)
|
||||
|
||||
r.Body = &LimitedReadCloser{
|
||||
LimitedReader: limitedReader,
|
||||
Closer: r.Body,
|
||||
}
|
||||
r.Body = http.MaxBytesReader(nil, r.Body, maxRequestBodySize)
|
||||
}
|
||||
|
||||
buf := bufio.NewReader(r.Body)
|
||||
var body io.Reader = buf
|
||||
gzipped := false
|
||||
if _, err := buf.Peek(1); err == io.EOF {
|
||||
nilBody = true
|
||||
} else {
|
||||
if r.Header.Get("content-encoding") == "gzip" {
|
||||
if strings.EqualFold(r.Header.Get("content-encoding"), "gzip") {
|
||||
gzipped = true
|
||||
gzr, err := gzip.NewReader(buf)
|
||||
if err != nil {
|
||||
return nil, BadRequestErr("gzip decoder error", err)
|
||||
}
|
||||
defer gzr.Close()
|
||||
body = gzr
|
||||
if maxRequestBodySize != -1 {
|
||||
// Limit decompressed bytes to prevent gzip bombs from bypassing
|
||||
// the raw body size limit applied above.
|
||||
body = http.MaxBytesReader(nil, gzr, maxRequestBodySize)
|
||||
} else {
|
||||
body = gzr
|
||||
}
|
||||
}
|
||||
|
||||
// Insert the JSON key rewriter into the reader pipeline
|
||||
@@ -610,11 +615,13 @@ func MakeDecoder(
|
||||
InternalErr: ace,
|
||||
}
|
||||
}
|
||||
|
||||
if errors.Is(err, io.ErrUnexpectedEOF) && limitedReader != nil && limitedReader.N == 0 && limitExhaustedBody(limitedReader) {
|
||||
return nil, platform_http.PayloadTooLargeError{ContentLength: r.Header.Get("Content-Length"), MaxRequestSize: maxRequestBodySize}
|
||||
if _, ok := errors.AsType[*http.MaxBytesError](err); ok {
|
||||
return nil, platform_http.PayloadTooLargeError{
|
||||
ContentLength: r.Header.Get("Content-Length"),
|
||||
MaxRequestSize: maxRequestBodySize,
|
||||
Gzipped: gzipped,
|
||||
}
|
||||
}
|
||||
|
||||
return nil, BadRequestErr("json decoder error", err)
|
||||
}
|
||||
v = reflect.ValueOf(req)
|
||||
@@ -682,10 +689,14 @@ func MakeDecoder(
|
||||
}
|
||||
}
|
||||
|
||||
if errors.Is(err, io.ErrUnexpectedEOF) {
|
||||
if limitedReader != nil && limitedReader.N == 0 && limitExhaustedBody(limitedReader) {
|
||||
return nil, platform_http.PayloadTooLargeError{ContentLength: r.Header.Get("Content-Length"), MaxRequestSize: maxRequestBodySize}
|
||||
if _, ok := errors.AsType[*http.MaxBytesError](err); ok {
|
||||
return nil, platform_http.PayloadTooLargeError{
|
||||
ContentLength: r.Header.Get("Content-Length"),
|
||||
MaxRequestSize: maxRequestBodySize,
|
||||
Gzipped: gzipped,
|
||||
}
|
||||
}
|
||||
if errors.Is(err, io.ErrUnexpectedEOF) {
|
||||
return nil, BadRequestErr("json decoder error", err)
|
||||
}
|
||||
return nil, err
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
package endpointer
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"compress/gzip"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
@@ -9,6 +11,7 @@ import (
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"reflect"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
@@ -276,9 +279,9 @@ func TestMakeDecoderRequestDecoderFalsePositive(t *testing.T) {
|
||||
|
||||
t.Run("malformed JSON exactly at limit returns decode error, not 413", func(t *testing.T) {
|
||||
// Build a body of exactly `limit` bytes that is malformed JSON (no closing
|
||||
// brace). The LimitedReader is exhausted (N==0), but a peek at the
|
||||
// underlying reader returns EOF — the body ended at the limit, it was not
|
||||
// cut short. Must not produce PayloadTooLargeError.
|
||||
// brace). The MaxBytesReader allows the full read (body fits within limit),
|
||||
// so the JSON decoder sees io.ErrUnexpectedEOF — not *http.MaxBytesError.
|
||||
// Must not produce PayloadTooLargeError.
|
||||
prefix := `{"data":"`
|
||||
body := strings.NewReader(prefix + strings.Repeat("x", limit-len(prefix))) // exactly limit bytes, no closing
|
||||
r := httptest.NewRequest("POST", "/", body)
|
||||
@@ -310,3 +313,161 @@ func TestMakeDecoderRequestDecoderFalsePositive(t *testing.T) {
|
||||
assert.Equal(t, "hello", rd.Data)
|
||||
})
|
||||
}
|
||||
|
||||
func TestMakeDecoderRequestDecoderGzipBomb(t *testing.T) {
|
||||
const limit = 100
|
||||
|
||||
makeDecoder := func() kithttp.DecodeRequestFunc {
|
||||
return MakeDecoder(&testRequestDecoderType{}, defaultJSONUnmarshal, nil, nil, nil, nil, limit)
|
||||
}
|
||||
|
||||
gzipBody := func(data string) *bytes.Buffer {
|
||||
var buf bytes.Buffer
|
||||
gw := gzip.NewWriter(&buf)
|
||||
_, err := gw.Write([]byte(data))
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, gw.Close())
|
||||
return &buf
|
||||
}
|
||||
|
||||
t.Run("gzip bomb exceeding decompressed limit returns 413", func(t *testing.T) {
|
||||
big := `{"data":"` + strings.Repeat("x", limit*10) + `"}`
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(big))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
_, err := makeDecoder()(context.Background(), r)
|
||||
require.Error(t, err)
|
||||
var ple platform_http.PayloadTooLargeError
|
||||
require.True(t, errors.As(err, &ple), "gzip bomb via RequestDecoder must produce PayloadTooLargeError, got: %v", err)
|
||||
assert.True(t, ple.Gzipped, "PayloadTooLargeError from gzip bomb must have Gzipped set")
|
||||
})
|
||||
|
||||
t.Run("valid gzip body within limit is decoded successfully", func(t *testing.T) {
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
result, err := makeDecoder()(context.Background(), r)
|
||||
require.NoError(t, err)
|
||||
rd, ok := result.(*testRequestDecoderType)
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "hi", rd.Data)
|
||||
})
|
||||
|
||||
t.Run("Content-Encoding header is cleared after decompression", func(t *testing.T) {
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
_, err := makeDecoder()(context.Background(), r)
|
||||
require.NoError(t, err)
|
||||
assert.Empty(t, r.Header.Get("Content-Encoding"), "Content-Encoding should be cleared after framework decompression")
|
||||
})
|
||||
}
|
||||
|
||||
type testGzipRequestType struct {
|
||||
Data string `json:"data"`
|
||||
}
|
||||
|
||||
// testRequestDecoderPayloadTooLargeType implements RequestDecoder and returns
|
||||
// a PayloadTooLargeError directly from DecodeRequest (simulating implementations
|
||||
// like getHostSoftwareRequest that enforce their own size limits).
|
||||
type testRequestDecoderPayloadTooLargeType struct {
|
||||
Data string `json:"data"`
|
||||
}
|
||||
|
||||
func (d *testRequestDecoderPayloadTooLargeType) DecodeRequest(_ context.Context, r *http.Request) (any, error) {
|
||||
return nil, platform_http.PayloadTooLargeError{
|
||||
ContentLength: r.Header.Get("Content-Length"),
|
||||
MaxRequestSize: 42,
|
||||
}
|
||||
}
|
||||
|
||||
type testGzipBodyDecoderType struct {
|
||||
Data string `json:"data"`
|
||||
}
|
||||
|
||||
func TestMakeDecoderGzipBomb(t *testing.T) {
|
||||
const limit = 100
|
||||
|
||||
makeDecoder := func() kithttp.DecodeRequestFunc {
|
||||
return MakeDecoder(testGzipRequestType{}, defaultJSONUnmarshal, nil, nil, nil, nil, limit)
|
||||
}
|
||||
|
||||
gzipBody := func(data string) *bytes.Buffer {
|
||||
var buf bytes.Buffer
|
||||
gw := gzip.NewWriter(&buf)
|
||||
_, err := gw.Write([]byte(data))
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, gw.Close())
|
||||
return &buf
|
||||
}
|
||||
|
||||
t.Run("gzip bomb exceeding decompressed limit returns 413", func(t *testing.T) {
|
||||
// Compressed payload is small but decompresses well beyond the limit.
|
||||
big := `{"data":"` + strings.Repeat("x", limit*10) + `"}`
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(big))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
_, err := makeDecoder()(context.Background(), r)
|
||||
require.Error(t, err)
|
||||
var ple platform_http.PayloadTooLargeError
|
||||
require.True(t, errors.As(err, &ple), "gzip bomb must produce PayloadTooLargeError, got: %v", err)
|
||||
assert.True(t, ple.Gzipped, "PayloadTooLargeError from gzip bomb must have Gzipped set")
|
||||
})
|
||||
|
||||
t.Run("valid gzip body within limit is decoded successfully", func(t *testing.T) {
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
result, err := makeDecoder()(context.Background(), r)
|
||||
require.NoError(t, err)
|
||||
rd, ok := result.(*testGzipRequestType)
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "hi", rd.Data)
|
||||
})
|
||||
|
||||
// Sub-tests for the bodyDecoder (DecodeBody) code path, where isBodyDecoder
|
||||
// returns true and decodeBody is called instead of jsonUnmarshal.
|
||||
isBodyDecoder := func(v reflect.Value) bool {
|
||||
_, ok := v.Interface().(*testGzipBodyDecoderType)
|
||||
return ok
|
||||
}
|
||||
decodeBodyFn := func(_ context.Context, _ *http.Request, v reflect.Value, body io.Reader) error {
|
||||
bd := v.Interface().(*testGzipBodyDecoderType)
|
||||
return json.NewDecoder(body).Decode(bd)
|
||||
}
|
||||
makeBodyDecoder := func() kithttp.DecodeRequestFunc {
|
||||
return MakeDecoder(testGzipBodyDecoderType{}, defaultJSONUnmarshal, nil, isBodyDecoder, decodeBodyFn, nil, limit)
|
||||
}
|
||||
|
||||
t.Run("DecodeBody gzip bomb exceeding decompressed limit returns 413", func(t *testing.T) {
|
||||
big := `{"data":"` + strings.Repeat("x", limit*10) + `"}`
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(big))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
_, err := makeBodyDecoder()(context.Background(), r)
|
||||
require.Error(t, err)
|
||||
var ple platform_http.PayloadTooLargeError
|
||||
require.True(t, errors.As(err, &ple), "gzip bomb via DecodeBody must produce PayloadTooLargeError, got: %v", err)
|
||||
assert.True(t, ple.Gzipped, "PayloadTooLargeError from gzip bomb must have Gzipped set")
|
||||
})
|
||||
|
||||
t.Run("DecodeBody valid gzip body within limit is decoded successfully", func(t *testing.T) {
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
result, err := makeBodyDecoder()(context.Background(), r)
|
||||
require.NoError(t, err)
|
||||
rd, ok := result.(*testGzipBodyDecoderType)
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "hi", rd.Data)
|
||||
})
|
||||
|
||||
// Sub-test for the RequestDecoder code path where DecodeRequest itself
|
||||
// returns a PayloadTooLargeError (covers lines 545-546).
|
||||
t.Run("DecodeRequest returning PayloadTooLargeError preserves inner fields and sets Gzipped", func(t *testing.T) {
|
||||
makePayloadDecoder := func() kithttp.DecodeRequestFunc {
|
||||
return MakeDecoder(&testRequestDecoderPayloadTooLargeType{}, defaultJSONUnmarshal, nil, nil, nil, nil, limit)
|
||||
}
|
||||
r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`))
|
||||
r.Header.Set("Content-Encoding", "gzip")
|
||||
_, err := makePayloadDecoder()(context.Background(), r)
|
||||
require.Error(t, err)
|
||||
var ple platform_http.PayloadTooLargeError
|
||||
require.True(t, errors.As(err, &ple), "DecodeRequest returning PayloadTooLargeError must propagate, got: %v", err)
|
||||
assert.True(t, ple.Gzipped, "Gzipped must be set when the request was gzip-encoded")
|
||||
assert.Equal(t, int64(42), ple.MaxRequestSize, "MaxRequestSize from inner error must be preserved")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -101,6 +101,7 @@ func (e *BadRequestError) IsClientError() bool {
|
||||
type PayloadTooLargeError struct {
|
||||
ContentLength string
|
||||
MaxRequestSize int64
|
||||
Gzipped bool
|
||||
}
|
||||
|
||||
func (e PayloadTooLargeError) Error() string {
|
||||
@@ -117,7 +118,11 @@ func (e PayloadTooLargeError) Internal() string {
|
||||
// We don't care if we failed to parse the number, only if we were successful
|
||||
size = units.HumanSize(contentLengthAsNumber)
|
||||
}
|
||||
msg += fmt.Sprintf(", Incoming Content-Length: %s", size)
|
||||
label := "Incoming Content-Length"
|
||||
if e.Gzipped {
|
||||
label = "Incoming Content-Length (compressed)"
|
||||
}
|
||||
msg += fmt.Sprintf(", %s: %s", label, size)
|
||||
}
|
||||
return msg
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user