From 804f74c782b45c09a8d4948d7b2357e54ba24fa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Sun, 25 Jun 2023 20:16:48 +0200 Subject: [PATCH] fix: blocks body buffer reader once the body buffer has been reset. Currently, the body buffer would hold a piece of the body, hence a reader will be sent to the connector in order to pass it to upstream. The problem happens when the transaction is closed, the body buffer is resetted but the reader is still out there and the connector tries to drain it (using something like io.Copy(io.Discard, body) in order to reuse the connection, the body buffer is already empty (lenght 0) but the body buffer reader points to the end of the buffer (e.g. 512), hence attempting to read it till the end (from 512 to 0) trigger an out of range error. Closes corazawaf/coraza-caddy#48. --- go.work.sum | 3 +- internal/corazawaf/body_buffer.go | 41 +++++++++++++++++++++----- internal/corazawaf/body_buffer_test.go | 33 +++++++++++++++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/go.work.sum b/go.work.sum index 96e0c04e2..9cd2990de 100644 --- a/go.work.sum +++ b/go.work.sum @@ -75,6 +75,7 @@ github.com/hashicorp/serf v0.9.6 h1:uuEX1kLR6aoda1TBttmJQKDLZE1Ob7KN0NPdE7EtCDc= github.com/hashicorp/vault/api v1.0.4 h1:j08Or/wryXT4AcHj1oCbMd7IijXcKzYUGw59LGu9onU= github.com/hashicorp/vault/sdk v0.1.13 h1:mOEPeOhT7jl0J4AMl1E705+BcmeRs1VmKNb9F0sMLy8= github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d h1:kJCB4vdITiW1eC1vq2e6IsrXKrZit1bv/TDYFGMp4BQ= +github.com/inconshreveable/mousetrap v1.0.1/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg= github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8= github.com/jpillora/backoff v1.0.0 h1:uvFg412JmmHBHw7iwprIxkPMI+sGQ4kzOWsMeHnm2EA= @@ -125,7 +126,6 @@ go.uber.org/multierr v1.6.0 h1:y6IPFStTAIT5Ytl7/XYmHvzXQ7S3g/IeZW9hyZ5thw4= go.uber.org/zap v1.17.0 h1:MTjgFu6ZLKvY6Pvaqk97GlxNBuMpV4Hy/3P6tRGlI2U= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 h1:7I4JAnoQBe7ZtJcBaYHi5UtiO8tQHbUSXxL+pnGRANg= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= -golang.org/x/crypto v0.10.0 h1:LKqV2xt9+kDzSTfOhx4FrkEBcMrAgHSYgzywV9zcGmM= golang.org/x/crypto v0.10.0/go.mod h1:o4eNf7Ede1fv+hwOwZsTHl9EsPFO6q6ZvYR8vYfY45I= golang.org/x/exp v0.0.0-20190121172915-509febef88a4 h1:c2HOrn5iMezYjSlGPncknSEr/8x5LELb/ilJbXi9DEA= golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 h1:VLliZ0d+/avPrXXH+OakdXhpJuEoBZuwh1m2j7U6Iug= @@ -158,6 +158,7 @@ golang.org/x/text v0.7.0 h1:4BRB4x83lYWy72KwLD/qYDuTu7q9PjSagHvijDw7cLo= golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.8.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= +golang.org/x/text v0.10.0 h1:UpjohKhiEgNc0CSauXmwYftY1+LlaC75SJwh0SgCX58= golang.org/x/text v0.10.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4 h1:SvFZT6jyqRaOeXpc5h/JSfZenJ2O330aBsf7JfSUXmQ= google.golang.org/appengine v1.4.0 h1:/wp5JvzpHIxhs/dumFmF7BXTf3Z+dd4uXta4kVyO508= diff --git a/internal/corazawaf/body_buffer.go b/internal/corazawaf/body_buffer.go index 8d5e5ce0d..abd280ddb 100644 --- a/internal/corazawaf/body_buffer.go +++ b/internal/corazawaf/body_buffer.go @@ -21,6 +21,7 @@ type BodyBuffer struct { buffer *bytes.Buffer writer *os.File length int64 + readers []*bodyBufferReader } var ( @@ -57,7 +58,7 @@ func (br *BodyBuffer) Write(data []byte) (n int, err error) { // Write has been called without checking the Limit, it should never happend. Raising an error. // The buffers are private and populated only by WriteRequestBody, ReadRequestBodyFrom, and similar functions // that have to perform limit checks before calling Write() - return 0, errors.New("Limit reached while writing") + return 0, errors.New("limit reached while writing") } targetLen := br.length + int64(len(data)) @@ -99,18 +100,25 @@ type bodyBufferReader struct { } func (b *bodyBufferReader) Read(p []byte) (n int, err error) { + if b.br == nil { + // reader has been closed and hence we don't attempt to do anymore read + return 0, io.EOF + } + if !environment.HasAccessToFS || b.br.writer == nil { buf := b.br.buffer.Bytes() + n = len(p) - if b.pos+n > len(buf) { - n = len(buf) - b.pos + if bl := b.br.buffer.Len(); b.pos+n > bl { + n = bl - b.pos } if n == 0 { return 0, io.EOF } - copy(p, buf[b.pos:b.pos+n]) - b.pos += n - return + + k := copy(p, buf[b.pos:b.pos+n]) + b.pos += k + return k, nil } n, err = b.br.writer.ReadAt(p, int64(b.pos)) @@ -118,11 +126,19 @@ func (b *bodyBufferReader) Read(p []byte) (n int, err error) { return } +// Close closes the reader +func (b *bodyBufferReader) Close() { + b.br = nil + b.pos = 0 +} + // Reader Returns a working reader for the body buffer in memory or file func (br *BodyBuffer) Reader() (io.Reader, error) { - return &bodyBufferReader{ + r := &bodyBufferReader{ br: br, - }, nil + } + br.readers = append(br.readers, r) + return r, nil } // Size returns the current size of the body buffer @@ -134,6 +150,15 @@ func (br *BodyBuffer) Size() int64 { func (br *BodyBuffer) Reset() error { br.buffer.Reset() br.length = 0 + + // close all readers, this is important because connectors may have + // a reference to a reader but the transaction is already closed and + // hence the reader is not valid anymore. + for _, r := range br.readers { + r.Close() + } + br.readers = nil + if environment.HasAccessToFS && br.writer != nil { w := br.writer br.writer = nil diff --git a/internal/corazawaf/body_buffer_test.go b/internal/corazawaf/body_buffer_test.go index 0ed8631d1..eb5f2e493 100644 --- a/internal/corazawaf/body_buffer_test.go +++ b/internal/corazawaf/body_buffer_test.go @@ -141,3 +141,36 @@ func TestWriteLimit(t *testing.T) { }) } } + +// See https://github.com/corazawaf/coraza-caddy/issues/48 +func TestBodyBufferResetAndReadTheReader(t *testing.T) { + br := NewBodyBuffer(types.BodyBufferOptions{ + MemoryLimit: 5, + Limit: 5, + }) + br.Write([]byte("test1")) + + r, _ := br.Reader() + + dest := make([]byte, 5) + nRead, err := r.Read(dest) + if err != nil { + t.Fatalf("unexpected error when creating reader %s", err.Error()) + } + if nRead != 5 { + t.Fatalf("unexpected number of bytes read, want: %d, have: %d", 5, nRead) + } + + err = br.Reset() + if err != nil { + t.Fatalf("unexpected error %s", err.Error()) + } + + nCopied, err := io.Copy(io.Discard, r) + if err != nil { + t.Fatalf("unexpected error %s", err.Error()) + } + if nCopied != 0 { + t.Fatalf("unexpected number of bytes read, want: %d, have: %d", 5, nCopied) + } +}