net/url: Malformed query code breaks backwards compatibility

[jefferai]

What version of Go are you using (go version)?

1.12 and now 1.11.6

We have two projects (Consul and Vault) that implement a K/V store with path-based URLs. As Go didn't check for control characters in previous releases we have some users that ended up writing keys at locations with various control characters due to bad key/path generation code.

We implemented a check ourselves to filter such paths out but for those users that needed to be able to access data they'd already written the check could be turned off. With Go 1.12 out we've been trying to figure out a decent way to work around the fact that these paths now throw errors from within Go. It's not trivial given that the key is used for things like AEAD tag data.

Unfortunately the updates in Go 1.12 and now 1.11.6 mean our current plan of sticking with the 1.11.x series until we have sorted something out isn't viable, and we still don't have a good option going forward anyways.

I realize that due to the security nature of the previous behavior golang's backwards compatibility promise doesn't apply. However, it would still be very nice if those of us that need to be able to control this behavior for backwards compatibility reasons had a way to do so. I'm happy to work up a patch if it's likely to be accepted.

Pinging @bradfitz

[bradfitz]

I think your best option (especially short term) is forking Go's HTTP/1.x net/http.Transport code into your own package. It'll still be a RoundTripper. If you're concerned about diverging from upstream net/http, you could conditionally use your fork only if the outbound request contains control characters. If you wanted to minimize your fork's size you could even rip out all the things you didn't need, perhaps including keep-alive connections.

See also: #14952 (net/http: add switches to accept/send bogus HTTP?)

[bradfitz]

Cross-referencing the original bugs: #27302, #22907

[jefferai]

That's a significantly higher maintenance burden than patching out the call to stringContainsCTLByte and building releases on that, but either way is obviously not ideal. And simply forking the Transport code won't work if it's still using the URL package since that's where this change was made. Unless I'm missing something.

I understand the security motivations for disallowing this, but they don't apply to all use-cases, while it is breaking compatibility for all use-cases, to the point where it can make user data inaccessible without downstream projects forking Go. Even a package level variable we can set to control this behavior on program startup would be helpful and sufficient. I wouldn't ask this if the change were made when transitioning to Go 2, but...

[bradfitz]

Do you only care about the client side, server side, or both?

I assume (by you mentioning backwards compat) that if all your clients just upgrade, then the problem goes away and things can be URL-escaped %xx in the requests instead?

[jefferai]

To make sure I'm following -- you're saying that since the check is on the raw URL during parsing, if clients have control characters in the request path but URL-escape them, those chars will be let through (because at the point that the URL is being parsed by the server via net/url it's operating on the raw characters)?

In a general sense clients can be any HTTP client so there isn't really any "upgrading" that can be done to them, other than if I'm understanding what you're suggesting above. But I have a feeling it's not what you meant as that would have odd implications for the security benefits of the patch.

[bradfitz]

I still don't know from your bug which which side of net/http is causing you problems. But I'm increasing guessing it's the server.

See https://play.golang.org/p/TXrofuCNhlG ... the first form is not valid per RFC 7230/RFC 3986. A path character must be:

pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"

If it's the Server causing you problems and you need to support old clients doing it wrong (which?), then you could write a net.Listener that transformed their bogus bytes into %xx form in the request line. (You'd need to also inject a Connection: close) header so you don't need to keep too much state and deal with keep-alives.

[jefferai]

Hi Brad,

I've modified your code slightly with some examples not far off from real world things we've actually seen:

package main

import (
        "bufio"
        "log"
        "net/http"
        "strings"
)

func main() {
        req("HTTP /foobar/happy\x00birthday HTTP/1.0\r\n\r\n")
        req("HTTP /foobar/happy\x0dbirthday HTTP/1.0\r\n\r\n")
}

func req(s string) {
        req, err := http.ReadRequest(bufio.NewReader(strings.NewReader(s)))
        if err != nil {
                log.Printf("error for %q: %v", s, err)
                return
        }
        log.Printf("Got request with RequestURI %q, URL: %+v", req.RequestURI, req.URL)
}

In Go 1.11.5 you see this:

2019/03/19 09:43:46 Got request with RequestURI "/foobar/happy\x00birthday", URL: /foobar/happy%00birthday
2019/03/19 09:43:46 Got request with RequestURI "/foobar/happy\rbirthday", URL: /foobar/happy%0Dbirthday

In Go 1.11.6 you see this:

2019/03/19 09:46:44 error for "HTTP /foobar/happy\x00birthday HTTP/1.0\r\n\r\n": parse /foobar/happybirthday: net/url: invalid control character in URL
birthday: net/url: invalid control character in URLbirthday HTTP/1.0\r\n\r\n": parse /foobar/happy

Because we are using the paths in an opaque fashion, there is no security-related purpose for this change, however, it does mean that values stored using that path are now inaccessible over our APIs when we build with Go 1.11.6+ or 1.12. And it has knock-on security effects because, as I mentioned, that path is also used as tag data for an AEAD, so we cannot simply unilaterally move them server-side as it will invalidate the tag data.

The only solution we've been able to think of is essentially having to have every customer run a tool against pre-Go-1.11.6 versions of the software to scan for paths that may have control characters, and then manually figure out some upgrade paradigm, which will often involve changing other expected paths in other parts of their software.

This seemingly minor change has some very large backwards compat ripple effects, and while in some cases the change can enhance security, in other cases there is no security story for it at all. Yes, users did the wrong thing in the first place by having badly-formed paths. But Go happily accepted it. Was Go out of RFC spec? Yes, but again...Go happily accepted it.

I also wonder about the security implications. The original bug #22907 suggests that the security issue is that malformed paths can be used in a round-trip fashion. An alternate approach would be to not force client behavior by rejecting these on ingress but rather to ensure that when the URL is written back out for round tripping or redirection or whatever the case that it is properly escaped at that point in time.

If you won't consider reverting this patch, please consider some way to turn it off, so that we can layer our own protections but still provide backwards compatibility to our users as we have been on our own for some time.

[bradfitz]

Sorry, but adding knobs leads to godoc clutter and thus user fatigue, and leads to maintainers being burdened by maintaining that support forever. Some knobs are okay, but not for spec compliance things.

And there's no point reverting the patch in Go 1.11.x if we're doing to keep it in Go 1.12.x (which we are).

Unfortunately you'll need to write some code on your side (a custom net.Listener) to keep accepting those requests. You can read all net.Conn data up to "\r\n\r\n" and then parse the almost-HTTP/1.x requests you want to accept, transform them into valid HTTP/1.x requests, smuggling the payload into a new HTTP header, and then passing the net.Conn to the net/http.Server via your custom net.Listener.

Yes, that's work for you, but it means that you're the one paying the price for the hack, not Go & its users & maintainers forever.

[jefferai]

Nobody would need to pay the price for the hack if Go hadn't broken its compatibility promise for a change that is of dubious security benefit for only a small subset of use cases.

[bradfitz]

See https://golang.org/doc/go1compat#expectations

If your clients are sending binary gibberish on the wire, you can't expect it all to come through. You were getting lucky some of the time. Once their binary payloads contained a newline or various other special bytes, it would've failed it anyway.