diff --git a/internal/actions/expirevar_test.go b/internal/actions/expirevar_test.go index ac72fff36..0d156f64c 100644 --- a/internal/actions/expirevar_test.go +++ b/internal/actions/expirevar_test.go @@ -11,14 +11,13 @@ import ( func TestExpirevar(t *testing.T) { t.Parallel() - a, err := Get("expirevar") - if err != nil { - t.Error("failed to get setvar action") + if _, err := Get("expirevar"); err != nil { + t.Error("failed to get expirevar action") } t.Run("no arguments", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "") + err := expirevar().Init(nil, "") if !errors.Is(err, ErrMissingArguments) { t.Errorf("expected error ErrMissingArguments, got %v", err) } @@ -26,7 +25,7 @@ func TestExpirevar(t *testing.T) { t.Run("invalid collection", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "INVALID.test=60") + err := expirevar().Init(nil, "INVALID.test=60") if !strings.Contains(err.Error(), "invalid collection, supported collections are: ") { t.Errorf("expected error 'invalid collection...', got %v", err) } @@ -34,7 +33,7 @@ func TestExpirevar(t *testing.T) { t.Run("missing variable name", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.=60") + err := expirevar().Init(nil, "IP.=60") if !errors.Is(err, ErrInvalidKVArguments) { t.Errorf("expected error ErrInvalidKVArguments, got %v", err) } @@ -42,23 +41,23 @@ func TestExpirevar(t *testing.T) { t.Run("missing ttl", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.test=") + err := expirevar().Init(nil, "IP.test=") if !strings.Contains(err.Error(), "invalid TTL, must be a positive integer") { t.Errorf("expected error 'missing TTL value', got %v", err) } }) - t.Run("missing ttl", func(t *testing.T) { + t.Run("negative ttl", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.test=-1") + err := expirevar().Init(nil, "IP.test=-1") if !strings.Contains(err.Error(), "invalid TTL, must be a positive integer") { t.Errorf("expected error 'missing TTL value', got %v", err) } }) - t.Run("missing ttl", func(t *testing.T) { + t.Run("zero ttl", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.test=0") + err := expirevar().Init(nil, "IP.test=0") if !strings.Contains(err.Error(), "invalid TTL, must be a positive integer") { t.Errorf("expected error 'missing TTL value', got %v", err) } @@ -66,7 +65,7 @@ func TestExpirevar(t *testing.T) { t.Run("valid input", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "TX.testvar=60") + err := expirevar().Init(nil, "TX.testvar=60") if err != nil { t.Errorf("unexpected error for valid input, got %v", err) } diff --git a/internal/actions/initcol_test.go b/internal/actions/initcol_test.go index 01673e4b0..6fa2d72b9 100644 --- a/internal/actions/initcol_test.go +++ b/internal/actions/initcol_test.go @@ -1,6 +1,8 @@ // Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors // SPDX-License-Identifier: Apache-2.0 +//go:build !tinygo + package actions_test import ( diff --git a/internal/corazawaf/body_buffer.go b/internal/corazawaf/body_buffer.go index ba0de41bc..2b92d4d50 100644 --- a/internal/corazawaf/body_buffer.go +++ b/internal/corazawaf/body_buffer.go @@ -96,10 +96,16 @@ func (br *BodyBuffer) Write(data []byte) (n int, err error) { type bodyBufferReader struct { pos int + // err, when set, is returned by every Read; used to surface + // types.ErrBodyTruncated on readers invalidated by Truncate. + err error br *BodyBuffer } func (b *bodyBufferReader) Read(p []byte) (n int, err error) { + if b.err != nil { + return 0, b.err + } if b.br == nil { // reader has been closed and hence we don't attempt to do anymore read return 0, io.EOF @@ -146,6 +152,61 @@ func (br *BodyBuffer) Size() int64 { return br.length } +// Truncate discards all buffered content beyond limit bytes, releasing the +// backing memory. The in-memory buffer is replaced with a fresh one holding +// only the prefix (bytes.Buffer.Truncate retains the full backing array, +// which transaction pooling would keep alive past Close); the file-backed +// buffer is truncated in place, or closed and removed when limit is 0. +// A limit at or above the current size is a no-op. +func (br *BodyBuffer) Truncate(limit int64) error { + if limit >= br.length { + return nil + } + + if environment.HasAccessToFS && br.writer != nil { + if limit == 0 { + w := br.writer + br.writer = nil + br.length = 0 + br.invalidateReaders() + err := w.Close() + // remove even when Close fails: br.writer is already nil, so + // this is the last reference to the file and Reset() would no + // longer clean it up. + if rmErr := os.Remove(w.Name()); err == nil { + err = rmErr + } + return err + } + if err := br.writer.Truncate(limit); err != nil { + // nothing changed: length and readers stay valid + return err + } + br.length = limit + br.invalidateReaders() + return nil + } + fresh := &bytes.Buffer{} + fresh.Write(br.buffer.Bytes()[:limit]) + br.buffer = fresh + br.length = limit + br.invalidateReaders() + return nil +} + +// invalidateReaders marks all outstanding readers with ErrBodyTruncated: +// reading with a position beyond a shrunk buffer would panic, and quietly +// serving the prefix or EOF would hide the contract violation of holding a +// reader across truncation. New readers (e.g. audit log part C) see the +// kept prefix. +func (br *BodyBuffer) invalidateReaders() { + for _, r := range br.readers { + r.br = nil + r.err = types.ErrBodyTruncated + } + br.readers = nil +} + // Reset will reset buffers and delete temporary files func (br *BodyBuffer) Reset() error { br.buffer.Reset() diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 64b85bb94..9cbcecd4e 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -153,6 +153,30 @@ func (tx *Transaction) SetScriptUsername(value string) { tx.variables.scriptUsername.Set(value) } +// TruncateRequestBody releases the buffered request body, keeping at most +// limit bytes as a preview for audit logging. See types.Transaction for the +// full contract. The REQUEST_BODY variable is truncated to the same prefix +// (copied, so the original backing string is released); REQUEST_BODY_LENGTH +// and parsed derivatives such as ARGS_POST are left intact. +func (tx *Transaction) TruncateRequestBody(limit int64) error { + if limit < 0 { + return errors.New("limit must be non-negative") + } + // lastPhase only advances when rules are evaluated, so it stays behind + // when the engine is off or the transaction was interrupted — states in + // which request-body analysis will never run and truncation is safe. + if tx.lastPhase < types.PhaseRequestBody && !tx.IsRuleEngineOff() && !tx.IsInterrupted() { + return errors.New("TruncateRequestBody must be called after ProcessRequestBody") + } + if err := tx.requestBodyBuffer.Truncate(limit); err != nil { + return err + } + if rb := tx.variables.requestBody.Get(); int64(len(rb)) > limit { + tx.variables.requestBody.Set(strings.Clone(rb[:limit])) + } + return nil +} + func (tx *Transaction) ID() string { return tx.id } diff --git a/internal/corazawaf/transaction_truncate_test.go b/internal/corazawaf/transaction_truncate_test.go new file mode 100644 index 000000000..6614106a5 --- /dev/null +++ b/internal/corazawaf/transaction_truncate_test.go @@ -0,0 +1,308 @@ +// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + +package corazawaf + +import ( + "errors" + "io" + "os" + "strconv" + "strings" + "testing" + + "github.com/corazawaf/coraza/v3/internal/environment" + "github.com/corazawaf/coraza/v3/types" +) + +// makeTruncateTx builds a transaction with an urlencoded body already +// analyzed by ProcessRequestBody. memLimit <= 0 keeps the WAF default +// (in-memory buffering). +func makeTruncateTx(t *testing.T, memLimit int64, body string) *Transaction { + t.Helper() + waf := NewWAF() + if memLimit > 0 { + waf.SetRequestBodyInMemoryLimit(memLimit) + } + tx := waf.NewTransaction() + tx.RequestBodyAccess = true + tx.AuditLogParts = types.AuditLogParts("ABCFHZ") + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.AddRequestHeader("Content-Type", "application/x-www-form-urlencoded") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte(body)); err != nil { + t.Fatal(err) + } + if _, err := tx.ProcessRequestBody(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { tx.Close() }) + return tx +} + +func TestTruncateRequestBodyInMemory(t *testing.T) { + body := "key=" + strings.Repeat("v", 64*1024) + tx := makeTruncateTx(t, 0, body) + + if got := tx.requestBodyBuffer.buffer.Cap(); got < len(body) { + t.Fatalf("test setup: expected in-memory buffer capacity >= %d, got %d", len(body), got) + } + + const limit = 16 + if err := tx.TruncateRequestBody(limit); err != nil { + t.Fatal(err) + } + + if got := tx.requestBodyBuffer.Size(); got != limit { + t.Errorf("buffer size: want %d, got %d", limit, got) + } + if got := tx.requestBodyBuffer.buffer.Cap(); got > 1024 { + t.Errorf("backing array not released: capacity still %d", got) + } + if got := tx.variables.requestBody.Get(); got != body[:limit] { + t.Errorf("REQUEST_BODY: want %q, got %q", body[:limit], got) + } + if got := tx.variables.requestBodyLength.Get(); got != strconv.Itoa(len(body)) { + t.Errorf("REQUEST_BODY_LENGTH: want %d, got %s", len(body), got) + } + // parsed derivatives stay intact + if got := tx.variables.argsPost.Get("key"); len(got) != 1 || got[0] != body[4:] { + t.Error("ARGS_POST was affected by truncation") + } + // audit part C carries exactly the preview + if got := tx.AuditLog().Transaction_.Request_.Body_; got != body[:limit] { + t.Errorf("audit part C: want %q, got %q", body[:limit], got) + } + + // idempotency: repeated and larger-limit calls are no-ops + for _, l := range []int64{limit, limit + 100} { + if err := tx.TruncateRequestBody(l); err != nil { + t.Fatal(err) + } + if got := tx.variables.requestBody.Get(); got != body[:limit] { + t.Errorf("REQUEST_BODY changed after TruncateRequestBody(%d): %q", l, got) + } + } +} + +func TestTruncateRequestBodyInMemoryDiscard(t *testing.T) { + body := "key=" + strings.Repeat("v", 64*1024) + tx := makeTruncateTx(t, 0, body) + + if err := tx.TruncateRequestBody(0); err != nil { + t.Fatal(err) + } + if got := tx.requestBodyBuffer.Size(); got != 0 { + t.Errorf("buffer size: want 0, got %d", got) + } + if got := tx.requestBodyBuffer.buffer.Cap(); got != 0 { + t.Errorf("backing array not released: capacity still %d", got) + } + if got := tx.variables.requestBody.Get(); got != "" { + t.Errorf("REQUEST_BODY: want empty, got %q", got) + } + if got := tx.AuditLog().Transaction_.Request_.Body_; got != "" { + t.Errorf("audit part C: want empty, got %q", got) + } +} + +func TestTruncateRequestBodyFile(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + body := "key=" + strings.Repeat("v", 1024) + tx := makeTruncateTx(t, 8, body) + + f := tx.requestBodyBuffer.writer + if f == nil { + t.Fatal("test setup: expected file-backed body buffer") + } + + const limit = 16 + if err := tx.TruncateRequestBody(limit); err != nil { + t.Fatal(err) + } + if st, err := os.Stat(f.Name()); err != nil { + t.Fatal(err) + } else if st.Size() != limit { + t.Errorf("tmp file size: want %d, got %d", limit, st.Size()) + } + if got := tx.requestBodyBuffer.Size(); got != limit { + t.Errorf("buffer size: want %d, got %d", limit, got) + } + if got := tx.AuditLog().Transaction_.Request_.Body_; got != body[:limit] { + t.Errorf("audit part C: want %q, got %q", body[:limit], got) + } +} + +func TestTruncateRequestBodyFileDiscard(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + body := "key=" + strings.Repeat("v", 1024) + tx := makeTruncateTx(t, 8, body) + + f := tx.requestBodyBuffer.writer + if f == nil { + t.Fatal("test setup: expected file-backed body buffer") + } + if err := tx.TruncateRequestBody(0); err != nil { + t.Fatal(err) + } + if tx.requestBodyBuffer.writer != nil { + t.Error("tmp file writer not released") + } + if _, err := os.Stat(f.Name()); err == nil { + t.Error("tmp file was not deleted") + } + if got := tx.requestBodyBuffer.Size(); got != 0 { + t.Errorf("buffer size: want 0, got %d", got) + } +} + +func TestTruncateRequestBodyBeforeProcessing(t *testing.T) { + tx := NewWAF().NewTransaction() + defer tx.Close() + tx.RequestBodyAccess = true + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte("key=value")); err != nil { + t.Fatal(err) + } + + if err := tx.TruncateRequestBody(0); err == nil { + t.Error("expected error when called before ProcessRequestBody") + } + if got := tx.requestBodyBuffer.Size(); got != int64(len("key=value")) { + t.Errorf("body must stay untouched on error, size: %d", got) + } +} + +// Readers obtained before truncation are invalidated: whether consumed or +// untouched, they must fail with ErrBodyTruncated on the next read instead +// of panicking or quietly serving the prefix/EOF. +func TestTruncateRequestBodyStaleReader(t *testing.T) { + tx := makeTruncateTx(t, 0, "key="+strings.Repeat("v", 1024)) + + consumed, err := tx.RequestBodyReader() + if err != nil { + t.Fatal(err) + } + if _, err := io.Copy(io.Discard, consumed); err != nil { + t.Fatal(err) + } + untouched, err := tx.RequestBodyReader() + if err != nil { + t.Fatal(err) + } + if err := tx.TruncateRequestBody(4); err != nil { + t.Fatal(err) + } + for name, r := range map[string]io.Reader{"consumed": consumed, "untouched": untouched} { + n, err := r.Read(make([]byte, 8)) + if n != 0 || !errors.Is(err, types.ErrBodyTruncated) { + t.Errorf("%s stale reader: want (0, ErrBodyTruncated), got (%d, %v)", name, n, err) + } + } +} + +func TestTruncateRequestBodyNegativeLimit(t *testing.T) { + tx := makeTruncateTx(t, 0, "key=value") + if err := tx.TruncateRequestBody(-1); err == nil { + t.Error("expected error for negative limit") + } +} + +// With the rule engine off, ProcessRequestBody skips rule evaluation and +// lastPhase never advances; truncation must still be accepted. +func TestTruncateRequestBodyEngineOff(t *testing.T) { + waf := NewWAF() + waf.RuleEngine = types.RuleEngineOff + tx := waf.NewTransaction() + defer tx.Close() + tx.RequestBodyAccess = true + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte("key=value")); err != nil { + t.Fatal(err) + } + if _, err := tx.ProcessRequestBody(); err != nil { + t.Fatal(err) + } + if err := tx.TruncateRequestBody(0); err != nil { + t.Errorf("engine off: unexpected error: %v", err) + } +} + +// On an interrupted transaction ProcessRequestBody returns early without +// evaluating rules; truncation must still be accepted. +func TestTruncateRequestBodyInterrupted(t *testing.T) { + tx := NewWAF().NewTransaction() + defer tx.Close() + tx.RequestBodyAccess = true + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte("key=value")); err != nil { + t.Fatal(err) + } + tx.interruption = &types.Interruption{Action: "deny"} + if _, err := tx.ProcessRequestBody(); err != nil { + t.Fatal(err) + } + if err := tx.TruncateRequestBody(0); err != nil { + t.Errorf("interrupted: unexpected error: %v", err) + } + if got := tx.requestBodyBuffer.Size(); got != 0 { + t.Errorf("buffer size: want 0, got %d", got) + } +} + +// Even when closing the temp file fails, the file must still be removed: +// br.writer is already nil at that point, so Reset() would never clean it up. +func TestTruncateRequestBodyCloseFailureStillRemovesFile(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + tx := makeTruncateTx(t, 8, "key="+strings.Repeat("v", 1024)) + + f := tx.requestBodyBuffer.writer + if err := f.Close(); err != nil { // makes the buffer's own Close fail + t.Fatal(err) + } + if err := tx.TruncateRequestBody(0); err == nil { + t.Error("expected error from failing Close") + } + if tx.requestBodyBuffer.writer != nil { + t.Error("tmp file writer not released") + } + if _, err := os.Stat(f.Name()); err == nil { + t.Error("tmp file was not deleted") + } +} + +// A failing file truncation must leave the buffer untouched: size keeps the +// original value and outstanding readers are not invalidated. +func TestTruncateRequestBodyFileTruncateFailureKeepsState(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + body := "key=" + strings.Repeat("v", 1024) + tx := makeTruncateTx(t, 8, body) + + r, err := tx.RequestBodyReader() + if err != nil { + t.Fatal(err) + } + if err := tx.requestBodyBuffer.writer.Close(); err != nil { // makes Truncate fail + t.Fatal(err) + } + if err := tx.TruncateRequestBody(16); err == nil { + t.Error("expected error from failing file truncation") + } + if got := tx.requestBodyBuffer.Size(); got != int64(len(body)) { + t.Errorf("buffer size: want %d, got %d", len(body), got) + } + if _, err := r.Read(make([]byte, 1)); errors.Is(err, types.ErrBodyTruncated) { + t.Error("reader invalidated although nothing was truncated") + } +} diff --git a/types/transaction.go b/types/transaction.go index 9edc604eb..6773fc45e 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -4,11 +4,19 @@ package types import ( + "errors" "io" "github.com/corazawaf/coraza/v3/debuglog" ) +// ErrBodyTruncated is returned by request body readers obtained before a +// TruncateRequestBody call: truncation invalidates them, and reading +// truncated data as if it were the full body would be silent data loss. +// Holding a reader across truncation means the caller's flow violates the +// TruncateRequestBody contract. +var ErrBodyTruncated = errors.New("request body released by TruncateRequestBody") + // Transaction is created from a WAF instance to handle web requests and responses, // it contains a copy of most WAF configurations that can be safely changed. // Transactions are used to store all data like URLs, request and response @@ -199,6 +207,24 @@ type Transaction interface { SetScriptFilename(string) SetScriptUsername(string) + // TruncateRequestBody releases the buffered request body once + // request-phase analysis is done, keeping at most limit bytes as a + // preview for audit logging. limit=0 discards the body entirely. + // Intended for connectors that keep a transaction open between the + // request and logging phases (waiting for the upstream response): those + // phases never re-read the request body, so holding it only inflates + // memory for the lifetime of the parked transaction. Call after + // ProcessRequestBody(); calling earlier returns an error and leaves the + // body untouched, unless the rule engine is off or the transaction is + // already interrupted — states in which request-body analysis will + // never run and truncation is accepted at any point. Afterwards audit log part C emits at most limit bytes + // (a raw prefix — it may cut mid-token) and the REQUEST_BODY variable + // holds the same prefix, while REQUEST_BODY_LENGTH keeps the original + // received length and parsed derivatives (ARGS_POST, JSON/XML matches) + // remain intact. Readers previously obtained via RequestBodyReader are + // invalidated and fail with ErrBodyTruncated. + TruncateRequestBody(limit int64) error + // Closer closes the transaction and releases any resources associated with it such as request/response bodies. io.Closer }