diff --git a/pkg/appsec/chunked_repro_test.go b/pkg/appsec/chunked_repro_test.go new file mode 100644 index 00000000000..4e3b26411ef --- /dev/null +++ b/pkg/appsec/chunked_repro_test.go @@ -0,0 +1,295 @@ +package appsec + +import ( + "bufio" + "encoding/hex" + "fmt" + "io" + "net" + "net/http" + "strconv" + "strings" + "sync" + "testing" + "time" + + log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestChunkedBodyForwardingMismatch verifies how readRequestBody behaves when a +// request claims Transfer-Encoding: chunked but the on-wire body is empty, +// unframed, or arrives on a connection that is never closed by the client. +// +// This reproduces the regression introduced by PR #4355 (commit 3d5c4d9b1, +// "WAF: enforce body size limitation"), which switched body reading from a +// no-op for chunked requests (io.ReadFull on a 0-length buffer when +// ContentLength==-1) to io.ReadAll, which actually drives Go's internal +// chunkedReader. +// +// The test uses raw TCP — http.Client and httptest.NewRequest both re-encode +// chunked correctly, which would not reproduce the wire-format mismatch +// observed when the nginx Lua bouncer forwards the chunked header without +// putting matching chunk frames on the wire to appsec. +func TestChunkedBodyForwardingMismatch(t *testing.T) { + // Short read deadline so the test stays fast. Real appsec uses 1s + // (DefaultBodyReadTimeout in pkg/acquisition/modules/appsec/config.go). + const readDeadline = 100 * time.Millisecond + const fastBudget = 50 * time.Millisecond + + logger := log.NewEntry(log.New()) + + // The handler returns the body bytes as a hex string in X-Body-Hex so the + // test can assert on the exact content readRequestBody saw. This is the + // data appsec would expose to body-related rules. + handler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + // Mirror the real appsec handler at config.go:354. + if err := http.NewResponseController(rw).SetReadDeadline(time.Now().Add(readDeadline)); err != nil { + t.Logf("set deadline err: %v", err) + } + + start := time.Now() + body, truncated, exceeded, err := readRequestBody(r, BodySettings{}, logger) + elapsed := time.Since(start) + + rw.Header().Set("X-Body-Len", strconv.Itoa(len(body))) + rw.Header().Set("X-Body-Hex", hex.EncodeToString(body)) + rw.Header().Set("X-Truncated", strconv.FormatBool(truncated)) + rw.Header().Set("X-Exceeded", strconv.FormatBool(exceeded)) + rw.Header().Set("X-Elapsed-Ms", strconv.FormatInt(elapsed.Milliseconds(), 10)) + if err != nil { + rw.Header().Set("X-Body-Err", err.Error()) + rw.WriteHeader(http.StatusInternalServerError) + return + } + rw.WriteHeader(http.StatusOK) + }) + + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + server := &http.Server{ + Handler: handler, + ReadHeaderTimeout: time.Second, + } + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + _ = server.Serve(listener) + }() + t.Cleanup(func() { + _ = server.Close() + wg.Wait() + }) + + addr := listener.Addr().String() + + type expectation struct { + // minStatus..maxStatus describes the acceptable status code range. + // Exact-match cases set both to the same value. + minStatus, maxStatus int + // allowDeadline=true means the response is expected to take ~readDeadline. + // allowDeadline=false means it should come back well before the deadline. + allowDeadline bool + // expectBody, if non-nil, is the exact decoded body readRequestBody + // must produce. Set on positive scenarios (well-framed chunked bodies) + // to confirm body-related rules would see the right bytes. + expectBody *string + } + + bodyOf := func(s string) *string { return &s } + + // helper: render N bytes as hex chunks separated by chunked framing. + chunked := func(parts ...string) string { + var sb strings.Builder + for _, p := range parts { + fmt.Fprintf(&sb, "%x\r\n%s\r\n", len(p), p) + } + sb.WriteString("0\r\n\r\n") + return sb.String() + } + + cases := []struct { + name string + writeBody string + halfClose bool // close our write half after sending + expect expectation + }{ + { + // Control: properly framed empty chunked body — the happy path. + name: "A_well_formed_chunked_empty", + writeBody: "0\r\n\r\n", + halfClose: true, + expect: expectation{minStatus: 200, maxStatus: 200, allowDeadline: false}, + }, + { + // chunked header, zero bytes, then close. chunkedReader gets EOF + // reading the chunk-size line; readRequestBody swallows + // io.ErrUnexpectedEOF (request.go:333-335). + name: "B_chunked_header_no_body_close", + writeBody: "", + halfClose: true, + expect: expectation{minStatus: 200, maxStatus: 200, allowDeadline: false}, + }, + { + // chunked header, zero bytes, connection STAYS OPEN. chunkedReader + // blocks; the read deadline fires; readRequestBody swallows the + // net.Error timeout. THIS IS THE FAILURE MODE that matches the + // bouncer logs from CVE-2023-34362 hubtest. + name: "C_chunked_header_no_body_keepopen", + writeBody: "", + halfClose: false, + expect: expectation{minStatus: 200, maxStatus: 200, allowDeadline: true}, + }, + { + // Plain unframed bytes with a chunked header. Depending on whether + // the parser hits a CRLF before EOF, this either errors as + // "malformed chunked encoding" (-> 500) or as ErrUnexpectedEOF + // (swallowed -> 200). Either way it should return promptly. + name: "D_chunked_header_plain_body", + writeBody: "body content", + halfClose: true, + expect: expectation{minStatus: 200, maxStatus: 599, allowDeadline: false}, + }, + { + // Same wire shape as the failing hubtest (terminator only) but + // without closing — confirms a properly framed empty chunked body + // returns fast even on a kept-open connection. + name: "E_well_formed_chunked_empty_keepopen", + writeBody: "0\r\n\r\n", + halfClose: false, + expect: expectation{minStatus: 200, maxStatus: 200, allowDeadline: false}, + }, + { + // Garbage chunk-size header followed by CRLF — should reliably + // trigger "malformed chunked encoding" and 500. + name: "F_chunked_header_invalid_size", + writeBody: "ZZ\r\n", + halfClose: true, + expect: expectation{minStatus: 500, maxStatus: 500, allowDeadline: false}, + }, + { + // Positive: a single-chunk body with real content. readRequestBody + // must hand the decoded bytes ("hello world") back so rules can + // inspect them. + name: "G_single_chunk_with_content", + writeBody: chunked("hello world"), + halfClose: true, + expect: expectation{ + minStatus: 200, maxStatus: 200, allowDeadline: false, + expectBody: bodyOf("hello world"), + }, + }, + { + // Positive, kept-open: same as G but without half-close. The + // terminator must be enough for chunkedReader to stop without + // hitting the read deadline. + name: "H_single_chunk_keepopen", + writeBody: chunked("hello world"), + halfClose: false, + expect: expectation{ + minStatus: 200, maxStatus: 200, allowDeadline: false, + expectBody: bodyOf("hello world"), + }, + }, + { + // Positive: multiple chunks must be reassembled in order. + name: "I_multi_chunk", + writeBody: chunked("foo=", "bar&", "baz=qux"), + halfClose: true, + expect: expectation{ + minStatus: 200, maxStatus: 200, allowDeadline: false, + expectBody: bodyOf("foo=bar&baz=qux"), + }, + }, + { + // Positive: a SQLi-shaped payload sent in two chunks — the kind of + // thing a body-rule would match. Confirms the bytes appsec sees + // after dechunking are byte-identical to the logical body. + name: "J_multi_chunk_sqli_payload", + writeBody: chunked( + "username=admin&password=' OR ", + "1=1 -- ", + ), + halfClose: true, + expect: expectation{ + minStatus: 200, maxStatus: 200, allowDeadline: false, + expectBody: bodyOf("username=admin&password=' OR 1=1 -- "), + }, + }, + { + // Positive: a moderately large body (8 KiB), single chunk. Stays + // well under DefaultMaxBodySize (10 MiB) so no truncation. + name: "K_large_single_chunk", + writeBody: chunked(strings.Repeat("A", 8*1024)), + halfClose: true, + expect: expectation{ + minStatus: 200, maxStatus: 200, allowDeadline: false, + expectBody: bodyOf(strings.Repeat("A", 8*1024)), + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + conn, err := net.Dial("tcp", addr) + require.NoError(t, err) + defer conn.Close() + + req := "POST / HTTP/1.1\r\n" + + "Host: x\r\n" + + "Transfer-Encoding: chunked\r\n" + + "\r\n" + + tc.writeBody + + start := time.Now() + _, err = conn.Write([]byte(req)) + require.NoError(t, err) + + if tc.halfClose { + if tcpConn, ok := conn.(*net.TCPConn); ok { + _ = tcpConn.CloseWrite() + } + } + + // Bound how long we wait for a response so a hung server doesn't hang the test. + _ = conn.SetReadDeadline(time.Now().Add(readDeadline + 2*time.Second)) + + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + require.NoError(t, err, "should get an HTTP response within the timeout") + elapsed := time.Since(start) + defer resp.Body.Close() + _, _ = io.Copy(io.Discard, resp.Body) + + t.Logf("scenario %s: status=%d elapsed=%s X-Body-Len=%s X-Body-Err=%q X-Elapsed-Ms=%s", + tc.name, + resp.StatusCode, elapsed, + resp.Header.Get("X-Body-Len"), + resp.Header.Get("X-Body-Err"), + resp.Header.Get("X-Elapsed-Ms"), + ) + + assert.GreaterOrEqual(t, resp.StatusCode, tc.expect.minStatus) + assert.LessOrEqual(t, resp.StatusCode, tc.expect.maxStatus) + + if tc.expect.allowDeadline { + assert.GreaterOrEqual(t, elapsed, readDeadline-10*time.Millisecond, + "expected handler to wait at least one read deadline (~%s)", readDeadline) + } else { + assert.Less(t, elapsed, readDeadline+fastBudget, + "expected handler to return faster than the read deadline; got %s", elapsed) + } + + if tc.expect.expectBody != nil { + gotHex := resp.Header.Get("X-Body-Hex") + gotBody, err := hex.DecodeString(gotHex) + require.NoError(t, err, "X-Body-Hex must decode") + assert.Equal(t, *tc.expect.expectBody, string(gotBody), + "readRequestBody must return the dechunked body so body rules can inspect it") + } + }) + } +}