Opened 13 months ago

Closed 8 weeks ago

#26348 closed defect (fixed)

Guard against large reads

Reported by: dcf Owned by: cohosh
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords: easy anti-censorship-roadmap-2019
Cc: dcf, arlolra Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor: Sponsor19

Description

Snowflake code calls ioutil.ReadAll from a socket/HTTP in many places in the code: 1 2 3 4 5.

These should all get an io.LimitReader or http.MaxBytesReader with a limit of 100 KB or so. Like this one:

	body, err := ioutil.ReadAll(http.MaxBytesReader(w, req.Body, 100000))
	if err != nil {
		http.Error(w, "Bad request.", http.StatusBadRequest)
		return
	}

Child Tickets

Change History (10)

comment:1 Changed 5 months ago by gaba

Points: 1
Sponsor: Sponsor19

comment:2 Changed 3 months ago by cohosh

Owner: set to cohosh
Status: newassigned

comment:3 Changed 3 months ago by cohosh

Status: assignedneeds_review

Here's a fix: https://github.com/cohosh/snowflake/compare/ticket26348

I wrote a test for one of the broker reads... writing tests for the others in a nice way would require a bit of a refactor because convey currently has the http request for /proxy and /client set.

There's also a test for the client. No tests for proxy-go right now because we have no tests for that... made a note on #29259.

I noticed https://github.com/cohosh/snowflake/blob/master/client/lib/lib_test.go#L28

type MockResponse struct{}

func (m *MockResponse) Read(p []byte) (int, error) {
	p = []byte(`{"type":"answer","sdp":"fake"}`)
	return 0, nil
}
func (m *MockResponse) Close() error { return nil }

is never used.

comment:4 Changed 2 months ago by gaba

Keywords: anti-censorship-roadmap-2019 added

comment:5 Changed 2 months ago by dcf

Status: needs_reviewneeds_revision

I think these changes do not belong in this ticket:

-               w.WriteHeader(http.StatusBadRequest)
+               http.Error(w, "", http.StatusBadRequest)

It seems like a reasonable change, but I feel it should be in a separate changeset. If this change was made to make the So(w.Body.String(), ShouldEqual, ...) tests work, I think a better solution is to remove those tests--I don't think the specific body text returned for HTTP errors matters for correctness.

I'm not sure about this, calling http.MaxBytesReader with a nil value for the http.ResponseWriter argument.

body, err := ioutil.ReadAll(http.MaxBytesReader(nil, resp.Body, 100000))

Looking at the implementation, it seems to work, but the documentation doesn't say anything about it. I've found myself in the exact same conundrum... I wish the standard library had something like io.LimitedReader that returns an error other than io.EOF when the limit is reached. The implementation of io.LimitedReader is simple, and all it needs is a change from EOF to ErrUnexpectedEOF or something--maybe it would be good to put such an interface in common/ and use that instead of http.MaxBytesReader when we don't have an http.ResponseWriter? An alternative, since MaxBytesReader is always called before a call to io.ReadAll, is to provide a separate limitedReadAll function that enforces the limit--it could be an io.ReadAll followed by a Read that expects to find EOF.

I think you found all the places where limiting the length of input is needed.

The limit of 100000 seems reasonable. I feel it ought to be a constant (doesn't have to be centrally defined, it can just be a constant with a comment in each of the 3 source files that need it).

comment:6 in reply to:  5 ; Changed 2 months ago by cohosh

Replying to dcf:

I think these changes do not belong in this ticket:

-               w.WriteHeader(http.StatusBadRequest)
+               http.Error(w, "", http.StatusBadRequest)

It seems like a reasonable change, but I feel it should be in a separate changeset. If this change was made to make the So(w.Body.String(), ShouldEqual, ...) tests work, I think a better solution is to remove those tests--I don't think the specific body text returned for HTTP errors matters for correctness.

I would agree with removing the tests, also hard-coded responses on the client side as defined here will be difficult to maintain and should probably be treated differently.

Maybe we can deal with these issues in separate tickets.. I'll add the test changes to #29259.

I'm not sure about this, calling http.MaxBytesReader with a nil value for the http.ResponseWriter argument.

body, err := ioutil.ReadAll(http.MaxBytesReader(nil, resp.Body, 100000))

Looking at the implementation, it seems to work, but the documentation doesn't say anything about it. I've found myself in the exact same conundrum... I wish the standard library had something like io.LimitedReader that returns an error other than io.EOF when the limit is reached. The implementation of io.LimitedReader is simple, and all it needs is a change from EOF to ErrUnexpectedEOF or something--maybe it would be good to put such an interface in common/ and use that instead of http.MaxBytesReader when we don't have an http.ResponseWriter?

Yeah, it seems like http.MaxBytesReader was originally intended for servers only and not clients, but then there's the following comment in the actual implementation:

// The server code and client code both use
// maxBytesReader. This "requestTooLarge" check is
// only used by the server code. To prevent binaries
// which only using the HTTP Client code (such as
// cmd/go) from also linking in the HTTP server, don't
// use a static type assertion to the server
// "*response" type. Check this interface instead

Still I think you're right that we should be uncomfortable with using something in a way that is not specified in the documentation.

An alternative, since MaxBytesReader is always called before a call to io.ReadAll, is to provide a separate limitedReadAll function that enforces the limit--it could be an io.ReadAll followed by a Read that expects to find EOF.

I'm not sure what you mean by this exactly. Do you mean call limitedReadAll instead of io.ReadAll? And then I'm not sure why we'd make a call to both io.ReadAll and Read...

I think you found all the places where limiting the length of input is needed.

The limit of 100000 seems reasonable. I feel it ought to be a constant (doesn't have to be centrally defined, it can just be a constant with a comment in each of the 3 source files that need it).

Cool, I can add that.

comment:7 in reply to:  6 Changed 2 months ago by dcf

Replying to cohosh:

An alternative, since MaxBytesReader is always called before a call to io.ReadAll, is to provide a separate limitedReadAll function that enforces the limit--it could be an io.ReadAll followed by a Read that expects to find EOF.

I'm not sure what you mean by this exactly. Do you mean call limitedReadAll instead of io.ReadAll? And then I'm not sure why we'd make a call to both io.ReadAll and Read...

Sorry, I mean like this. Actually the second call should be to io.ReadFull to avoid needing to handle the case where the underlying Reader returns (0, nil).

func limitedReadAll(r io.Reader, limit int64) ([]byte, error) {
	p, err := ioutil.ReadAll(io.LimitReader(r, limit))
	if err != nil {
		return p, err
	}
	// Another read to see whether the LimitedReader hit EOF or not.
	var tmp [1]byte
	_, err = io.ReadFull(r, tmp[:])
	if err == io.EOF {
		err = nil
	} else if err == nil {
		err = io.ErrUnexpectedEOF
	}
	return p, err
}

comment:8 Changed 2 months ago by cohosh

Status: needs_revisionneeds_review

Here's another go: https://github.com/cohosh/snowflake/compare/large_reads

The two copies of limitedRead are a bit smelly, but I'm not sure there's enough there that justifies a new shared library in common/

comment:9 Changed 2 months ago by dcf

Status: needs_reviewmerge_ready

Okay, this looks fine to me.

I thought of a potentially better way to write limitedRead: pass limit+1 bytes to io.LimitedReader, and then return an error if either io.ReadAll returns an error, or len(p) == limit+1. Basically, roll the second read of 1 byte into the original read. If that works and it looks better to you, that's good to merge as well.

comment:10 in reply to:  9 Changed 8 weeks ago by cohosh

Resolution: fixed
Status: merge_readyclosed

Replying to dcf:

Okay, this looks fine to me.

I thought of a potentially better way to write limitedRead: pass limit+1 bytes to io.LimitedReader, and then return an error if either io.ReadAll returns an error, or len(p) == limit+1. Basically, roll the second read of 1 byte into the original read. If that works and it looks better to you, that's good to merge as well.

Ah that looks a lot nicer. I went with this, and also made a small change to get the CI to pass: https://github.com/cohosh/snowflake/pull/2

It's been merged to master

Note: See TracTickets for help on using tickets.