From 04d2c21e532e14008b0d97f58f924f6bebdcd8ef Mon Sep 17 00:00:00 2001 From: Juan Pablo Tosso Date: Mon, 6 Dec 2021 17:29:04 -0300 Subject: [PATCH] fix: multipart body processor was closing the streams --- actions/prepend.go | 5 +++- body_buffer.go | 10 ++++--- body_buffer_test.go | 18 ++++++++++-- bodyprocessors/multipart.go | 1 - transaction.go | 15 ++++++++-- transaction_test.go | 55 +++++++++++++++++++++++++++++++++++++ 6 files changed, 92 insertions(+), 12 deletions(-) diff --git a/actions/prepend.go b/actions/prepend.go index 683ffb81f..b73fc6434 100644 --- a/actions/prepend.go +++ b/actions/prepend.go @@ -41,7 +41,10 @@ func (a *prependFn) Evaluate(r *coraza.Rule, tx *coraza.Transaction) { if err != nil { tx.Waf.Logger.Debug("failed to write buffer while evaluating prepend action") } - reader := tx.ResponseBodyBuffer.Reader() + reader, err := tx.ResponseBodyBuffer.Reader() + if err != nil { + tx.Waf.Logger.Debug("failed to read response body while evaluating prepend action") + } _, err = io.Copy(buf, reader) if err != nil { tx.Waf.Logger.Debug("failed to append response buffer while evaluating prepend action") diff --git a/body_buffer.go b/body_buffer.go index 4bc82fc3e..1fa9ee48e 100644 --- a/body_buffer.go +++ b/body_buffer.go @@ -56,12 +56,14 @@ func (br *BodyBuffer) Write(data []byte) (n int, err error) { } // Reader Returns a working reader for the body buffer in memory or file -func (br *BodyBuffer) Reader() io.Reader { +func (br *BodyBuffer) Reader() (io.Reader, error) { if br.writer == nil { - return bytes.NewReader(br.buffer.Bytes()) + return bytes.NewReader(br.buffer.Bytes()), nil } - _, _ = br.writer.Seek(0, 0) - return br.writer + if _, err := br.writer.Seek(0, 0); err != nil { + return nil, err + } + return br.writer, nil } // Size returns the current size of the body buffer diff --git a/body_buffer_test.go b/body_buffer_test.go index 0475ac82d..f33727f1a 100644 --- a/body_buffer_test.go +++ b/body_buffer_test.go @@ -27,7 +27,11 @@ func TestBodyReaderMemory(t *testing.T) { t.Error(err) } buf := new(strings.Builder) - if _, err := io.Copy(buf, br.Reader()); err != nil { + reader, err := br.Reader() + if err != nil { + t.Error(err) + } + if _, err := io.Copy(buf, reader); err != nil { t.Error(err) } if buf.String() != "test" { @@ -43,7 +47,11 @@ func TestBodyReaderFile(t *testing.T) { t.Error(err) } buf := new(strings.Builder) - if _, err := io.Copy(buf, br.Reader()); err != nil { + reader, err := br.Reader() + if err != nil { + t.Error(err) + } + if _, err := io.Copy(buf, reader); err != nil { t.Error(err) } if buf.String() != "test" { @@ -67,7 +75,11 @@ func TestBodyReaderWriteFromReader(t *testing.T) { t.Error(err) } buf := new(strings.Builder) - if _, err := io.Copy(buf, br.Reader()); err != nil { + reader, err := br.Reader() + if err != nil { + t.Error(err) + } + if _, err := io.Copy(buf, reader); err != nil { t.Error(err) } if buf.String() != "test" { diff --git a/bodyprocessors/multipart.go b/bodyprocessors/multipart.go index 19bd33d7f..a305a7180 100644 --- a/bodyprocessors/multipart.go +++ b/bodyprocessors/multipart.go @@ -30,7 +30,6 @@ func (mbp *multipartBodyProcessor) Read(reader io.Reader, mime string, storagePa req, _ := http.NewRequest("GET", "/", reader) req.Header.Set("Content-Type", mime) err := req.ParseMultipartForm(1000000000) - defer req.Body.Close() if err != nil { return err } diff --git a/transaction.go b/transaction.go index a83121055..ca1369529 100644 --- a/transaction.go +++ b/transaction.go @@ -506,7 +506,10 @@ func (tx *Transaction) ProcessRequest(req *http.Request) (*types.Interruption, e if err != nil { return tx.Interruption, err } - reader := tx.RequestBodyBuffer.Reader() + reader, err := tx.RequestBodyBuffer.Reader() + if err != nil { + return tx.Interruption, err + } req.Body = io.NopCloser(reader) } return tx.ProcessRequestBody() @@ -671,7 +674,10 @@ func (tx *Transaction) ProcessRequestBody() (*types.Interruption, error) { mime = m[0] } - reader := tx.RequestBodyBuffer.Reader() + reader, err := tx.RequestBodyBuffer.Reader() + if err != nil { + return nil, err + } // Chunked requests will always be written to a temporary file if tx.RequestBodyBuffer.Size() >= tx.RequestBodyLimit { @@ -790,7 +796,10 @@ func (tx *Transaction) ProcessResponseBody() (*types.Interruption, error) { tx.Waf.Rules.Eval(types.PhaseResponseBody, tx) return tx.Interruption, nil } - reader := tx.ResponseBodyBuffer.Reader() + reader, err := tx.ResponseBodyBuffer.Reader() + if err != nil { + return nil, err + } reader = io.LimitReader(reader, tx.Waf.ResponseBodyLimit) buf := new(strings.Builder) length, _ := io.Copy(buf, reader) diff --git a/transaction_test.go b/transaction_test.go index e3f24e5b5..d61feed56 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -15,8 +15,14 @@ package coraza import ( + "bufio" + "bytes" "fmt" + "io" + "io/ioutil" + "mime/multipart" "net/http" + "os" "regexp" "strconv" "strings" @@ -391,6 +397,55 @@ func TestAuditLogMessages(t *testing.T) { } +func TestProcessRequestMultipart(t *testing.T) { + req, _ := http.NewRequest("POST", "/some", nil) + if err := multipartRequest(req); err != nil { + t.Fatal(err) + } + tx := makeTransaction() + tx.RequestBodyAccess = true + if _, err := tx.ProcessRequest(req); err != nil { + t.Error(err) + } + if req.Body == nil { + t.Error("failed to process multipart request") + } + reader := bufio.NewReader(req.Body) + if _, err := reader.ReadString('\n'); err != nil { + t.Error("failed to read multipart request", err) + } +} + +func multipartRequest(req *http.Request) error { + var b bytes.Buffer + w := multipart.NewWriter(&b) + tempfile, err := os.CreateTemp("/tmp", "tmpfile*") + if err != nil { + return err + } + defer os.Remove(tempfile.Name()) + for i := 0; i < 1024*5; i++ { + // this should create a 5mb file + if _, err := tempfile.Write([]byte(strings.Repeat("A", 1024))); err != nil { + return err + } + } + var fw io.Writer + if fw, err = w.CreateFormFile("fupload", tempfile.Name()); err != nil { + return err + } + if _, err := tempfile.Seek(0, 0); err != nil { + return err + } + if _, err = io.Copy(fw, tempfile); err != nil { + return err + } + req.Body = ioutil.NopCloser(&b) + req.Header.Set("Content-Type", w.FormDataContentType()) + req.Method = "POST" + return nil +} + func BenchmarkTransactionCreation(b *testing.B) { waf := NewWaf() for i := 0; i < b.N; i++ {