From c4efe7fb2777fca05a904b0c62bee8915e13b03f Mon Sep 17 00:00:00 2001 From: Olivier Szika Date: Fri, 30 Jul 2021 12:02:51 +0200 Subject: [PATCH] mime/multipart: allow nested boundary with outer boundary+dash prefix Fixes #46042 Change-Id: Icd243eb12c6e260aeead04710f12340048a0e859 Reviewed-on: https://go-review.googlesource.com/c/go/+/338549 Trust: Damien Neil Run-TryBot: Damien Neil TryBot-Result: Gopher Robot Reviewed-by: Damien Neil Trust: Cherry Mui Reviewed-by: Robert Griesemer --- src/mime/multipart/multipart.go | 21 ++++++- src/mime/multipart/multipart_test.go | 84 ++++++++++++++++++++++++---- 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/src/mime/multipart/multipart.go b/src/mime/multipart/multipart.go index 1054e7a4ce..c7bcb4d121 100644 --- a/src/mime/multipart/multipart.go +++ b/src/mime/multipart/multipart.go @@ -264,7 +264,8 @@ func scanUntilBoundary(buf, dashBoundary, nlDashBoundary []byte, total int64, re // and the caller has verified already that bytes.HasPrefix(buf, prefix) is true. // // matchAfterPrefix returns +1 if the buffer does match the boundary, -// meaning the prefix is followed by a dash, space, tab, cr, nl, or end of input. +// meaning the prefix is followed by a double dash, space, tab, cr, nl, +// or end of input. // It returns -1 if the buffer definitely does NOT match the boundary, // meaning the prefix is followed by some other character. // For example, "--foobar" does not match "--foo". @@ -278,9 +279,25 @@ func matchAfterPrefix(buf, prefix []byte, readErr error) int { return 0 } c := buf[len(prefix)] - if c == ' ' || c == '\t' || c == '\r' || c == '\n' || c == '-' { + + if c == ' ' || c == '\t' || c == '\r' || c == '\n' { return +1 } + + // Try to detect boundaryDash + if c == '-' { + if len(buf) == len(prefix)+1 { + if readErr != nil { + // Prefix + "-" does not match + return -1 + } + return 0 + } + if buf[len(prefix)+1] == '-' { + return +1 + } + } + return -1 } diff --git a/src/mime/multipart/multipart_test.go b/src/mime/multipart/multipart_test.go index 741d2304ed..e043e36ef7 100644 --- a/src/mime/multipart/multipart_test.go +++ b/src/mime/multipart/multipart_test.go @@ -291,24 +291,34 @@ func TestLineLimit(t *testing.T) { } func TestMultipartTruncated(t *testing.T) { - testBody := ` + for _, body := range []string{ + ` This is a multi-part message. This line is ignored. --MyBoundary foo-bar: baz Oh no, premature EOF! -` - body := strings.ReplaceAll(testBody, "\n", "\r\n") - bodyReader := strings.NewReader(body) - r := NewReader(bodyReader, "MyBoundary") +`, + ` +This is a multi-part message. This line is ignored. +--MyBoundary +foo-bar: baz - part, err := r.NextPart() - if err != nil { - t.Fatalf("didn't get a part") - } - _, err = io.Copy(io.Discard, part) - if err != io.ErrUnexpectedEOF { - t.Fatalf("expected error io.ErrUnexpectedEOF; got %v", err) +Oh no, premature EOF! +--MyBoundary-`, + } { + body = strings.ReplaceAll(body, "\n", "\r\n") + bodyReader := strings.NewReader(body) + r := NewReader(bodyReader, "MyBoundary") + + part, err := r.NextPart() + if err != nil { + t.Fatalf("didn't get a part") + } + _, err = io.Copy(io.Discard, part) + if err != io.ErrUnexpectedEOF { + t.Fatalf("expected error io.ErrUnexpectedEOF; got %v", err) + } } } @@ -751,6 +761,7 @@ html things }, }, }, + // Issue 12662: Check that we don't consume the leading \r if the peekBuffer // ends in '\r\n--separator-' { @@ -767,6 +778,7 @@ Content-Type: application/octet-stream }, }, }, + // Issue 12662: Same test as above with \r\n at the end { name: "peek buffer boundary condition", @@ -782,6 +794,7 @@ Content-Type: application/octet-stream }, }, }, + // Issue 12662v2: We want to make sure that for short buffers that end with // '\r\n--separator-' we always consume at least one (valid) symbol from the // peekBuffer @@ -799,6 +812,7 @@ Content-Type: application/octet-stream }, }, }, + // Context: https://github.com/camlistore/camlistore/issues/642 // If the file contents in the form happens to have a size such as: // size = peekBufferSize - (len("\n--") + len(boundary) + len("\r") + 1), (modulo peekBufferSize) @@ -832,6 +846,52 @@ val }, }, + // Issue 46042; a nested multipart uses the outer separator followed by + // a dash. + { + name: "nested separator prefix is outer separator followed by a dash", + sep: "foo", + in: strings.Replace(`--foo +Content-Type: multipart/alternative; boundary="foo-bar" + +--foo-bar + +Body +--foo-bar + +Body2 +--foo-bar-- +--foo--`, "\n", "\r\n", -1), + want: []headerBody{ + {textproto.MIMEHeader{"Content-Type": {`multipart/alternative; boundary="foo-bar"`}}, + strings.Replace(`--foo-bar + +Body +--foo-bar + +Body2 +--foo-bar--`, "\n", "\r\n", -1), + }, + }, + }, + + // A nested boundary cannot be the outer separator followed by double dash. + { + name: "nested separator prefix is outer separator followed by double dash", + sep: "foo", + in: strings.Replace(`--foo +Content-Type: multipart/alternative; boundary="foo--" + +--foo-- + +Body + +--foo--`, "\n", "\r\n", -1), + want: []headerBody{ + {textproto.MIMEHeader{"Content-Type": {`multipart/alternative; boundary="foo--"`}}, ""}, + }, + }, + roundTripParseTest(), } -- 2.50.0