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: still return the content on malformed MIME header line #21290

Open
joaolsilva opened this issue Aug 3, 2017 · 15 comments
Open

net/http: still return the content on malformed MIME header line #21290

joaolsilva opened this issue Aug 3, 2017 · 15 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@joaolsilva
Copy link

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8.3 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jsilva/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.8.3/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.8.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_n/q8bzrfk93ss0nf_0fbk2ns8c0000gn/T/go-build250479046=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Fetch "http://107-182-231-168.webnow.net.br/status2.xsl?mount=/98fm.aac" or a similar URL from another Icecast server using http.Client.

Source: https://gist.github.com/joaolsilva/d24b7bf5602f0d6615eb5d385fd0ffdc
https://play.golang.org/p/MiokU7tQgI

What did you expect to see?

The content of the URL

What did you see instead?

err: Get http://107-182-231-168.webnow.net.br/status2.xsl?mount=/98fm.aac: malformed MIME header line: Content-Disposition = attachment; filename=file.asc

The MIME header might indeed be malformed, but other http clients such as cURL, browsers, etc. work (they seem to ignore the malformed line) and return the content.

@ALTree ALTree changed the title malformed MIME header line: Content-Disposition = attachment; filename=file.asc net/http: still return the content on malformed MIME header Aug 3, 2017
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 3, 2017
@ALTree ALTree changed the title net/http: still return the content on malformed MIME header net/http: still return the content on malformed MIME header line Aug 3, 2017
@ALTree ALTree added this to the Go1.10 milestone Aug 3, 2017
@odeke-em
Copy link
Member

/cc @tombergan

@tombergan
Copy link
Contributor

tombergan commented Aug 14, 2017

Sigh.

On one hand, other browsers accepting this response is evidence that we should accept it as well.

On the other hand, the error comes from the net/textproto library and that "malformed MIME header line" error has been there since the initial implementation in 2010. The net/textproto library is widely used beyond net/http and I'm concerned that removing the error will break other users of net/textproto. There are many other uses of that library, including uses inside and outside the standard library. Also, this, although that ship has sailed.

This will require some thought.

Edit: Maybe this isn't so bad. I suppose we could add an IgnoreMalformedHeaderLines bool to textproto.Reader which defaults to false and is set to true by net/http.

@tombergan tombergan self-assigned this Aug 14, 2017
@rsc
Copy link
Contributor

rsc commented Nov 6, 2017

ping @tombergan. should we do this for Go 1.10?

@tombergan
Copy link
Contributor

We're talking about this on golang.org/cl/75350. I think we should fix it but I'll leave it up to Brad on that CL if it's too late for 1.10.

@tombergan
Copy link
Contributor

This is not going to happen for 1.10 because it's too late in the release cycle to resolve the open API questions.

@tombergan tombergan modified the milestones: Go1.10, Go1.11 Nov 9, 2017
@odeke-em
Copy link
Member

Hello there @joaolsilva thank you for this bug report!

So currently with Go tip(pre-Go1.11) I can't produce this bug, and I also tried with Go1.8 but no dice, so perhaps the server might have fixed the header?
However, I have made a simple self contained snippet https://play.golang.org/p/whxnvo9-ZlF or inlined below, that can be run end-to-end without relying on an external server and I have tweaked the various parts of the "Content-Disposition" header but can't produce the problem either

package main

import (
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
)

func main() {
    log.SetFlags(0)
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		switch r.URL.Query().Get("k") {
		case "":
			w.Header().Set("Content-Disposition", `attachment; filename="file.txt"`)
		case "unq":
			w.Header().Set("Content-Disposition", `attachment; filename=file.txt`)
		case "lq":
			w.Header().Set("Content-Disposition", `attachment; filename="file.txt`)
		case "rq":
			w.Header().Set("Content-Disposition", `attachment; filename=file.txt"`)
		}
		w.Write([]byte(`Hello, World!`))
	}))
	defer cst.Close()

	for _, sym := range []string{"", "unq", "lq", "rq"} {
		resp, err := http.Get(cst.URL + "?k=" + sym)
		if err != nil {
			log.Printf("Failed to perform plain get: %v", err)
			continue
		}
		blob, _ := ioutil.ReadAll(resp.Body)
		_ = resp.Body.Close()
		log.Printf("sym=%-5q blob:%q\n", sym, blob)
	}
}

which just gives me

$ go run repro.go 
sym=""    blob:"Hello, World!"
sym="unq" blob:"Hello, World!"
sym="lq"  blob:"Hello, World!"
sym="rq"  blob:"Hello, World!"

that is all the data successfully parsed. Could you please take a look and tweak as possible?

I also tried reverting some changes to net/textproto as per https://github.com/golang/go/commits/master/src/net/textproto/reader.go but hit a point where my Go recompilation was crashing so figured I'd make the repro and hand it over for someone else to take a look. Also /cc @bradfitz

@joaolsilva
Copy link
Author

The original URL fixed the header. The following URL still has the same error:

http://ruby.streamguys.com:8120/status2.xsl?mount=/live

err: Get http://ruby.streamguys.com:8120/status2.xsl?mount=/live: net/http: HTTP/1.x transport connection broken: malformed MIME header line: Content-Disposition = attachment; filename=file.asc

I'm able to reproduce this issue with both go version go1.10.3 darwin/amd64 and go version devel +75fdeaa801 Fri Jun 22 13:28:19 2018 +0000 darwin/amd64

The headers are:

curl -I -XGET 'http://ruby.streamguys.com:8120/status2.xsl?mount=/live'

HTTP/1.0 200 OK
Content-Type: text/plain
Content-Length: 207
Content-Disposition = attachment; filename=file.asc
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: Origin, Accept, X-Requested-With, Content-Type
Access-Control-Allow-Methods: GET, OPTIONS, HEAD

Please note the Content-Disposition line, and how it's missing the ':'

I've updated the gist at https://gist.github.com/joaolsilva/d24b7bf5602f0d6615eb5d385fd0ffdc with the new URL.

@bradfitz
Copy link
Contributor

I'm inclined to do nothing here.

That's a pretty blatant spec violation, even if browsers accept it. But browsers accepting too much has led to security problems in the past, so I'm not eager to go down that road.

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned Jun 22, 2018
@joaolsilva
Copy link
Author

@bradfitz , what can a Go program do to override this behavior and still use net/http? All other http libraries for Go that I've tried had significant issues (not thread-safe, etc.).

@odeke-em
Copy link
Member

The original URL fixed the header. The following URL still has the same error:

@joaolsilva thanks for the new URL. Perhaps might you have noticed in the self-contained repro in #21290 (comment)
screen shot 2018-06-22 at 1 44 33 pm
if possible, for posterity please update that code to reproduce your bug so that in the future when someone is trying to fix it they don't rely on a server that can fix its response as the first URL did.

@joaolsilva
Copy link
Author

if possible, for posterity please update that code to reproduce your bug so that in the future when someone is trying to fix it they don't rely on a server that can fix its response as the first URL did.

I can't (or at least I don't know how). The error originates from net/textproto/reader.go. The ReadMIMEHeaderfunction fails if it can't find a ':' in the header, however setting a header using w.Header().Set() will write well formed headers.

@odeke-em
Copy link
Member

Perfect, thank you for the prescription @joaolsilva! Here is the self contained repro for posterity

package main

import (
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
)

func main() {
	log.SetFlags(0)
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		conn, bufw, _ := w.(http.Hijacker).Hijack()
		conn.Write([]byte(
`HTTP/1.0 200 OK
Content-Length: 13
Content-Disposition = attachment; filename=file.asc


Hello, World!`))
                bufw.Flush()
                conn.Close()
	}))
	defer cst.Close()

	resp, err := http.Get(cst.URL)
	if err != nil {
		log.Fatalf("Failed to perform plain get: %v", err)
	}
	_, _ = ioutil.ReadAll(resp.Body)
	_ = resp.Body.Close()
}

@C-Sto
Copy link

C-Sto commented Dec 17, 2018

As @bradfitz noted, total spec violation - however it would be good to be able to explicitly allow mangled headers when working with misbehaving servers

@freswa
Copy link

freswa commented Oct 12, 2023

Wanted to add another use case to allow broken headers:
curl -D - 'https://identity.apple.com/pushcert/caservice/renew'

HTTP/1.1 200
Server: Apple
Date: Thu, 12 Oct 2023 12:29:23 GMT
Content-Length: 0
Connection: keep-alive
Set-Cookie: X_CSRF_COOKIE=b1e7b35d8ff41d2e330e555d2d90b107; expires=Thu, 12-Oct-23 13:29:23 GMT; domain=.apple.com; path=/; HttpOnly; secure
1;: mode=block
max-age=31536000;: includeSubdomains
Strict-Transport-Security: max-age=31536000; includeSubdomains
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
Host: identity.apple.com
X-Frame-Options: SAMEORIGIN

@leonklingele
Copy link
Contributor

I have created freswa/dovecot-xaps-daemon#36 which adds code to ignore known, malformed HTTP response headers such as the ones returned from Apple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

9 participants