Guard against large reads

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
	}

added anti-censorship-roadmap-2019 in Legacy / Trac component::circumvention/snowflake in Legacy / Trac easy in Legacy / Trac owner::cohosh in Legacy / Trac points::1 in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac severity::normal in Legacy / Trac sponsor::19 in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac labels

Trac:
Points: N/A to 1
Sponsor: N/A to Sponsor19

Trac:
Status: new to assigned
Owner: N/A to cohosh

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 legacy/trac#29259 (moved).

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.

Trac:
Status: assigned to needs_review

Trac:
Keywords: easy deleted, easy anti-censorship-roadmap-2019 added

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).

Trac:
Status: needs_review to needs_revision

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 legacy/trac#29259 (moved).

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.

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
}

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/

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to merge_ready

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

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

changed time estimate to 8h

moved from legacy/trac#26348 (moved)

added Bug First Contribution labels and removed 1 deleted label

removed 1 deleted label