Skip to content

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

Closed
@howardjohn

Description

@howardjohn

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.

Metadata

Metadata

Assignees

Labels

FrozenDueToAgeNeedsFixThe path to resolution is known, but the work has not been done.Security

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions