strconv: add HTML quoting for string returned by strconv.ParseInt error

[ianlancetaylor]

We have a report of a potential XSS vulnerability due to code that does, effectively, this:

num, err := strconv.Atoi(r.URL.Query().Get("field"))
    if err != nil {
        http.Error(w, err.Error(), http.StatusBadRequest)
}

The issue is that strconv.Atoi and friends return the string they are trying to parse as part of the error value, and it is included in the err.Error string. http.Error adds headers that are intended to ensure that returned page is not interpreted as HTML, but reportedly some HTML readers ignore these options.

Perhaps this kind of code should be more careful, or perhaps strconv.ParseInt should not return the input in the error message, or perhaps HTML readers should not be buggy. But we are where we are. We don't necessarily have to change anything in the Go library, but it occurs to me that there is one relatively simple way to help code vulnerable to this sort of attack: quote all non-ASCII characters in the error message, and also quote any angle brackets. Since these characters can not reasonably appear in strings passed to strconv.Atoi and friends, this doesn't seem like a significant handicap, and should fit within the Go 1 compatibility guarantee.

I have a CL for this that I will send out. I've opened this issue for discussion in case people think this change might be unwise or unwarranted.

[mdempsky]

It seems unfortunate for strconv to need to know about HTML escaping, and not all strconv errors will be output into an HTML relevant context (e.g., console/log output). It also punishes users who do proper HTML escaping already, because their error messages will be double escaped.

Does the report specify which HTML readers are vulnerable?

[DisposaBoy]

http.Error adds headers that are intended to ensure that returned page is not interpreted as HTML, but reportedly some HTML readers ignore these options.

If I understood the issue correctly, in this case, the client is broken and I don't think there's anything the server can reasonably do to fix that.
It's no different than if the server intentionally outputs

Content-Type: text/plain...
...
example code <script>alert('hello world')</script>

IMO, any code that dumps undefined internal state like error messages is broken and trying to be helpful is only going to mask the problem.

I also think that this would potentially make the error message useless because it would no longer report what it actually failed to parse (I input < and it complains about <)

[gopherbot]

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

[bradfitz]

I'm opposed to changing anything.

[ianlancetaylor]

@mdempsky I agree that clearly strconv.ParseInt should not have to care about HTML quoting. In retrospect I think it was probably a mistake for the error returned by ParseInt to include the input string. That should have been left up to the caller, not made the default.

I am not worried about callers who do proper HTML quoting, because no normal input to ParseInt is going to include angle brackets anyhow. In any case, with my proposed change the error message simply won't contain any angle brackets, so any extra quoting done by the caller will most likely have no effect.

The report was for people using flash and Adobe reader plugins to Internet Explorer.

@DisposaBoy Yes, of course the HTML reader is broken. That is not in dispute here. The error message is already quoted using strconv.Quote, so in my opinion adding some additional quoting will do little harm.

[mdempsky]

To clarify, by "HTML escaping" I thought you were proposing that < and > would be replaced by < and >, which I'd be opposed to.

I see that strconv already does Go-style string literal escaping in error messages (e.g., " -> \"), and I think applying that sort of escaping to < and > (like in your CL) is okay, though unfortunate.

[rsc]

There's no reason strconv is exceptional here. Unless we're going to change every Error method in the entire code base, surely the fix belongs somewhere else. I filed #13150.