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

x/net/http2/h2c: ineffective mitigation for unsafe io.ReadAll #56352

Closed
howardjohn opened this issue Oct 20, 2022 · 6 comments
Closed

x/net/http2/h2c: ineffective mitigation for unsafe io.ReadAll #56352

howardjohn opened this issue Oct 20, 2022 · 6 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@howardjohn
Copy link
Contributor

howardjohn commented Oct 20, 2022

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

$ go version
1.19

Does this issue reproduce with the latest release?

Yes

What did you do?

In https://go.dev/cl/407454, an io.ReadAll(r.Body) call was added to the h2c handler. This is unsafe for untrusted inputs generally. The CL also adds a comment (https://go-review.googlesource.com/c/net/+/407454/4/http2/h2c/h2c.go) to use MaxBytesHandler if this is an issue.

There are two issues here:

  1. Not much that can be done now, but this essentially introduced a DOS vector into the http2 library without any release notes. While the comment is helpful, most users probably don't read the full diff of changes in core libraries like this. It would be nice to have more visibility into unsafe changes, or to make them opt-in, in the future.

  2. The mitigation provided isn't useable in many cases

Consider the following program:

package main

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"runtime"

	"golang.org/x/net/http2"
	"golang.org/x/net/http2/h2c"
)

func main() {
	h2s := &http2.Server{}

	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		PrintMemUsage()
		n, err := io.Copy(io.Discard, r.Body)
		fmt.Fprintf(w, "http: %v, res: %v/%v\n", r.Proto, n, err)
		log.Printf("http: %v, res: %v/%v\n", r.Proto, n, err)
		PrintMemUsage()
	})

	server := &http.Server{
		Addr:    "0.0.0.0:8888",
		Handler: h2c.NewHandler(handler, h2s),
	}

	fmt.Printf("Listening [0.0.0.0:8888]...\n")
	log.Println(server.ListenAndServe())
}
func PrintMemUsage() {
	var m runtime.MemStats
	runtime.ReadMemStats(&m)
	// For info on each, see: https://golang.org/pkg/runtime/#MemStats
	fmt.Printf("Alloc = %v MiB", bToMb(m.Alloc))
	fmt.Printf("\tTotalAlloc = %v MiB", bToMb(m.TotalAlloc))
	fmt.Printf("\tSys = %v MiB", bToMb(m.Sys))
	fmt.Printf("\tNumGC = %v\n", m.NumGC)
}

func bToMb(b uint64) uint64 {
	return b / 1024 / 1024
}

I then run the following curls:

curl -X POST -d @output.dat localhost:8888
curl -X POST -d @output.dat localhost:8888 --http2-prior-knowledge
curl -X POST -d @output.dat localhost:8888 --http2

Up until the final call, memory usage is ~0. Its only the final call that is an issue.

However, if I add the MaxBytesHandler, all of them are broken.

Its also not possible to just disable H2c upgrade and allow only prior knowledge or http/1.1.

One solution could be to try to make a MaxBytesHandlerForH2CUpdate, copying the isH2CUpgrade and hoping the internal logic never changes. Something like:

func MaxBytesHandlerForH2CUpdate(h http.Handler, n int64) http.Handler {
	mbh := http.MaxBytesHandler(h, n)
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if isH2CUpgrade(r.Header) {
			mbh.ServeHTTP(w, r)
		}
		h.ServeHTTP(w, r)
	})
}

func isH2CUpgrade(h http.Header) bool {
	return httpguts.HeaderValuesContainsToken(h[textproto.CanonicalMIMEHeaderKey("Upgrade")], "h2c") &&
		httpguts.HeaderValuesContainsToken(h[textproto.CanonicalMIMEHeaderKey("Connection")], "HTTP2-Settings")
}

However, I would expect that it is possible to use h2c in a secure manner without resorting to workarounds like this.

@neild neild self-assigned this Oct 20, 2022
@dmitshur dmitshur changed the title h2c: ineffective mitigation for unsafe io.ReadAll x/net/http2/h2c: ineffective mitigation for unsafe io.ReadAll Oct 20, 2022
@gopherbot gopherbot added this to the Unreleased milestone Oct 20, 2022
@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

@bcmills bcmills modified the milestones: Unreleased, Backlog Oct 21, 2022
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 21, 2022
@gopherbot
Copy link

Change https://go.dev/cl/447396 mentions this issue: http2/h2c: handle errors when reading HTTP/1 request body

howardjohn added a commit to howardjohn/istio that referenced this issue Nov 8, 2022
In the Go upstream http2 library, a DOS vector was introduced. The fix
for the DOS introduces a request smuggling vulnerability.
See golang/go#56352.

All of these are related to h2c upgrades; we don't actually care about
this - we only want HTTP/2 prior knowledge and HTTP/1.1.

This PR introduces a wrapper around the h2c library denying h2c
upgrades. It also enforces via linting that this is used.
istio-testing pushed a commit to istio/istio that referenced this issue Nov 8, 2022
In the Go upstream http2 library, a DOS vector was introduced. The fix
for the DOS introduces a request smuggling vulnerability.
See golang/go#56352.

All of these are related to h2c upgrades; we don't actually care about
this - we only want HTTP/2 prior knowledge and HTTP/1.1.

This PR introduces a wrapper around the h2c library denying h2c
upgrades. It also enforces via linting that this is used.
istio-testing pushed a commit to istio-testing/istio that referenced this issue Nov 9, 2022
In the Go upstream http2 library, a DOS vector was introduced. The fix
for the DOS introduces a request smuggling vulnerability.
See golang/go#56352.

All of these are related to h2c upgrades; we don't actually care about
this - we only want HTTP/2 prior knowledge and HTTP/1.1.

This PR introduces a wrapper around the h2c library denying h2c
upgrades. It also enforces via linting that this is used.
@neild
Copy link
Contributor

neild commented Nov 9, 2022

@gopherbot please open backport issues, this is a potential request smuggling vector

@gopherbot
Copy link

Backport issue(s) opened: #56675 (for 1.18), #56676 (for 1.19).

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

istio-testing added a commit to istio/istio that referenced this issue Nov 9, 2022
In the Go upstream http2 library, a DOS vector was introduced. The fix
for the DOS introduces a request smuggling vulnerability.
See golang/go#56352.

All of these are related to h2c upgrades; we don't actually care about
this - we only want HTTP/2 prior knowledge and HTTP/1.1.

This PR introduces a wrapper around the h2c library denying h2c
upgrades. It also enforces via linting that this is used.

Co-authored-by: John Howard <howardjohn@google.com>
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
When processing an HTTP/1 Upgrade: h2c request, detect errors reading
the request body and fail the request rather than passing off the
partially-read request to the HTTP/2 server.

Correctly handles the case where a MaxBytesHandler has limited the
size of the initial HTTP/1 request body.

Fixes golang/go#56352

Change-Id: I08d60953cea26961cffbab3094cc1b44236f4e37
Reviewed-on: https://go-review.googlesource.com/c/net/+/447396
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: John Howard <howardjohn@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
@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 Nov 16, 2022
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Nov 16, 2022
@neild neild added Security and removed NeedsFix The path to resolution is known, but the work has not been done. labels Nov 16, 2022
@neild
Copy link
Contributor

neild commented Nov 21, 2022

This is CVE-2022-41721.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 7, 2022
@DavidKorczynski
Copy link

This is CVE-2022-41721.

Is this public? As far as I can see it's still private -- is this intentional? https://cve.report/CVE-2022-41721

kfaseela pushed a commit to Nordix/istio that referenced this issue Jan 24, 2023
In the Go upstream http2 library, a DOS vector was introduced. The fix
for the DOS introduces a request smuggling vulnerability.
See golang/go#56352.

All of these are related to h2c upgrades; we don't actually care about
this - we only want HTTP/2 prior knowledge and HTTP/1.1.

This PR introduces a wrapper around the h2c library denying h2c
upgrades. It also enforces via linting that this is used.

Co-authored-by: John Howard <howardjohn@google.com>
istio-testing added a commit to istio/istio that referenced this issue Jan 24, 2023
In the Go upstream http2 library, a DOS vector was introduced. The fix
for the DOS introduces a request smuggling vulnerability.
See golang/go#56352.

All of these are related to h2c upgrades; we don't actually care about
this - we only want HTTP/2 prior knowledge and HTTP/1.1.

This PR introduces a wrapper around the h2c library denying h2c
upgrades. It also enforces via linting that this is used.

Co-authored-by: John Howard <howardjohn@google.com>

Co-authored-by: Istio Automation <istio-testing-bot@google.com>
Co-authored-by: John Howard <howardjohn@google.com>
@golang golang locked and limited conversation to collaborators Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

No branches or pull requests

7 participants