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

x/net/http2: validate transmitted header field names & values #14048

Closed
bradfitz opened this issue Jan 21, 2016 · 3 comments
Closed

x/net/http2: validate transmitted header field names & values #14048

bradfitz opened this issue Jan 21, 2016 · 3 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 21, 2016

The opposite of #14029, we should also validate the header field names & values that we transmit, returning an error to the user if they have a bogus one.

@bradfitz bradfitz self-assigned this Jan 21, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Jan 21, 2016
@bradfitz bradfitz changed the title x/net/http: validate transmitted header field names & values x/net/http2: validate transmitted header field names & values Jan 21, 2016
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 31, 2016

CL https://golang.org/cl/19131 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Feb 1, 2016
This adds a way for http.Handlers to set trailers after the header has
already been flushed. This comment from the code explains:

// promoteUndeclaredTrailers permits http.Handlers to set trailers
// after the header has already been flushed. Because the Go
// ResponseWriter interface has no way to set Trailers (only the
// Header), and because we didn't want to expand the ResponseWriter
// interface, and because nobody used trailers, and because RFC 2616
// says you SHOULD (but not must) predeclare any trailers in the
// header, the official ResponseWriter rules said trailers in Go must
// be predeclared, and then we reuse the same ResponseWriter.Header()
// map to mean both Headers and Trailers.  When it's time to write the
// Trailers, we pick out the fields of Headers that were declared as
// trailers. That worked for a while, until we found the first major
// user of Trailers in the wild: gRPC (using them only over http2),
// and gRPC libraries permit setting trailers mid-stream without
// predeclarnig them. So: change of plans. We still permit the old
// way, but we also permit this hack: if a Header() key begins with
// "Trailer:", the suffix of that key is a Trailer. Because ':' is an
// invalid token byte anyway, there is no ambiguity. (And it's already
// filtered out) It's mildly hacky, but not terrible.

The official pre-declaring way still works. Example from net/http docs:
https://golang.org/pkg/net/http/#example_ResponseWriter_trailers
And ResponseWriter docs explaining it:
https://golang.org/pkg/net/http/#ResponseWriter

I don't want to add a new interface-assertable upgrade type (like
Hijacker or Flusher) for this because it's hurts composability and
makes everybody in the ecocsystem implement those, and two optional
interfaces is already bad enough. This is a weird enough feature
(Trailers by itself is weird enough), that I don't feel like a third
optional interface is worth it.

This code also filters invalid header fields (updates golang/go#14048),
since I had to update that code as part of this. But I want to later
return an error back to the user if possible. Or log something.

With this CL, all the grpc-go end2end tests pass with a new http2-based
Server implementation:
bradfitz/grpc-go@a06f8f0

Change-Id: I80f863d05a1810bd6f302f34932ad9df0a6646a6
Reviewed-on: https://go-review.googlesource.com/19131
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 31, 2016
This makes sure the net/http package never attempts to transmit a
bogus header field key or value and instead fails fast with an error
to the user, rather than relying on the server to maybe return an
error.

It's still possible to use x/net/http2.Transport directly to send
bogus stuff. This change only stops h1 & h2 usage via the net/http
package. A future change will update x/net/http2.

This change also moves some code from request.go to lex.go, which in a
separate future change should be moved so it can be shared with http2
to reduce code bloat.

Updates #14048

Change-Id: I0a44ae1ab357fbfcbe037aa4b5d50669a87f2856
Reviewed-on: https://go-review.googlesource.com/21326
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 19, 2016

CL https://golang.org/cl/23229 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue May 19, 2016
The http2.Transport was able to send bogus header keys & values.
This changes rejects them earlier, before they hit the wire.

In the process, mirror the lexical rules from the http package to x/net.
Maintaining two copies has gotten increasingly annoying.

Updates golang/go#14048

Change-Id: I20abcdeea92e7dc8706a1bbd60688ee8843a2b12
Reviewed-on: https://go-review.googlesource.com/23229
Reviewed-by: Andrew Gerrand <adg@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 19, 2016

CL https://golang.org/cl/23234 mentions this issue.

@gopherbot gopherbot closed this in 255e206 May 19, 2016
@golang golang locked and limited conversation to collaborators May 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.