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

Jump to bottom

net/http: allow user-defined behaviour on malformed HTTP requests #25116

Open
mjwbyrne opened this issue Apr 27, 2018 · 9 comments
Open

net/http: allow user-defined behaviour on malformed HTTP requests #25116

mjwbyrne opened this issue Apr 27, 2018 · 9 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@mjwbyrne
Copy link

mjwbyrne commented Apr 27, 2018 •
edited

The net/http HTTP server handles certain malformed HTTP requests completely autonomously, without ever passing control to non-library code. This means that the application is never made aware of them, so it cannot take any action, e.g. logging that a malformed request was received. It also prevents the application from customising the response.

For example, running a minimal Go HTTP server at localhost:8000 and doing this:

curl -v "http://localhost:8000/foo bar"

results in

*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /foo bar HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.55.1
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< Content-Type: text/plain; charset=utf-8
< Connection: close
< 
* Closing connection 0
400 Bad Request

(The error is caused by the whitespace in the first line of the request - the parser naively splits it on space and expects [method, URI, protocol]).

This is a problem because:

  • The application can't log the malformed request.
  • I can't make a guarantee like "my API always returns JSON", because "400 Bad Request" is not valid JSON.
  • The error message is very little help to the end user, and when the end user contacts me for support I have nothing to go on when diagnosing the problem because I couldn't log the request.
  • Certain proxy_pass directives in Nginx do a silent (and probably unexpected) URL-decode on the request URI. So if you run a Go HTTP server behind an Nginx reverse proxy (surely a common scenario), the error above will be triggered by a correct-looking request with a %20 somewhere in it.

I propose adding an exported variable of type

func(error, http.ResponseWriter, []byte)

to http.Server. It can be set by the application before ListenAndServe or ListenAndServeTLS is called. If this is nil (the default), the current behaviour stays. If it is not nil, then it is called (with the error, responseWriter and, optionally, a byte slice containing the offending part of the request - or even all of it?) when the server encounters a malformed request which it cannot pass on to the handler.
@bcmills bcmills added this to the Unplanned milestone Apr 27, 2018
@bcmills
Copy link
Contributor

bcmills commented Apr 27, 2018

CC: @bradfitz

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 27, 2018
@bradfitz
Copy link
Contributor

bradfitz commented May 4, 2018

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

The signature func(error, http.ResponseWriter, []byte) is a bit bizarre, especially that last argument. If it's malformed, that means we don't really understand it, so it's not clear we even know what's wrong. And how does this work with HTTP/2? Give you the whole binary frame? Or is this an HTTP/1-only thing, with the idea that HTTP/2 requests are all well formed since they haven't degraded over years yet?

I think I'd be fine adding in some HTTP1-specific hook for malformed requests, but I don't think I'd want to involve ResponseWriter at all. I'd prefer to just provide the net.Conn and the bytes consumed. It'd probably look more like the Hijacker interface. i.e. "On bogus HTTP/1.x request, the connection is all yours... have fun!"

@bradfitz
Copy link
Contributor

bradfitz commented May 4, 2018

If somebody wants to make a prototype along those lines and send a CL we can take a look.

@mjwbyrne
Copy link
Author

mjwbyrne commented May 4, 2018

Regarding the signature, the idea is that control is passed to the application to do whatever it wants. It might just discard the []byte (if, as you say, it just doesn't know what to do with a malformed request), but why not let the application decide whether the request is so broken it can't be dealt with? For example, in the "literal space in the URL" example I gave originally, it's conceivable that application code could in fact rescue the request.

I can't comment on how this would interact with HTTP/2 because I don't know enough (i.e. anything, really) about HTTP/2.

The Hijacker-style solution sounds great to me. Happy to knock out a first draft and send it for review.

@bradfitz
Copy link
Contributor

bradfitz commented May 4, 2018

it's conceivable that application code could in fact rescue the request.

Maybe, but you won't be able to pass it back to net/http to continue using. But yeah, you might be able to fire back a response that the client is happy with & then close the TCP connection. If you're cool with that.

@mjwbyrne
Copy link
Author

mjwbyrne commented May 4, 2018

Yes, that's what I had in mind - when a malformed request comes in, the hook gets called and from then on it's up to the application to handle it entirely - if it wants to rescue the request it has to do all the header parsing and so on by itself.

I expect the real-life use cases would simply be to respond with a 400 Bad Request, but in a more customisable way than literally putting "400 Bad Request" on the wire. Also logging.

@gopherbot
Copy link

Change https://golang.org/cl/111695 mentions this issue: net/http: add a malformed HTTP request callback to http.Server

@mjwbyrne
Copy link
Author

mjwbyrne commented May 5, 2018

Proposed change submitted. This is a somewhat simpler design than what we've been discussing - it does the minimum required to give the application a reasonable say in how malformed requests are handled.

@jefferai
Copy link

jefferai commented Feb 5, 2019

Just to pile on here:

A user is adamant that not being able to supress this is a security vulnerability, because when they allow users to manually configure URLs, those users can then specify URLs that are not actually HTTP endpoints, leading to lines like net/http: HTTP/1.x transport connection broken: malformed HTTP response \"SSH-2.0-OpenSSH_7.4\". This allows the users that they have allowed to craft URLs to then craft URLs that disclose to them information about the local and/or remote systems.

The proposed change only seems to affect the http.Server. Addressing this user's concern would require being able to specify such a handler per-request and/or per-client and/or globally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@bradfitz @jefferai @bcmills @gopherbot @mjwbyrne