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: don't sniff Content-Type in Server when X-Content-Type-Options:nosniff #24795

Closed
bradfitz opened this issue Apr 10, 2018 · 15 comments
Closed
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2018

In https://go-review.googlesource.com/c/go/+/89275 the HTTP/1 server was modified to not sniff Content-Type when nosniff was present.

Do the same for HTTP/2.

@bradfitz bradfitz added this to the Go1.11 milestone Apr 10, 2018
@mikesamuel

This comment has been minimized.

Copy link
Contributor

@mikesamuel mikesamuel commented Apr 10, 2018

I can take this on.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Apr 10, 2018

See also #24513

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 15, 2018

Change https://golang.org/cl/107295 mentions this issue: http2: don't sniff Content-type in Server when X-Content-Type-Options:nosniff

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Apr 18, 2018

Needs to be bundled back into std.

@bradfitz bradfitz reopened this Apr 18, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 5, 2018

Change https://golang.org/cl/111655 mentions this issue: net/http: update bundled http2

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Jul 26, 2018

This change is probably incompatible with ever doing #24513 (automatically add X-Content-Type-Options:nosniff), since adding the header automatically is now equivalent to disabling server-side content sniffing for everyone. Do we want to close off that avenue?

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Jul 30, 2018

I'm reopening this for confirmation that we actually want this change.

To my understanding, it's generally accepted that it's always a good idea to set the X-Content-Type-Options:nosniff header. #24513 proposes automatically adding this header; I believe the only reason we haven't followed through on it is that it causes failures in tests which have unreasonable expectations about the precise set of headers returned. Inside Google, our standard http package wrapper always adds this header.

Changes https://golang.org/cl/89275 and https://golang.org/cl/107295 disable server-side Content-Type sniffing when X-Content-Type-Options:nosniff is set.

If we tie client-side and server-side Content-Type sniffing in this fashion, then #24513 becomes equivalent to disabling the automatic setting of the Content-Type by default. This will break more than just tests; I'm certain plenty of users are depending on this ability.

I think we're better off leaving these decoupled so that we can go ahead with setting X-Content-Type-Options:nosniff by default. That change is, so far as I know, a strict security upgrade for all users and shouldn't break anything other than overly-restrictive tests. In contrast, the change here only applies to users who 1. set X-Content-Type-Options:nosniff, and 2. don't set Content-Type explicitly; even if it's a security benefit in this case, it's one which applies to fewer users than #24513 will.

@neild neild reopened this Jul 30, 2018
@neild neild assigned neild and bradfitz and unassigned neild Jul 30, 2018
@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jul 30, 2018

I'm fine reverting it. Was that decided? I wasn't the one who initiated this change, though. Are we waiting on opinions or do you want me to just revert it?

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Jul 30, 2018

I think you're the decider on whether to revert or not; I'm just presenting my opinion here.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jul 30, 2018

That change is, so far as I know, a strict security upgrade for all users and shouldn't break anything other than overly-restrictive tests.

This is not true, otherwise modern browsers would always behave as if it were set. It still breaks loading script and style tags with the wrong Content-Type, or HTML without Content-Type. https://fetch.spec.whatwg.org/#x-content-type-options-header

The problem with server-side sniffing is that it's less defined and less context-aware than browser sniffing, will probably be more unexpected, and not as well scrutinized as time passes. Sniffing is dizzily complex: https://mimesniff.spec.whatwg.org

The rationale of this change is that if nosniff is explicitly set we shouldn't defuse it and override it by doing server-side sniffing. It's true that always setting nosniff would be a bigger win, but I don't think it's something we can get away with in the open ecosystem yet. Note how even #24513 was restricted to only a few Content-Types, until it was explained to me that the world is more complex than I thought and it would be useless.

Inside Google I think we can manage to keep both default nosniff and this change, by landing it well, while on the outside I think this is the best we can do. So I'm -1 on reverting.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jul 31, 2018

I think we should revert it for now and figure it out all at once if/when #24513 happens. I agree with @neild that we shouldn't close off any avenues.

Also, I just verified that the net/http server won't mimesniff if the response "Content-Type" header's []string value is set to nil. (that is, in the map, but with no values)

So if Google wants such behavior internally, its middleware that sets nosniff can also set the Content-Type to nil.

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Jul 31, 2018

Okay, I definitely think this should be reverted now. It turns out that hoisting the automatic setting of Content-Type into user code is quite a bit more complicated than I'd realized.

If the Content-Type isn't set by the user, the http package sets one based on the first 512 bytes of data written to the ResponseWriter. If a user wants to set X-Content-Type-Options:nosniff but preserve the server-side sniffing, they need to manage the buffering of the first 512 bytes of response themselves. This is more complicated than just wrapping a custom ResponseWriter around the usual one, since the ResponseWriter interface doesn't have a Close method. (End-of-response being signaled by the handler function returning rather than an explicit close.)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 31, 2018

Change https://golang.org/cl/126895 mentions this issue: http2: revert CL 107295 (don't sniff Content-type in Server when nosniff)

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jul 31, 2018

@neild, I sent two CLs. PTAL.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 31, 2018

Change https://golang.org/cl/126896 mentions this issue: net/http: revert CL 89275 (don't sniff Content-Type when nosniff set)

gopherbot pushed a commit to golang/net that referenced this issue Jul 31, 2018
…iff)

Updates golang/go#24795

Change-Id: Idb018ad9eba1292e91d9339190fdd24ef8a0af4e
Reviewed-on: https://go-review.googlesource.com/126895
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
@gopherbot gopherbot closed this in d3c3aaa Jul 31, 2018
jeet-parekh added a commit to jeet-parekh/go that referenced this issue Jul 31, 2018
Also updates the bundled http2 to x/net/http2 git rev 49c15d80 for:

   http2: revert CL 107295 (don't sniff Content-type in Server when nosniff)
   https://golang.org/cl/126895

Fixes golang#24795

Change-Id: I6ae1a21c919947089274e816eb628d20490f83ce
Reviewed-on: https://go-review.googlesource.com/126896
Reviewed-by: Damien Neil <dneil@google.com>
@golang golang locked and limited conversation to collaborators Jul 31, 2019
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
5 participants
You can’t perform that action at this time.