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: ReadResponse rejects duplicate Transfer-Encoding: chunked headers #44591

Open
andybalholm opened this issue Feb 24, 2021 · 10 comments
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@andybalholm
Copy link
Contributor

Since Go 1.15 (commit d5734d4), http.ReadResponse (and therefore http.Transport.RoundTrip) returns an error if the response has more than one Transfer-Encoding header.

This makes it impossible to download content from certain sites. For example, attempting to download from https://sis.cat.com/ returns the error too many transfer encodings: ["chunked" "chunked"]. (https://play.golang.org/p/3rJb7ClBQ_U)

If I understand the discussion around the change correctly, there are security issues related to multiple Transfer-Encoding headers on a request. But this is on a response.

Could we be a little less strict for responses? Or allow multiple Transfer-Encoding headers if they are all "chunked"?

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 24, 2021
@thinkwelltwd
Copy link

Here's another problematic URL:

https://onestop.md.gov

@networkimprov
Copy link

cc @fraenkel @bradfitz @neild @odeke-em

@fraenkel
Copy link
Contributor

fraenkel commented Mar 3, 2021

I did some further investigation.
The RFC is pretty clear on the matter but I was curious why curl and chrome didn't seem to care.

My testing included sending back multiple chunked headers on a single line as well as with multiple headers. I also sent back gzip, chunked variations which Go doesn't support.

Chrome didn't care what came back and always managed to print my response. They are just playing fast and loose to make the user experience appear to work in the face of all types of possible errors.

Curl didn't care how many chunked encodings were sent, but did fail when gzip, chunked didn't correctly encode the response body. Looking at their source code as well as some others, "chunked" is being scanned for and just marked as present. The additional encodings are then stacked. If one were to send chunked, gzip, curl doesn't care that its the wrong order.

Coalescing multiple chunked encodings won't hurt. The downside is that these broken configured servers are allowed to go on their merry way.

@thinkwelltwd
Copy link

The downside is that these broken configured servers are allowed to go on their merry way.

That is vexing.

If curl and Chrome need to "make the user experience appear to work", and if the user can't between something that works vs "appears to work", should we as users of the Go library take that to mean that the world's waiting on the Go http library to enforce the spec?

Or could there be some other way to compile the Go library to "appear to work" as well as Chrome / curl? 🥺

As it currently is, I've downgraded to 1.14.14 and that doesn't seem like the best of alternatives either.

@neild
Copy link
Contributor

neild commented Mar 21, 2021

@FiloSottile

Should we accept incorrectly duplicated "Transfer-Encoding: chunked" headers? This doesn't seem like it poses a request smuggling risk, although it does add a small amount of additional complexity to Transfer-Encoding parsing.

andybalholm added a commit to andybalholm/redwood that referenced this issue Apr 22, 2021
Copy and modify http.ReadResponse and its dependencies to accept
responses with multiple "Transfer-Encoding: chunked" headers.

golang/go#44591
@andybalholm
Copy link
Contributor Author

The modified version of parseTransferEncoding from my workaround (linked above) looks like this:

// parseTransferEncoding sets t.Chunked based on the Transfer-Encoding header.
func (t *transferReader) parseTransferEncoding() error {
	raw, present := t.Header["Transfer-Encoding"]
	if !present {
		return nil
	}
	delete(t.Header, "Transfer-Encoding")

	// Issue 12785; ignore Transfer-Encoding on HTTP/1.0 requests.
	if !t.protoAtLeast(1, 1) {
		return nil
	}

	// The only transfer encoding we support is "chunked". If it is specified
	// multiple times, it is treated the same as if it were specified only once.
	for _, v := range raw {
		if strings.ToLower(textproto.TrimString(v)) != "chunked" {
			return &unsupportedTEError{fmt.Sprintf("unsupported transfer encoding: %q", v)}
		}
	}

	// RFC 7230 3.3.2 says "A sender MUST NOT send a Content-Length header field
	// in any message that contains a Transfer-Encoding header field."
	//
	// but also: "If a message is received with both a Transfer-Encoding and a
	// Content-Length header field, the Transfer-Encoding overrides the
	// Content-Length. Such a message might indicate an attempt to perform
	// request smuggling (Section 9.5) or response splitting (Section 9.4) and
	// ought to be handled as an error. A sender MUST remove the received
	// Content-Length field prior to forwarding such a message downstream."
	//
	// Reportedly, these appear in the wild.
	delete(t.Header, "Content-Length")

	t.Chunked = true
	return nil
}

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@alsals126
Copy link

alsals126 commented Jan 11, 2023

I'm currently going through the same issue.

What if it's the response data not request?
because it's response data, I can't seem to use smuggling. So I'm wondering if there are other ideas.

And I wonder what is the progress on this issue?

@danp
Copy link
Contributor

danp commented Feb 19, 2023

Just ran into this. Would a change like @andybalholm's above be accepted to fix this? Based on all the given examples and what I'm seeing it would help.

@gopherbot
Copy link

Change https://go.dev/cl/493975 mentions this issue: net/http: allow multiple Transfer-Encoding headers in responses

@superleche
Copy link

After days of searching, technical reading and understanding that it is not a bug but that it should work that way, I have found a solution that does not involve installing a previous version or modifying the normal operation.

Option 1

req, _ := http.NewRequest("GET", url, nil)
req.Header.Set("Accept-Encoding","identity")
client:= & http.Client {}
resp, _ := client.Do(req)

Option 2

tr := &http.Transport {
     DisableCompression:true,
}
client := &http.Client {Transport:tr}
req, _ := http.NewRequest("GET", url, nil)
resp, _ := client.Do(req)

I found the solution and explanation at https://rarnu.xyz/archives/ktortogo. You will need to translate it first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

10 participants