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/http: change Error to generate an HTML page #13150

Closed
rsc opened this issue Nov 4, 2015 · 18 comments

Comments

Projects
None yet
@rsc
Copy link
Contributor

commented Nov 4, 2015

There is constant background noise about this web client or that web client mistakenly treating http.Error's responses as HTML and therefore being subject to scripting attacks. This is awful, and depressing, and generally disgusting.

One way to eliminate the noise would be to change Error from sending back (approximately)

Content-Type: text/plain

<ERROR HERE>

to

Content-Type: text/html

<pre>
&lt;ERROR HERE&gt;

That is, if everyone is going to interpret the result as HTML, okay fine, let's send (and correctly Content-Type) an actual HTML response with proper escaping of the message.

Anyone see any reasons not to do this? The only one I can think of is that it makes clients of API services that send back http.Error errors have to deal with the HTML, but as a writer of API service clients myself, most of the errors I see come back in HTML anyway, because they're generated by some box in front of the API service.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

Yeah, sure. This is the least offensive option.

@dsymonds

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

It does make a mess for people trying to hit HTTP servers with wget/curl and then would not be able to read non-trivial error messages. It also makes a mess if an HTTP client receives such an error, and escapes it again for embedding in its own error message (e.g. frontend reporting a backend error).

Do we have a list of user agents that don't respect the existing Content-Type+X-Content-Type-Options headers? Are we deciding to support them at the expense of other use cases? Will they still behave reasonably with the proposed change?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 5, 2015

Yes, I would like to wait on this change until we can get a public statement with real data here about which user agents mess this up, and publish that information on this bug.

I don't want to just perpetuate the idea that all browsers always suck. Certainly it's not all of them.

@adg

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2015

I'm concerned about breaking clients of Go services that use the current implementation. They expect error messages in plain text; swapping it out for HTML could cause them to do the wrong thing.

I agree that I'd like to see some hard data on affected browsers before taking further steps.

@slrz

This comment has been minimized.

Copy link

commented Nov 7, 2015

I concur with Andrew. This certainly has the potential to break some of our clients. In case this gets done, the change should be made very well known, so servers can remove the calls to http.Error if deemed necessary.

@rsc rsc modified the milestones: Unplanned, Go1.6 Dec 17, 2015

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2016

I don't have any hard data about affected browsers, and I don't have anything public I can point to. That said, I have been told that the Adobe Reader browser plugin, among others, is vulnerable to attacks when the user can control parts of the error message (which happens when the Go error string quotes part of the user's input). This is true even with the Content-Type and X-Content-Type-Options headers, as the browser plugin apparently ignores those headers.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.7, Unplanned Jan 21, 2016

@ailg

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2016

I talked with some colleagues to get you more data:

  1. text/plain triggers content sniffing in IE. Microsoft recommendation (https://msdn.microsoft.com/en-us/library/ms775147(v=vs.85).aspx) is to use something more specific (e.g. application/go-error-message). Other browsers are OK (they will respect text/plain and not interpret it as HTML).
  2. As Ian just said the main problem is plugins, on all browsers, as they ignore headers (Content-Type, X-Content-Type-Options), read the content, sniff, execute then can execute javascript (XSS).
    • Adobe Reader (PDF) will execute a PDF if it's found anywhere in the page
    • Adobe Flash (SWF) is a bit more restrictive, it'll only execute if the flash file is at the beginning. Often errors are prefixed with a program string (e.g. error: %v) in which case this attack would not work, but that's not a guarantee.

I hope this helps so you can make a decision, let me know if you need more info.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 27, 2016

I think only old MSIE is a problem. MSIE 8+ gets this right, I believe: https://blogs.msdn.microsoft.com/ie/2008/07/02/ie8-security-part-v-comprehensive-protection/

If the problem is only Adobe, perhaps we can vary our behavior based on the User-Agent and only HTML escape for Adobe and ancient MSIE.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2016

If the problem is only Adobe, perhaps we can vary our behavior based
on the User-Agent and only HTML escape for Adobe and ancient MSIE.

Given that Error only takes a ResponseWriter, is it actually possible for it
to determine the user agent without undue dirtiness?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 28, 2016

@rogpeppe, oh, I always forget which helpers take only ResponseWriter vs ResponseWriter and Request.

But it could still work. The ResponseWriter has the request inside of it, so we can cheat and interface assert it to the private *http.response type to get the Request. It's not super gross, almost no code, and at least it won't impact user code. It won't work for custom ResponseWriter types, but I think that's okay. http.Error is just a small helper utility.

@slrz

This comment has been minimized.

Copy link

commented Jan 28, 2016

What's the rationale for not fixing it properly on the plugin side? Adobe does have the infrastructure in place to push fixes to users, right? Do too many sites rely on the fact that serving a PDF or Flash file with a wrong Content-Type header works today?

Also, Go HTTP servers can't be the only thing serving text/plain while possibly including user-provided content.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 28, 2016

@slrz, wrong bug tracker. Ask Adobe. :)

@lawrence9811

This comment has been minimized.

Copy link

commented Apr 6, 2016

I think we need to leave the current function untouched as existing program may break if we simply change:
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
to
w.Header().Set("Content-Type", "text/html; charset=utf-8")

I'd propose we make it a new function like this, use Fprint instead of Fprintln, and content type set to text/html:
func ErrorHTML(w ResponseWriter, error string, code int) {
w.Header().Set("Content-Type", "text/html; charset=utf-8")
w.Header().Set("X-Content-Type-Options", "nosniff")
w.WriteHeader(code)
fmt.Fprint(w, error)
}

HTML custom error page is very common now to help visitor to get back on the right page. As long as the correct status code (e.g. 404) is sent in the HTTP header it shouldn't break any well designed robots.

@bradfitz bradfitz added the Thinking label May 16, 2016

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Go1.7 May 16, 2016

@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Oct 20, 2016

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 May 24, 2017

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

It looks like this only matters for ancient software. MSIE 8 and Adobe Flash are at least soon to be no longer with us. I retract this suggestion as infeasible / maybe no longer necessary.

@rsc rsc closed this Nov 22, 2017

@andybons andybons self-assigned this Mar 23, 2018

@andybons andybons modified the milestones: Go1.10, Go1.10.1 Mar 23, 2018

@andybons andybons reopened this Mar 23, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

Ugh, why?

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

This can be mitigated without breaking anyone (unless they rely specifically on the MIME type of the error responses). The content doesn't change and the display of the text in the browser doesn't either.

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

whoops. sorry. wrong issue.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 24, 2018

whoops. sorry. wrong issue.

Phew. :)

@golang golang locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.