-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Comments
Does net/textproto do this intentionally for some reason? Do other protocol implementations using net/textproto require this? net/mail? net/smtp? |
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. |
Yeah, even the classic RFC 822 makes it clear we're just doing it wrong: https://www.ietf.org/rfc/rfc822.txt
|
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? |
I'd add an optional param to: func (r *Reader) readContinuedLineSlice() ([]byte, error) { like:
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 And make the existing So the end result is that only |
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. |
Even if net/http rejects it, there are other uses of net/textproto. |
Change https://golang.org/cl/199838 mentions this issue: |
- Replace tabs with spaces at line starts to match net/http - Don't allow multi line header names. See: golang/go#34702
- Replace tabs with spaces at line starts to match net/http - Don't allow multi line header names. See: golang/go#34702
Yes, the patch is right. Field name can't be multi line, only value can. |
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)
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)
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
:Servers should reject requests with a field-name containing whitespace.
/cc @bradfitz @FiloSottile
The text was updated successfully, but these errors were encountered: