go package github.com/corazawaf/coraza is vulnerable to denial of service

@@ -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

@@ -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=

@@ -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

@@ -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)
+	}
+}

@@ -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")

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

@@ -17,7 +17,11 @@ package bodyprocessors
 import (
 	"fmt"
 	"io"
-	"net/http"
+	"log"
+	"mime"
+	"mime/multipart"
+	"os"
+	"strings"
 
 	"github.com/jptosso/coraza-waf/v2/types/variables"
 )
@@ -26,53 +30,84 @@ type multipartBodyProcessor struct {
 	collections *collectionsMap
 }
 
-func (mbp *multipartBodyProcessor) Read(reader io.Reader, mime string, storagePath string) error {
-	req, _ := http.NewRequest("GET", "/", reader)
-	req.Header.Set("Content-Type", mime)
-	err := req.ParseMultipartForm(1000000000)
+func (mbp *multipartBodyProcessor) Read(reader io.Reader, mimeType string, storagePath string) error {
+	mediaType, params, err := mime.ParseMediaType(mimeType)
 	if err != nil {
-		return err
+		log.Fatal(err)
 	}
-	totalSize := int64(0)
-	fn := map[string][]string{
-		"": {},
-	}
-	fl := map[string][]string{
-		"": {},
+	if !strings.HasPrefix(mediaType, "multipart/") {
+		return fmt.Errorf("not a multipart body")
 	}
-	fs := map[string][]string{
-		"": {},
-	}
-	for field, fheaders := range req.MultipartForm.File {
-		// TODO add them to temporal storage
-		// or maybe not, according to http.MultipartForm, it does exactly that
-		// the main issue is how do I get this path?
-		fn[""] = append(fn[""], field)
-		for _, header := range fheaders {
-			fl[""] = append(fl[""], header.Filename)
-			totalSize += header.Size
-			fs[""] = append(fs[""], fmt.Sprintf("%d", header.Size))
+	mr := multipart.NewReader(reader, params["boundary"])
+	totalSize := int64(0)
+	filesNames := []string{}
+	filesArgNames := []string{}
+	fileList := []string{}
+	fileSizes := []string{}
+	postNames := []string{}
+	postFields := map[string][]string{}
+	for {
+		p, err := mr.NextPart()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			return err
+		}
+		// we create a temp file
+
+		// if is a file
+		if p.FileName() != "" {
+			temp, err := os.CreateTemp(storagePath, "crzmp*")
+			if err != nil {
+				return err
+			}
+			sz, err := io.Copy(temp, p)
+			if err != nil {
+				return err
+			}
+			totalSize += sz
+			filesNames = append(filesNames, p.FileName())
+			fileList = append(fileList, temp.Name())
+			fileSizes = append(fileSizes, fmt.Sprintf("%d", sz))
+			filesArgNames = append(filesArgNames, p.FormName())
+		} else {
+			fmt.Println("VARIABLE", p.FormName())
+			// if is a field
+			data, err := io.ReadAll(p)
+			if err != nil {
+				return err
+			}
+			totalSize += int64(len(data))
+			postNames = append(postNames, p.FormName())
+			if _, ok := postFields[p.FormName()]; !ok {
+				postFields[p.FormName()] = []string{}
+			}
+			postFields[p.FormName()] = append(postFields[p.FormName()], string(data))
+
 		}
-	}
-	m := map[string][]string{}
-	names := []string{}
-	for k, vs := range req.MultipartForm.Value {
-		m[k] = vs
-		names = append(names, k)
-	}
-	fcs := map[string][]string{
-		"": {fmt.Sprintf("%d", totalSize)},
 	}
 	mbp.collections = &collectionsMap{
-		variables.FilesNames:        fn,
-		variables.Files:             fl,
-		variables.FilesSizes:        fs,
-		variables.FilesCombinedSize: fcs,
-		variables.ArgsPost:          m,
+		variables.FilesNames: map[string][]string{
+			"": filesArgNames,
+		},
+		variables.FilesTmpNames: map[string][]string{
+			"": fileList,
+		},
+		variables.Files: map[string][]string{
+			"": filesNames,
+		},
+		variables.FilesSizes: map[string][]string{
+			"": fileSizes,
+		},
 		variables.ArgsPostNames: map[string][]string{
-			"": names,
+			"": postNames,
+		},
+		variables.ArgsPost: postFields,
+		variables.Args:     postFields,
+		variables.FilesCombinedSize: map[string][]string{
+			"": {fmt.Sprintf("%d", totalSize)},
 		},
-		variables.Args: m,
 	}
 
 	return nil

@@ -0,0 +1,60 @@
+package bodyprocessors
+
+import (
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/jptosso/coraza-waf/v2/types/variables"
+)
+
+func TestMultipartProcessor(t *testing.T) {
+	payload := `-----------------------------9051914041544843365972754266
+Content-Disposition: form-data; name="text"
+
+text default
+-----------------------------9051914041544843365972754266
+Content-Disposition: form-data; name="file1"; filename="a.txt"
+Content-Type: text/plain
+
+Content of a.txt.
+
+-----------------------------9051914041544843365972754266
+Content-Disposition: form-data; name="file2"; filename="a.html"
+Content-Type: text/html
+
+<!DOCTYPE html><title>Content of a.html.</title>
+
+-----------------------------9051914041544843365972754266--`
+	payload = strings.ReplaceAll(payload, "\n", "\r\n")
+
+	p := multipartBodyProcessor{}
+	if err := p.Read(strings.NewReader(payload), "multipart/form-data; boundary=---------------------------9051914041544843365972754266", "/tmp"); err != nil {
+		t.Error(err)
+	}
+
+	res := p.Collections()
+	if len(res[variables.FilesNames][""]) != 2 {
+		t.Errorf("Expected 2 files, got %d", len(res[variables.FilesNames]))
+	}
+	if len(res[variables.ArgsPostNames]) != 1 {
+		t.Errorf("Expected 1 args, got %d", len(res[variables.ArgsPostNames]))
+	}
+	if len(res[variables.ArgsPost]["text"]) == 0 || res[variables.ArgsPost]["text"][0] != "text default" {
+		t.Errorf("Expected text3 to be 'some super text content 3', got %v", res[variables.ArgsPost])
+	}
+	if len(res[variables.FilesTmpNames][""]) != 2 {
+		t.Errorf("Expected 2 files, got %d", len(res[variables.FilesTmpNames]))
+	}
+	if len(res[variables.FilesTmpNames]) > 0 {
+		if len(res[variables.FilesTmpNames][""]) == 0 {
+			t.Errorf("Expected files, got %d", len(res[variables.FilesTmpNames][""]))
+		} else {
+			fname := res[variables.FilesTmpNames][""][0]
+			if _, err := os.Stat(fname); err != nil {
+				t.Errorf("Expected file %s to exist", fname)
+			}
+
+		}
+	}
+}

@@ -91,7 +91,7 @@ func TestTxMultipart(t *testing.T) {
 	}
 	exp := map[string]string{
 		"%{args_post.text}":      "test-value",
-		"%{files_combined_size}": "50",
+		"%{files_combined_size}": "60",
 		"%{files}":               "a.html",
 		"%{files_names}":         "file1",
 	}

@@ -141,7 +141,7 @@ const (
 	// Geo contains the location information of the client
 	Geo                 RuleVariable = iota
 	RequestCookiesNames RuleVariable = iota
-	FilesTmpnames       RuleVariable = iota
+	FilesTmpNames       RuleVariable = iota
 	// ArgsNames contains the names of the arguments (POST and GET)
 	ArgsNames RuleVariable = iota
 	// ArgsGetNames contains the names of the GET arguments
@@ -243,7 +243,7 @@ var rulemap = map[RuleVariable]string{
 	ResponseHeaders:               "RESPONSE_HEADERS",
 	Geo:                           "GEO",
 	RequestCookiesNames:           "REQUEST_COOKIES_NAMES",
-	FilesTmpnames:                 "FILES_TMPNAMES",
+	FilesTmpNames:                 "FILES_TMPNAMES",
 	ArgsNames:                     "ARGS_NAMES",
 	ArgsGetNames:                  "ARGS_GET_NAMES",
 	ArgsPostNames:                 "ARGS_POST_NAMES",