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: Empty header names erroneously accepted #65244

Closed
kenballus opened this issue Jan 24, 2024 · 4 comments
Closed

net/http: Empty header names erroneously accepted #65244

kenballus opened this issue Jan 24, 2024 · 4 comments
Assignees
@panjf2000
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone
Go1.23

Comments

@kenballus
Copy link

kenballus commented Jan 24, 2024 •
edited

Go version

go version devel go1.23-b3acaa8230 Tue Jan 23 20:08:07 2024 +0000 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/root/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/root/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/app/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/app/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.23-b3acaa8230 Tue Jan 23 20:08:07 2024 +0000'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build965832195=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Started a web server using net/http, and sent a request that contained a header with an empty name. (for example, GET / HTTP/1.1\r\n: ignored\r\nHost: whatever\r\n\r\n)

What did you see happen?

The server ignored the offending header (It was not available through the Request.Header interface)

What did you expect to see?

A 400 response. This is what Apache, Nginx, H2O, Node (with llhttp built from main), Lighttpd, and most other popular HTTP implementations do.

There are two reasons to reject messages with empty headers:

  1. The standard says that header names must be nonempty:
  field-name     = token
  token          = 1*tchar
  tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                 / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
                 / DIGIT / ALPHA
                 ; any VCHAR, except delimiters
  1. Some servers treat \r\n:\r\n as equivalent to \r\n\r\n. Those servers will then see the end of the message body where net/http sees (and ignores) an empty header. This is a potential HTTP request smuggling vector.
@panjf2000
Copy link
Member

Thanks for the bug report, I'll take on this.

@panjf2000 panjf2000 self-assigned this Jan 24, 2024
@panjf2000 panjf2000 added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 24, 2024
@panjf2000 panjf2000 added this to the Go1.23 milestone Jan 24, 2024
@gopherbot
Copy link

Change https://go.dev/cl/558095 mentions this issue: net/textproto: reject HTTP requests with empty header keys

@panjf2000
Copy link
Member

panjf2000 commented Jan 24, 2024 •
edited

It turns out that we had handled this kind of case in a previous CL 11242. Although it didn't seem like a good idea to skip it and do nothing about it in that CL, we unfortunately might break some existing HTTP clients if we submit CL 558095.

Not sure how this issue is going to be tackled eventually: remain doing nothing about it or reject empty header keys along with breaking a tiny backward compatibility (in that case, we may need a proposal?). @neild

@panjf2000 panjf2000 added Security NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 24, 2024
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Jan 24, 2024
@kenballus
Copy link
Author

kenballus commented Jan 24, 2024 •
edited

This is very unlikely to break any existing clients, because all the other large HTTP implementations already reject requests containing empty keys, so any client producing empty header keys would be incompatible with nearly every web server out there.

@panjf2000 panjf2000 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 24, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@panjf2000 @ezz-no
net/textproto: reject HTTP requests with empty header keys
d7c3ec8 
According to RFC 7230, empty field names in HTTP header are invalid.
However, there are no specific instructions for developers to deal
with that kind of case in the specification. CL 11242 chose to skip
it and do nothing about it, which now seems like a bad idea because
it has led `net/http` to behave inconsistently with the most widely-used
HTTP implementations: Apache, Nginx, Node with llhttp, H2O, Lighttpd, etc.
in the case of empty header keys.

There is a very small chance that this CL will break a few existing HTTP clients.

Fixes golang#65244

Change-Id: Ie01e9a6693d27caea4d81d1539345cf42b225535
Reviewed-on: https://go-review.googlesource.com/c/go/+/558095
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@panjf2000 panjf2000

Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
3 participants
@panjf2000 @gopherbot @kenballus