Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mime: bypassing multi-boundary check via Parameter Value Continuations #47602

Open
SYM01 opened this issue Aug 9, 2021 · 7 comments
Open

mime: bypassing multi-boundary check via Parameter Value Continuations #47602

SYM01 opened this issue Aug 9, 2021 · 7 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@SYM01
Copy link

SYM01 commented Aug 9, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/sym01/Library/Caches/go-build"
GOENV="/Users/sym01/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/sym01/Workspace/Go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/sym01/Workspace/Go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.4"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cr/jbhvnrzx39s46k8btfr1s2_r0000gp/T/go-build52418455=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/JLYLXf40j8y

What did you expect to see?

net/http has a secure-by-default implement for multipart/form-data that doesn't allow multi boundaries.

go/src/mime/mediatype.go

Lines 188 to 191 in 507cc34

if _, exists := pmap[key]; exists {
// Duplicate parameter name is bogus.
return "", nil, errors.New("mime: duplicate parameter name")
}

E.g., It will stop parsing the form with header Content-Type: multipart/form-data;boundary="boundary";boundary="another-boundary".

What did you see instead?

Due to the support of RFC 2231, we can bypass this security check with header Content-Type: multipart/form-data;boundary="boundary";boundary*0="another";boundary*1="-boundary"

This incorrect implementation could be used for bypassing some security mechanisms, such as Web Application Firewall.
SYM01 added a commit to SYM01/go that referenced this issue Aug 9, 2021
@SYM01
mime: avoid parameter override issue and improve the security. fixed g…
bbd7777 
…olang#47602
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 9, 2021
@seankhliao
Copy link
Member

cc @neild

@neild
Copy link
Contributor

neild commented Aug 9, 2021

Is there some RFC or other authoritative source indicating that the current implementation is incorrect? RFC 2231 uses Content-Type as an example and makes no indication that the boundary parameter is excluded.

@SYM01
Copy link
Author

SYM01 commented Aug 10, 2021 •
edited

Yea, the following Content-Type is normal, and should be supported.

Content-Type: message/external-body; access-type=URL;
URL*0="ftp://";
URL*1="cs.utk.edu/pub/moore/bulk-mailer/bulk-mailer.tar"

But how about the following Content-Type ? Which URL should be treated as the right one?

Content-Type: message/external-body; access-type=URL;
URL="ftp://evil.com/path/to/xxx";
URL*0="ftp://";
URL*1="cs.utk.edu/pub/moore/bulk-mailer/bulk-mailer.tar"

RFC 2231 doesn't indicate that which one is right, but Go will always get the second URL.

@neild
Copy link
Contributor

neild commented Aug 10, 2021

Ah, I get you. The problem is that we aren't detecting duplicate parameter names when one is specified as an RFC 2231 continuation. (Sorry, I misread your initial example and misunderstood the issue.)

@neild neild added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 10, 2021
@neild neild changed the title net/http: bypassing multi-boundary check via Parameter Value Continuations mime: bypassing multi-boundary check via Parameter Value Continuations Aug 10, 2021
@neild
Copy link
Contributor

neild commented Aug 10, 2021

We have two test cases for mime.ParseMediaType explicitly verifying that when a parameter is present in both RFC 2231 and traditional forms, the RFC 2231 form takes precedence.

These test cases come from:
http://test.greenbytes.de/tech/tc2231/#attfnboth
http://test.greenbytes.de/tech/tc2231/#attfnboth2

RFC 5987 Section 4.2 states:

Header field specifications need to define whether multiple instances
of parameters with identical parmname components are allowed, and how
they should be processed. This specification suggests that a
parameter using the extended syntax takes precedence. This would
allow producers to use both formats without breaking recipients that
do not understand the extended syntax yet.

The test results from the greenbytes.de test cases indicate that most browsers pick the RFC 2231 encoded value.

I think the current behavior is consistent with common practice in interpreting duplicated parameters.

@SYM01
Copy link
Author

SYM01 commented Aug 14, 2021

Yea, I totally agree with you. It's a trade-off between compatibility and security.

This issue is more like a proposal instead of a bug.

@iredmail
Copy link

iredmail commented Oct 8, 2021

I got similar issue but with real-world email header, sample:

Content-Type: multipart/alternative; boundary="----RTIR0GV26TDW15Q5CVX979LBL6XN18"; boundary="----RTIR0GV26TDW15Q5CVX979LBL6XN18"

There's a proposal but closed without a fix: #28618

This was referenced Oct 8, 2021
Closed
Closed
Closed
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@seankhliao seankhliao added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@neild @iredmail @seankhliao @SYM01