[go] mime/multipart: nested boundary can have the outer boundary as prefix followed by a dash
45 views
Skip to first unread message
Damien Neil (Gerrit)
Mar 31, 2022, 9:18:11 PM3/31/22
to Olivier Szika, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Robert Griesemer, Cherry Mui, Gopher Robot, Minux Ma, golang-co...@googlegroups.com
Damien Neil submitted this change.
Approvals:
Robert Griesemer: Looks good to me, approved
Damien Neil: Looks good to me, approved; Trusted; Run TryBots
Cherry Mui: Trusted
Gopher Robot: TryBots succeeded
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 <dn...@google.com>
Run-TryBot: Damien Neil <dn...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Damien Neil <dn...@google.com>
Trust: Cherry Mui <cher...@google.com>
Reviewed-by: Robert Griesemer <g...@golang.org>
---
M src/mime/multipart/multipart.go
M src/mime/multipart/multipart_test.go
2 files changed, 109 insertions(+), 14 deletions(-)
diff --git a/src/mime/multipart/multipart.go b/src/mime/multipart/multipart.go
index 1054e7a..c7bcb4d 100644
--- a/src/mime/multipart/multipart.go
+++ b/src/mime/multipart/multipart.go
@@ -264,7 +264,8 @@
// 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 @@
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 741d230..e043e36 100644
--- a/src/mime/multipart/multipart_test.go
+++ b/src/mime/multipart/multipart_test.go
@@ -291,24 +291,34 @@
}
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 @@
},
},
},
+
// Issue 12662: Check that we don't consume the leading \r if the peekBuffer
// ends in '\r\n--separator-'
{
@@ -767,6 +778,7 @@
},
},
},
+
// Issue 12662: Same test as above with \r\n at the end
{
name: "peek buffer boundary condition",
@@ -782,6 +794,7 @@
},
},
},
+
// 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 @@
},
},
},
+
// 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 @@
},
},
+ // 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(),
}
To view, visit change 338549. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages