Merge branch 'makes_benchmarks_fail' of github.com:corazawaf/coraza i…

…nto makes_benchmarks_fail

.github/workflows/lint.yml

@@ -1,8 +1,19 @@
 name: Lint (pre-commit)
 
 on:
-  pull_request:
   push:
+    branches:
+      - main
+    paths-ignore:
+      - "**/*.md"
+      - "LICENSE"
+  pull_request:
+    branches:
+      - main
+    paths-ignore:
+      - "**/*.md"
+      - "LICENSE"
+
 jobs:
   lint:
     runs-on: ubuntu-latest

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=

internal/bodyprocessors/multipart.go

@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"log"
 	"mime"
 	"mime/multipart"
 	"os"
@@ -25,7 +24,7 @@ func (mbp *multipartBodyProcessor) ProcessRequest(reader io.Reader, v plugintype
 	storagePath := options.StoragePath
 	mediaType, params, err := mime.ParseMediaType(mimeType)
 	if err != nil {
-		log.Fatalf("failed to parse media type: %s", err.Error())
+		return err
 	}
 	if !strings.HasPrefix(mediaType, "multipart/") {
 		return errors.New("not a multipart body")

internal/bodyprocessors/multipart_test.go

@@ -77,3 +77,20 @@ Content-Type: text/html
 		}
 	}
 }
+
+func TestInvalidMultipartCT(t *testing.T) {
+	payload := strings.TrimSpace(`
+-----------------------------9051914041544843365972754266
+Content-Disposition: form-data; name="text"
+
+text default
+-----------------------------9051914041544843365972754266
+`)
+	mp := multipartProcessor(t)
+	v := corazawaf.NewTransactionVariables()
+	if err := mp.ProcessRequest(strings.NewReader(payload), v, plugintypes.BodyProcessorOptions{
+		Mime: "multipart/form-data; boundary=---------------------------9051914041544843365972754266; a=1; a=2",
+	}); err == nil {
+		t.Error("multipart processor should fail for invalid content-type")
+	}
+}

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,30 +100,45 @@ 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 n == 0 {
 			return 0, io.EOF
 		}
-		copy(p, buf[b.pos:b.pos+n])
-		b.pos += n
-		return
+
+		an := copy(p, buf[b.pos:b.pos+n])
+		b.pos += an
+		return an, nil
 	}
 
 	n, err = b.br.writer.ReadAt(p, int64(b.pos))
 	b.pos += n
 	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

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")) // nolint
+
+	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)
+	}
+}