Skip to content

/go

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: the server doesn't handle whitespace before the first header field correctly #22464

Open
crvv opened this Issue Oct 27, 2017 · 12 comments

Comments

Assignees
No one assigned
Labels
NeedsFix
Projects
None yet
Milestone
  Go1.13
6 participants
@crvv @ianlancetaylor @tombergan @bradfitz @gopherbot @andybons
@crvv
Copy link
Contributor

crvv commented Oct 27, 2017

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

go version devel +7a8e8b2f19 Fri Oct 27 05:47:09 2017 +0000 darwin/amd64
go version go1.9.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

darwin/amd64

What did you do?

Send a wrong HTTP request as
"GET / HTTP/1.1\r\n Host: host\r\n\r\n". A right request should be
"GET / HTTP/1.1\r\nHost: host\r\n\r\n"

The Host header should not be parsed according to RFC7230.
And the request may be rejected.

A recipient that receives whitespace between the
start-line and the first header field MUST either reject the message
as invalid or consume each whitespace-preceded line without further
processing of it (i.e., ignore the entire line, along with any
subsequent lines preceded by whitespace, until a properly formed
header field is received or the header section is terminated)

A runnable program is

package main

import (
	"io"
	"log"
	"net"
	"net/http"
	"os"
	"time"
)

func server() {
	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
                log.Println(r.Host)
	})
	go func() {
		log.Fatal(http.ListenAndServe("localhost:8000", nil))
	}()
}

func request() {
	conn, err := net.Dial("tcp", "localhost:8000")
	if err != nil {
		panic(err)
	}
	_, err = conn.Write([]byte("GET / HTTP/1.1\r\n Host: host\r\n\r\n"))
	if err != nil {
		panic(err)
	}
	go func() {
		<-time.After(time.Millisecond * 100)
		conn.Close()
	}()
	io.Copy(os.Stdout, conn)
}
func main() {
	server()
	request()
}

What did you expect to see?

The request fails because of missing Host header or the whitespace .
The response from Nginx is

HTTP/1.1 400 Bad Request
Server: nginx/1.13.3
Date: Fri, 27 Oct 2017 07:29:15 GMT
Content-Type: text/html
Content-Length: 173
Connection: close

<html>
<head><title>400 Bad Request</title></head>
<body bgcolor="white">
<center><h1>400 Bad Request</h1></center>
<hr><center>nginx/1.13.3</center>
</body>
</html>

What did you see instead?

A successful request.

2017/10/27 15:35:36 host
HTTP/1.1 200 OK
Date: Fri, 27 Oct 2017 07:35:36 GMT
Content-Length: 0
Content-Type: text/plain; charset=utf-8
@ianlancetaylor

This comment has been minimized.

Sign in to view
Copy link
Contributor

ianlancetaylor commented Oct 27, 2017

CC @tombergan

@ianlancetaylor ianlancetaylor added the NeedsInvestigation label Oct 27, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Oct 27, 2017

@tombergan

This comment has been minimized.

Sign in to view
Copy link
Contributor

tombergan commented Oct 27, 2017

Thanks for the report. I will have to do a bit more code archaeology to see if this was intentional. Per this comment, whitespace at the end of a key is intentionally stripped despite being forbidden by the RFC. It's unclear so far if whitespace at the beginning of the first key is intentionally ignored, or if that was an oversight.
@bradfitz

This comment has been minimized.

Sign in to view
Copy link
Member

bradfitz commented Nov 2, 2017

The trailing whitespace was intentional, as seen from this test from 31e9429 from a security camera I used to have:

// Test that we read slightly-bogus MIME headers seen in the wild,                                                                                 
// with spaces before colons, and spaces in keys.                                                                                                  
func TestReadMIMEHeaderNonCompliant(t *testing.T) {
        // Invalid HTTP response header as sent by an Axis security                                                                                
        // camera: (this is handled by IE, Firefox, Chrome, curl, etc.)                                                                            
        r := reader("Foo: bar\r\n" +
                "Content-Language: en\r\n" +
                "SID : 0\r\n" +
                "Audio Mode : None\r\n" +
                "Privilege : 127\r\n\r\n")
        m, err := r.ReadMIMEHeader()
        want := MIMEHeader{
                "Foo":              {"bar"},
                "Content-Language": {"en"},
                "Sid":              {"0"},
                "Audio Mode":       {"None"},
                "Privilege":        {"127"},
        }
        if !reflect.DeepEqual(m, want) || err != nil {
                t.Fatalf("ReadMIMEHeader =\n%v, %v; want:\n%v", m, err, want)
        }
}

I know of no reason to accept leading whitespace, though, and I'm surprised we do.

@bradfitz bradfitz added the NeedsFix label Nov 2, 2017

@gopherbot gopherbot removed the NeedsInvestigation label Nov 2, 2017

@gopherbot

This comment has been minimized.

Sign in to view
Copy link

gopherbot commented Nov 2, 2017

Change https://golang.org/cl/75350 mentions this issue: net/textproto: retain leading whitespace in the first header key

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 10, 2017

gopherbot pushed a commit that referenced this issue Nov 10, 2017

@crvv @bradfitz
net/textproto: ignore initial lines with leading whitespaces in ReadM… 
…IMEHeader

A header line with leading whitespaces is not valid in HTTP as per
RFC7230. This change ignores these invalid lines in ReadMIMEHeader.

Updates #22464

Change-Id: Iff9f00380d28a9617a55ff7888a76fba82001402
Reviewed-on: https://go-review.googlesource.com/75350
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
5d0cab0

@tombergan tombergan self-assigned this Nov 27, 2017

@tombergan tombergan modified the milestones: Go1.11, Go1.10 Nov 27, 2017

@tombergan

This comment has been minimized.

Sign in to view
Copy link
Contributor

tombergan commented Nov 27, 2017

We fixed the parser to ignore headers like this:

GET / HTTP/1.1
  Host: foo.com
Accept-Encoding: gzip

However, headers like the following used to return an error, but don't anymore (note the lack of colon in the second line):

GET / HTTP/1.1
  Host foo.com
Accept-Encoding: gzip
@tombergan

This comment has been minimized.

Sign in to view
Copy link
Contributor

tombergan commented Nov 27, 2017

For Go 1.10, I will fix the above issue. For Go 1.11, we will reconsider if textproto.Reader needs multiple modes to more precisely handle the various RFCs it implements and to allow the caller to control whether we ignore invalid header lines or fail loudly.
@bradfitz

This comment has been minimized.

Sign in to view
Copy link
Member

bradfitz commented Nov 27, 2017

I'd be in favor of reverting the net/textproto changes entirely for Go 1.10 and instead writing the few line header parser in net/http that net/http needs (also for Go 1.10).

All this effort working across two packages and differing needs isn't worth it.
@tombergan

This comment has been minimized.

Sign in to view
Copy link
Contributor

tombergan commented Nov 27, 2017

I'd be in favor of reverting the net/textproto changes entirely for Go 1.10 and instead writing the few line header parser in net/http that net/http needs (also for Go 1.10).

I agree that net/texproto is doing too much, but I don't think that's a complete solution. It solves the problem for net/http but no one else. net/textproto is still wrong: it should not accept headers where the first line begins with a space. This holds for both RFC 5322 (smtp) and RFC 7230 (http). And, there are people outside net/http using net/textproto to parse HTTP headers (I know of one fairly major use inside Google). Those parsers will be broken if we don't fix net/textproto.
@gopherbot

This comment has been minimized.

Sign in to view
Copy link

gopherbot commented Nov 27, 2017

Change https://golang.org/cl/80055 mentions this issue: net/textproto: reject all headers with a leading space

gopherbot pushed a commit that referenced this issue Nov 27, 2017

@tombergan
net/textproto: reject all headers with a leading space 
Previously, golang.org/cl/75350 updated ReadMIMEHeader to ignore the
first header line when it begins with a leading space, as in the
following example:

GET / HTTP/1.1
  Host: foo.com
Accept-Encoding: gzip

However, golang.org/cl/75350 changed ReadMIMEHeader's behavior for the
following example: before the CL it returned an error, but after the
CL it ignored the first line.

GET / HTTP/1.1
  Host foo.com
Accept-Encoding: gzip

This change updates ReadMIMEHeader to always fail when the first header
line starts with a space. During the discussion for golang.org/cl/75350,
we realized we had three competing needs:

1. HTTP clients should accept malformed response headers when possible
   (ignoring the malformed lines).

2. HTTP servers should reject all malformed request headers.

3. The net/textproto package is used by multiple protocols (most notably,
   HTTP and SMTP) which have slightly different parsing semantics. This
   complicates changes to net/textproto.

We weren't sure how to best fix net/textproto without an API change, but
it is too late for API changes in Go 1.10. We decided to ignore initial
lines that begin with spaces, thinking that would have the least impact on
existing users -- malformed headers would continue to parse, but the
initial lines would be ignored. Instead, golang.org/cl/75350 actually
changed ReadMIMEHeader to succeed in cases where it previously failed
(as in the above example).

Reconsidering the above two examples, there does not seem to be a good
argument to silently ignore ` Host: foo.com` but fail on ` Host foo.com`.
Hence, this change fails for *all* headers where the initial line begins
with a space.

Updates #22464

Change-Id: I68d3d190489c350b0bc1549735bf6593fe11a94c
Reviewed-on: https://go-review.googlesource.com/80055
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
1c69384
@bradfitz

This comment has been minimized.

Sign in to view
Copy link
Member

bradfitz commented Nov 27, 2017

@tombergan, what remains here?

@tombergan tombergan modified the milestones: Go1.10, Go1.11 Nov 27, 2017

@tombergan tombergan removed their assignment Nov 27, 2017

@tombergan

This comment has been minimized.

Sign in to view
Copy link
Contributor

tombergan commented Nov 27, 2017

Nothing remains for 1.10. What remains is deciding whether or not to make a larger change for Go 1.11, which could include one or both of:

  • Copy ReadMIMEHeader into a private implementation of net/http and fine tune the behavior for HTTP clients and servers. Perhaps also do the same for net/mail and net/smtp.

  • Add a "mode" enum to textproto.Reader that would add loose/strict modes and specialize ReadMIMEHeader for each RFC. e.g., ModeSMTP, ModeLooseHTTP, ModeStrictHTTP.
@bradfitz

This comment has been minimized.

Sign in to view
Copy link
Member

bradfitz commented Nov 27, 2017

SGTM. Thanks for the milestone update.

@bcmills bcmills referenced this issue Apr 2, 2018

Closed

net/textproto: ReadLineBytes() returns strange []byte #24642

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 9, 2018

@andybons andybons removed this from the Go1.12 milestone Feb 12, 2019

@andybons andybons added this to the Go1.13 milestone Feb 12, 2019

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