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: panic on misformed If-None-Match Header with http.ServeContent #39817

Closed
mraerino opened this issue Jun 24, 2020 · 13 comments
Closed

net/http: panic on misformed If-None-Match Header with http.ServeContent #39817

mraerino opened this issue Jun 24, 2020 · 13 comments
Labels
Milestone

Comments

@mraerino
Copy link
Contributor

@mraerino mraerino commented Jun 24, 2020

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

1.14

Does this issue reproduce with the latest release?

Yep. Problematic code can still be found in master.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/marcus/Library/Caches/go-build"
GOENV="/Users/marcus/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=",github.com/netlify,github.com/netlify"
GOOS="darwin"
GOPATH="/Users/marcus/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org/,direct"
GOROOT="/usr/local/Cellar/go/1.14/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/marcus/src/netlify/netlify-server/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ps/ssczlsb94v77k6s49ks2_96w0000gn/T/go-build356338045=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

go run main.go

package main

import (
	"bytes"
	"fmt"
	"log"
	"net/http"
	"net/http/httptest"
	"time"
)

func main() {
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		pl := []byte("Hello World!")
		buf := bytes.NewReader(pl)
		http.ServeContent(w, r, "hello", time.Time{}, buf)
	})
	srv := httptest.NewServer(handler)
	defer srv.Close()

	fmt.Println(srv.URL)

	req, err := http.NewRequest(http.MethodGet, srv.URL, nil)
	if err != nil {
		log.Fatal(err)
	}

	req.Header.Set("If-None-Match", ",")

	resp, err := http.DefaultClient.Do(req)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Printf("%+v\n", resp)
}

What did you expect to see?

I expected it to print a response with a 200 status code

What did you see instead?

This panic:

2020/06/24 16:14:22 http: panic serving 127.0.0.1:53287: runtime error: index out of range [0] with length 0
goroutine 24 [running]:
net/http.(*conn).serve.func1(0xc0000bca00)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/server.go:1772 +0x139
panic(0x12ea760, 0xc0000c21c0)
        /usr/local/Cellar/go/1.14/libexec/src/runtime/panic.go:973 +0x396
net/http.checkIfNoneMatch(0x137b140, 0xc0000fa0e0, 0xc0000fe100, 0x0)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/fs.go:415 +0x336
net/http.checkPreconditions(0x137b140, 0xc0000fa0e0, 0xc0000fe100, 0x0, 0x0, 0x0, 0x490c800, 0x20300000000000, 0x4afffff)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/fs.go:522 +0x7d
net/http.serveContent(0x137b140, 0xc0000fa0e0, 0xc0000fe100, 0x130c852, 0x5, 0x0, 0x0, 0x0, 0xc000062b30, 0x1377bc0, ...)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/fs.go:184 +0xd9
net/http.ServeContent(0x137b140, 0xc0000fa0e0, 0xc0000fe100, 0x130c852, 0x5, 0x0, 0x0, 0x0, 0x1377bc0, 0xc0000a3110)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/fs.go:165 +0xd8
main.main.func1(0x137b140, 0xc0000fa0e0, 0xc0000fe100)
        /Users/marcus/src/netlify/netlify-server/repro/main.go:16 +0xef
net/http.HandlerFunc.ServeHTTP(0x1324018, 0x137b140, 0xc0000fa0e0, 0xc0000fe100)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/server.go:2012 +0x44
net/http.serverHandler.ServeHTTP(0xc0000fa000, 0x137b140, 0xc0000fa0e0, 0xc0000fe100)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/server.go:2807 +0xa3
net/http.(*conn).serve(0xc0000bca00, 0x137b700, 0xc000090300)
        /usr/local/Cellar/go/1.14/libexec/src/net/http/server.go:1895 +0x86c
created by net/http.(*Server).Serve
        /usr/local/Cellar/go/1.14/libexec/src/net/http/server.go:2933 +0x35c
2020/06/24 16:14:22 Get "http://127.0.0.1:53286": EOF
exit status 1

This seems to be the problematic code:
https://github.com/golang/go/blob/master/src/net/http/fs.go#L410-L419

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 24, 2020

Change https://golang.org/cl/239699 mentions this issue: net/http: fix panic with If-None-Match value in http.ServeContent

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 24, 2020

This is not a Denial of Service vulnerability, because the panic is recovered automatically, correct?

@nightlyone
Copy link
Contributor

@nightlyone nightlyone commented Jun 24, 2020

Since the panic is logged, it may lead to remote log flooding and Go http servers cannot defend against this. I believe this fact warrants a backport.

@mraerino
Copy link
Contributor Author

@mraerino mraerino commented Jun 24, 2020

This is not a Denial of Service vulnerability, because the panic is recovered automatically, correct?

I put some thought into this question, but i'm entirely unsure about the impact. if used inside a http.Handler the panic should be recovered, so not putting a service at risk.

Looking at the code the problem should be present down to Go 1.8 where the If-None-Match handling was introduced.

@cagedmantis cagedmantis added this to the Backlog milestone Jun 29, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jun 29, 2020

@gopherbot gopherbot closed this in ce81a8f Jun 29, 2020
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 29, 2020

Submitted to master. Sent Go 1.14 backport as https://go-review.googlesource.com/c/go/+/240343

@bradfitz bradfitz reopened this Jun 29, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 29, 2020

Change https://golang.org/cl/240343 mentions this issue: [release-branch.go1.14] net/http: fix panic with If-None-Match value in http.ServeContent

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 29, 2020

@gopherbot please backport to 1.14. CL at https://golang.org/cl/240343

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 29, 2020

Backport issue(s) opened: #39920 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 29, 2020

This probably should have requested a freeze exception (which I think should have been granted).

We don't keep the main issue open for backports, but use the gopherbot-created one. See http://golang.org/wiki/MinorReleases.

Why no backport to Go 1.13?

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 29, 2020

@gopherbot please backport to 1.13. CL at https://golang.org/cl/240343.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 29, 2020

Well that didn't work. Opened manually: #39925

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 11, 2020

Change https://golang.org/cl/242077 mentions this issue: [release-branch.go1.13] net/http: fix panic with If-None-Match value in http.ServeContent

gopherbot pushed a commit that referenced this issue Jul 13, 2020
…in http.ServeContent

Updates #39817.
Fixes #39925.

Change-Id: I79f2ad7c836a8a46569f603aca583fdd526d22dc
GitHub-Last-Rev: 5b88aad
GitHub-Pull-Request: #39821
Reviewed-on: https://go-review.googlesource.com/c/go/+/239699
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit ce81a8f)
Reviewed-on: https://go-review.googlesource.com/c/go/+/242077
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Marcus Weiner <marcus.weiner@gmail.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue Jul 13, 2020
…in http.ServeContent

Updates #39817.
Fixes #39920.

Change-Id: I79f2ad7c836a8a46569f603aca583fdd526d22dc
GitHub-Last-Rev: 5b88aad
GitHub-Pull-Request: #39821
Reviewed-on: https://go-review.googlesource.com/c/go/+/239699
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit ce81a8f)
Reviewed-on: https://go-review.googlesource.com/c/go/+/240343
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants
You can’t perform that action at this time.