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

net/textproto: allows multi-line header field names #34702

Closed
katiehockman opened this issue Oct 4, 2019 · 10 comments
Closed

net/textproto: allows multi-line header field names #34702

katiehockman opened this issue Oct 4, 2019 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@katiehockman
Copy link
Contributor

katiehockman commented Oct 4, 2019

Reported by @empijei

Go supports multiline headers with the obsolete leading whitespace syntax as described in RPC 7230 3.2.3 and 3.2.4. This is acceptable for header field values as long as servers "replace each received obs-fold with one or more SP octets prior to interpreting the field value or forwarding the message downstream"

However, this behavior also occurs with header field names, which isn't specified in any RFC and is likely an over-extension of HTTP parsing rules. In other words, the following is allowed, and resolves to Gopher-New- Line:

GET /path HTTP/1.1
Host: golang.org
Gopher-New-
 Line: This is a header on multiple lines
Content-Type: application/goroutines

Servers should reject requests with a field-name containing whitespace.

/cc @bradfitz @FiloSottile

@katiehockman katiehockman self-assigned this Oct 4, 2019
@katiehockman katiehockman added this to the Go1.14 milestone Oct 4, 2019
@bradfitz
Copy link
Contributor

bradfitz commented Oct 4, 2019

Does net/textproto do this intentionally for some reason? Do other protocol implementations using net/textproto require this? net/mail? net/smtp?

@bradfitz
Copy link
Contributor

bradfitz commented Oct 4, 2019

Looking at older RFCs (e.g. https://www.ietf.org/rfc/rfc2822.txt section 2.2) I'm not seeing any support for folding mid field name.

So this can probably be fixed in net/textproto without anything in net/http.

@bradfitz
Copy link
Contributor

bradfitz commented Oct 4, 2019

Yeah, even the classic RFC 822 makes it clear we're just doing it wrong:

https://www.ietf.org/rfc/rfc822.txt

     3.1.1.  LONG HEADER FIELDS

        Each header field can be viewed as a single, logical  line  of
        ASCII  characters,  comprising  a field-name and a field-body.
        For convenience, the field-body  portion  of  this  conceptual
        entity  can be split into a multiple-line representation; this
        is called "folding"

@bradfitz
Copy link
Contributor

bradfitz commented Oct 4, 2019

@katiehockman
Copy link
Contributor Author

So here's the dilemma:

There are two places we could fail headers of this type: in readContinuedLineSlice() or ReadMIMEHeader().

Initially, I thought it would be safe to change readContinuedLineSlice() to fail if the first line doesn't contain a colon, but this function is exported through ReadContinuedLineBytes() and we have no way of guaranteeing that people are only using this function to read header lines. If we start throwing errors if the first line doesn't contain a colon then we may break users.

So that leaves ReadMIMEHeader(). Unfortunately, ReadMIMEHeader() doesn't know the difference between a space from the normalization caused by readContinuedLineSlice() and a space that was in the request originally. We could just fail every field name with a space, but this function currently accepts field names with spaces (even if those are invalid server-side).

I think it's possible to do this check in ReadMIMEHeader() with some additional fields returned by readContinuedLineSlice() but it's adding non-insignificant complexity for something that is already rejected server-side, and likely isn't hurting anyone.

Note that Chrome and curl both accept responses with newlines and/or spaces in the header field names.

At this point I think we should just leave this alone. @bradfitz thoughts?

@bradfitz
Copy link
Contributor

bradfitz commented Oct 4, 2019

I'd add an optional param to:

func (r *Reader) readContinuedLineSlice() ([]byte, error) {

like:

validateFirstLine func([]byte) error

And then if it's nil, act like today.

But if it's non-nil, then call that validation func with the first line.

Then make ReadMIMEHeader pass a validation func that makes sure there's a colon on the first line or whatever.

And make the existing ReadContinuedLineBytes pass nil with no extra validation, like today.

So the end result is that only ReadMIMEHeader gets stricter. (and thus net/http, net/smtp, net/mail, etc).

@katiehockman
Copy link
Contributor Author

Sure I can look into that. @FiloSottile and I were brainstorming a few ways of doing this last week.

My concern was the additional complexity to these functions given that this issue doesn't seem likely to be a notable security risk. Sounds like the fix is important enough to warrant the changes though.

@bradfitz
Copy link
Contributor

bradfitz commented Oct 7, 2019

Even if net/http rejects it, there are other uses of net/textproto.

@bradfitz bradfitz changed the title net/http: allows multi-line header field names net/textproto: allows multi-line header field names Oct 7, 2019
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2019
@gopherbot
Copy link

Change https://golang.org/cl/199838 mentions this issue: textproto: do not allow multi-line header field names

erikdubbelboer added a commit to valyala/fasthttp that referenced this issue Dec 14, 2019
- Replace tabs with spaces at line starts to match net/http
- Don't allow multi line header names. See: golang/go#34702
erikdubbelboer added a commit to valyala/fasthttp that referenced this issue Dec 14, 2019
- Replace tabs with spaces at line starts to match net/http
- Don't allow multi line header names. See: golang/go#34702
@andrew-aladev
Copy link

Yes, the patch is right. Field name can't be multi line, only value can.

staylor14 added a commit to cloudfoundry/cli that referenced this issue Aug 19, 2020
Go 1.14's net/textproto/reader.go changes the reason for a malformed
header in golang/go#34702

Our malformed header integration test strictly relied on the net
package's output.  We no longer consider the flavor text of the error,
and only verify that the cause of the complaint is the name of the malformed
key we pass in via -H.

Found while sanity testing for

[#173716617](https://www.pivotaltracker.com/story/show/173716617)
JenGoldstrich pushed a commit to cloudfoundry/cli that referenced this issue Aug 20, 2020
Go 1.14's net/textproto/reader.go changes the reason for a malformed
header in golang/go#34702

Our malformed header integration test strictly relied on the net
package's output.  We no longer consider the flavor text of the error,
and only verify that the cause of the complaint is the name of the malformed
key we pass in via -H.

Found while sanity testing for

[#173716617](https://www.pivotaltracker.com/story/show/173716617)
@golang golang locked and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants