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: 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
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@mraerino
Copy link
Contributor

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
@mraerino mraerino mentioned this issue Jun 24, 2020
Closed
@gopherbot
Copy link

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

@FiloSottile
Copy link
Contributor

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

@nightlyone
Copy link
Contributor

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

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 29, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Jun 29, 2020
@cagedmantis
Copy link
Contributor

/cc @bradfitz @bcmills @neild

@bradfitz
Copy link
Contributor

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

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

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

@gopherbot
Copy link

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.

@gopherbot gopherbot mentioned this issue Jun 29, 2020
Closed
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Jun 29, 2020
@FiloSottile
Copy link
Contributor

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
Contributor

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

@FiloSottile FiloSottile mentioned this issue Jun 29, 2020
Closed
@FiloSottile
Copy link
Contributor

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

@dmitshur dmitshur mentioned this issue Jun 29, 2020
Open
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 29, 2020
@gopherbot
Copy link

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
@mraerino @dmitshur
[release-branch.go1.13] net/http: fix panic with If-None-Match value …
e19af0f 
…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
@mraerino @dmitshur
[release-branch.go1.14] net/http: fix panic with If-None-Match value …
de0e241 
…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>
@golang golang locked and limited conversation to collaborators Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

Successfully merging a pull request may close this issue.

net/http: fix panic with If-None-Match value in http.ServeContent mraerino/go
8 participants
@bradfitz @cagedmantis @networkimprov @FiloSottile @nightlyone @dmitshur @mraerino @gopherbot