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: make HTTPS server detect mistaken plaintext HTTP requests and reply with HTTP 400 #23689

Closed
kernle32dll opened this issue Feb 4, 2018 · 19 comments
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@kernle32dll
Copy link

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

go version go1.9 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOOS="linux"

Running on Ubuntu 16.04

What did you do?

https://play.golang.org/p/Vw4VokSXnYc

I migrated my project from http to https. As for https, everything is working perfectly fine (I changed the scheme, and the port remained the same). However, http still causes problems:

While https is handled fine, requesting the same address via http returns garbage and a http: TLS handshake error from [::1]:45548: tls: first record does not look like a TLS handshake output in the application.

Of course, TLS over http does not work. The inherit problem is that it is impossible to intervene in the process. E.g. implementing a redirect from http to https (note: same port), or at very least preventing the server from returning garbage. The only place to intervene is via an http.Handler, but the handler is never called due to the tls check erroring out way before.

What did you expect to see?

If the url in the playground is requested as https://localhost:9000/hello, The text hello is printed out. If the url in the playground is requested as http://localhost:9000/hello (note http), the connection is either refused (empty body and a proper status code) or redirected.

What did you see instead?

https://localhost:9000/hello works as expected, but http://localhost:9000/hello returns garbage (Note: Not even a proper status code is set!).

Proposed changes

I propose that the connection in above scenario should be refused properly (empty body and proper status code). Ideally, it should be possible for the user to intervene in the process, e.g. implement an https to http redirect.

@gbbr gbbr changed the title https server returns garbage on http (which cannot be prevented) net/http: https server returns garbage on http (which cannot be prevented) Feb 4, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2018

This isn't a Go question, really. You need to listen on two separate ports: one for HTTP and one for HTTPS.

You could in theory sniff the first few bytes of the connection and guess the protocol, but nobody does that because everybody uses 80 and 443, as those are the protocol defaults if no explicit port number is provided.

@bradfitz bradfitz closed this as completed Feb 4, 2018
@kernle32dll
Copy link
Author

@bradfitz I think you misunderstood the question. I don't want to implement a standard redirect e.g. from http:80 to https:443.

Please read the What did you expect to see? and What did you see instead? sections again: The problem is, that go responds with garbage, when using a TLS server and making an http (not: NOT https) call. I'm not aware of any server who behaves like this. Or in other terms - go shouldn't even respond at all, when doing an http call against a TLS server.

The problem is hidden by the fact that http and https have different standard ports (referring to the redirect on two ports). But the samples in the issue description detail how to get the mentioned garbage output.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2018

No, I think I understood. You're trying to speak HTTP to an HTTPS server/port. That is undefined. You should expect browsers and any other clients (curl, telnet, whatever) to get gibberish.

I'm not aware of any server who behaves like this

Basically every other HTTP(s) server? Which ones do not? If this has become the norm lately, I'd be very surprised and intrigued.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2018

Okay, so Apache has a nice error message for this nowadays:

$ curl -v http://apache.org:443/
*   Trying 140.211.11.105...
* TCP_NODELAY set
* Connected to apache.org (140.211.11.105) port 443 (#0)
> GET / HTTP/1.1
> Host: apache.org:443
> User-Agent: curl/7.52.1
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< Date: Sun, 04 Feb 2018 20:17:02 GMT
< Server: Apache/2.4.7 (Ubuntu)
< Content-Length: 362
< Connection: close
< Content-Type: text/html; charset=iso-8859-1
< 
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>400 Bad Request</title>
</head><body>
<h1>Bad Request</h1>
<p>Your browser sent a request that this server could not understand.<br />
Reason: You're speaking plain HTTP to an SSL-enabled server port.<br />
 Instead use the HTTPS scheme to access this URL, please.<br />
</p>
</body></html>
* Curl_http_done: called premature == 0
* Closing connection 0

But Microsoft's webserver doesn't:

$ curl -v http://www.microsoft.com:443/
*   Trying 2600:1409:12:3b8::356e...
* TCP_NODELAY set
* Connected to www.microsoft.com (2600:1409:12:3b8::356e) port 443 (#0)
> GET / HTTP/1.1
> Host: www.microsoft.com:443
> User-Agent: curl/7.52.1
> Accept: */*
> 
* Recv failure: Connection reset by peer
* Curl_http_done: called premature == 1
* stopped the pause stream!
* Closing connection 0
curl: (56) Recv failure: Connection reset by peer

And Google doesn't:

$ curl -v http://www.google.com:443/
*   Trying 2607:f8b0:4008:80b::2004...
* TCP_NODELAY set
* Connected to www.google.com (2607:f8b0:4008:80b::2004) port 443 (#0)
> GET / HTTP/1.1
> Host: www.google.com:443
> User-Agent: curl/7.52.1
> Accept: */*
> 
* Curl_http_done: called premature == 0
* Empty reply from server
* Connection #0 to host www.google.com left intact

Cloudflare (nginx?) does reply nicely too:

$ curl -v http://www.cloudflare.com:443/
*   Trying 2400:cb00:2048:1::c629:d7a2...
* TCP_NODELAY set
* Connected to www.cloudflare.com (2400:cb00:2048:1::c629:d7a2) port 443 (#0)
> GET / HTTP/1.1
> Host: www.cloudflare.com:443
> User-Agent: curl/7.52.1
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< Server: cloudflare-nginx
< Date: Sun, 04 Feb 2018 20:18:39 GMT
< Content-Type: text/html
< Content-Length: 275
< Connection: close
< CF-RAY: -
< 
<html>
<head><title>400 The plain HTTP request was sent to HTTPS port</title></head>
<body bgcolor="white">
<center><h1>400 Bad Request</h1></center>
<center>The plain HTTP request was sent to HTTPS port</center>
<hr><center>cloudflare-nginx</center>
</body>
</html>
* Curl_http_done: called premature == 0
* Closing connection 0

So, that's interesting. Apparently Apache and nginx do. I wonder what Node's built-in server does.

I'll consider this a feature request and reopen but it's not a priority for me.

@bradfitz bradfitz reopened this Feb 4, 2018
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. FeatureRequest labels Feb 4, 2018
@bradfitz bradfitz added this to the Unplanned milestone Feb 4, 2018
@bradfitz bradfitz changed the title net/http: https server returns garbage on http (which cannot be prevented) net/http: make HTTPS server detect mistaken plaintext HTTP requests and reply with HTTP 400 Feb 4, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2018

/cc @tombergan as FYI

@kernle32dll
Copy link
Author

Thanks for the comparison @bradfitz .

One other noteworthy thing is is that some of the pages (all but apache) redirect to the correct https url (in browser). I am opposed to this, but this is a subjective security point. Thus, I proposed for this to be interchangeable (e.g. make the developer decide how he wants to handle the improper request - as at the moment he can't).

Would it be possible to detect the problem case by looking at the URL of the request maybe? If it is readable (all but the hostname is encrypted on https - right?), it should be a rouge http request. Not sure if this works tho - just some food for thought.

The essential difference from the current go implementation to the others is the fact that they do send either an empty or neat (with status code and all) response, and then reset or otherwise terminate(?) the connection. Go goes the latter, but also sends the aforementioned garbage:

$ curl -v http://localhost:9000/hello
*   Trying ::1...
* Connected to localhost (::1) port 9000 (#0)
> GET /hello HTTP/1.1
> Host: localhost:9000
> User-Agent: curl/7.47.0
> Accept: */*
> 
�����
* Connection #0 to host localhost left intact

It would be probably fine to remove the garbage for a first fix, and then think about how to given developers control over the matter - my 2 cents.

@agnivade
Copy link
Contributor

agnivade commented Feb 6, 2018

I wonder what Node's built-in server does.

This -

curl http://localhost:8080/ -v 
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.47.0
> Accept: */*
> 
* Empty reply from server
* Connection #0 to host localhost left intact
curl: (52) Empty reply from server

So it seems this is the same approach as Google's servers. I like this empty reply approach. Should we implement this ?

@bradfitz
Copy link
Contributor

bradfitz commented Feb 6, 2018

I like this empty reply approach. Should we implement this ?

There is no meaningful difference between "reply with binary gibberish" and an "empty reply". The only question is whether we go out of our way to recognize when the wrong protocol is being spoken. If we do not (because no spec says you must, and many servers don't), then it's completely undefined what we do.

@agnivade
Copy link
Contributor

agnivade commented Feb 6, 2018

To me, giving an empty reply is certainly more cleaner than replying with gibberish. But if we are to detect the protocol and respond accordingly, we might as well respond with a proper message. Just my thoughts.

@oliwer
Copy link

oliwer commented Feb 12, 2018

Here, instead of just returning, we could send "HTTP/1.1 400 TLS Handshake failed" and close the connection. Is there a downside to this?

@bradfitz
Copy link
Contributor

@oliwer, there's no guarantee at that line that Handshake hasn't already written to the socket.

@oliwer
Copy link

oliwer commented Feb 15, 2018

@bradfitz you're right! My bad. I'm looking at how to inspect the request before or during the handshake but it is quite invasive.

@gopherbot
Copy link

Change https://golang.org/cl/98447 mentions this issue: net/http: make HTTPS server reject HTTP requests with HTTP 400

@m4ns0ur
Copy link
Contributor

m4ns0ur commented Mar 4, 2018

@bradfitz is any documentation update required for this?

@bradfitz
Copy link
Contributor

bradfitz commented Mar 5, 2018

No.

@bradfitz
Copy link
Contributor

bradfitz commented Mar 5, 2018

For the record, this was done for the client side of net/http in #11111 by @cespare. Notably, see 45d1c8a

The net/http server should look for that RecordHeaderError type.

@m4ns0ur
Copy link
Contributor

m4ns0ur commented Jun 13, 2018

@bradfitz #24401 is closed, so could you please review CL again (after release of course)?

@gopherbot
Copy link

Change https://golang.org/cl/143177 mentions this issue: net/http: make server nicely reject HTTP requests to HTTPS server

@gopherbot
Copy link

Change https://golang.org/cl/144517 mentions this issue: net/http: fix comment change omitted between versions of CL 143177

gopherbot pushed a commit that referenced this issue Oct 25, 2018
Updates #23689

Change-Id: Icddec2fcc39802cacd651a9c94290e86cf1e48d1
Reviewed-on: https://go-review.googlesource.com/c/144517
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants