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.

Sign up for GitHub

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: readRequest fails to handle URIs ending with space characters #63598

Open
colin969 opened this issue Oct 17, 2023 · 8 comments
Open

net/http: readRequest fails to handle URIs ending with space characters #63598

colin969 opened this issue Oct 17, 2023 · 8 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@colin969
Copy link

colin969 commented Oct 17, 2023 •
edited

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

$ go version
go version go1.20.2 windows/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\colin\AppData\Local\go-build
set GOENV=C:\Users\colin\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\colin\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\colin\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.2
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=J:\Data\Projects\fpProxy\go.mod
set GOWORK=J:\Data\Projects\fpProxy\go.work
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\colin\AppData\Local\Temp\go-build4117129570=/tmp/go-build -gno-record-gcc-switches

What did you do?

I work with particularly old software, which in some cases will send HTTP requests without stripping whitespace from the Request URI. After changing our proxy to be Go based, these requests just weren't being received by the proxy between the application and the file server.

When putting a Python or Fiddler proxy between the application and Go proxy server the requests would suddenly start working. The only idea I could come up with is that they're sanitizing the requests are they pass through.

I went digging and found that when reading the request, parseRequestLine works on the assumption that each element of the request line is seperated by a single space character, as defined in the RFC. When it splits, the HTTP proto gets an extra space before it. This means it'll error as a malformed proto. I know this behaviour isn't strictly supported by the RFC but I'd like to see it reflect how other software handles it.

The easiest way to test this is to send variants of GET /test HTTP/1.0 and GET /test HTTP/1.0 with something like telnet to a Go HTTP server, where the latter will return 400 Bad Request.

For demonstration just to display where the space prefixing the proto comes from, I took a copy of the current parseRequestLine and then fed the proto into ParseHTTPVersion

https://go.dev/play/p/8F2Q9A5Fhx1

What did you expect to see?

'GET', 'http://www.nabiscoworld.com/shock/nbgrm.dcr', 'HTTP/1.0'

What did you see instead?

'GET', 'http://www.nabiscoworld.com/shock/nbgrm.dcr', ' HTTP/1.0'
Malformed proto
@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 17, 2023
@seankhliao
Copy link
Member

cc @neild

@krum110487
Copy link

Ideally, it would be nice to optionally have the functionality like this, to allow them to pass through, but escaped.

https://go.dev/play/p/fkBi238Kgav

@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2023

I know this behaviour isn't strictly supported by the RFC but I'd like to see it reflect how other software handles it.

Generally Go's net/http tries to stick to the RFCs unless the deviations are used widely. Can you give some examples of widely-used clients that require this behavior, and perhaps more detail about why it cannot be fixed in those clients instead?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 18, 2023
@bcmills bcmills added this to the Unplanned milestone Oct 18, 2023
@colin969
Copy link
Author

It's a tricky one - It isn't commonly used and virtually any client in the past 15+ years shouldn't ever do this, but we're finding incorrect use of it in some Adobe Shockwave games, games closer to 20-25 years old.

The behavior is reflected in every other software I've tried - Fiddler, Python, Apache etc all handle it fine (in fact the http://go.dev/ site itself considers it valid). I've really struggled to find documentation for why they support it in the first place, the only thing I could dig up was this section of the URI Generic Syntax RFC from 1998 under 'Syntax Notation and Common Elements', but it was removed by the time the current RFC from 2005 appears.

<first>/<second>;<third>?<fourth>

The component names are enclosed in angle-brackets and any characters
outside angle-brackets are literal separators. Whitespace should be
ignored. These descriptions are used informally and do not define
the syntax requirements.

Obviously having this supported is something I'd like considering our use case, but I'm understanding of why you might also choose not to.

@krum110487
Copy link

krum110487 commented Oct 18, 2023 •
edited

Just to add some more samples of it working Using OpenSSL to test, go.dev worked with any number of spaces

All valid (Including spaces AFTER HTTP/1.1)
GET go.dev  HTTP/1.1
GET go.dev    HTTP/1.1
GET        go.dev      HTTP/1.1
GET   go.dev/play/       HTTP/1.1   
GET   https://www.google.com/imghp?hl=en&ogbl       HTTP/1.1
GET        https://learn.microsoft.com/en-us/    HTTP/1.1   
GET        https://httpd.apache.org/    HTTP/1.1   

Invalid (Returns 400)
  GET go.dev HTTP/1.1
GET go.dev/play / HTTP/1.1
GET go.dev/pl ay/ HTTP/1.1

I tried to find other examples of nginx and apache, but I couldn't get any request to work openssl with nginx.org or facebook.com, everything was rejected, I am sure I am doing something wrong.

All sites rejected anything starting with a space, but every site I tried that I could get to work normally worked with multiple spaces between Method and URI and URI and Protocol and Protocol to end.

@seankhliao seankhliao removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 15, 2023
@jskcnsl
Copy link

jskcnsl commented Jan 2, 2024

We use Golang to parse raw http request in cybersecurity, and there is an easy sql inject example, which can't be parsed by http.ReadRequest.

$ curl -vv "http://10.1.1.2:8010/?id=1 or 1=2"
*   Trying 10.1.1.2:8010...
* TCP_NODELAY set
* Connected to 10.1.1.2 (10.1.1.2) port 8010 (#0)
> GET /?id=1 or 1=2 HTTP/1.1
> Host: 10.1.1.2:8010
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Server: Test-Server
< Date: Tue, 02 Jan 2024 10:16:52 GMT
< Content-Length: 0
<

Of cause, this is not a Generally http request, but we want to use Go do more 😁

@seankhliao
Copy link
Member

cc @golang/security

sounds like we should reject making the parser any laxer

@jskcnsl
Copy link

jskcnsl commented Jan 2, 2024

cc @golang/security

sounds like we should reject making the parser any laxer

I mean we need more laxer to make Golang can be used to parse these no generally request. 😂
We want to get a key-value pair from http query "id" = "1 or 1=2".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@krum110487 @colin969 @bcmills @jskcnsl @seankhliao