[tor-bugs] #26348 [Obfuscation/Snowflake]: Guard against large reads

[Tor Bug Tracker & Wiki]

#26348: Guard against large reads
-----------------------------------------------+---------------------------
 Reporter:  dcf                                |          Owner:  cohosh
     Type:  defect                             |         Status:
                                               |  needs_revision
 Priority:  Medium                             |      Milestone:
Component:  Obfuscation/Snowflake              |        Version:
 Severity:  Normal                             |     Resolution:
 Keywords:  easy anti-censorship-roadmap-2019  |  Actual Points:
Parent ID:                                     |         Points:  1
 Reviewer:                                     |        Sponsor:  Sponsor19
-----------------------------------------------+---------------------------

Comment (by cohosh):

 Replying to [comment:5 dcf]:
 > I think these changes do not belong in this ticket:
 > {{{#!diff
 > -             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
 [https://github.com/cohosh/snowflake/blob/master/client/lib/rendezvous.go#L28
 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))
 > }}}
 >
 [https://github.com/golang/go/blob/go1.12.5/src/net/http/request.go#L1101-L1113
 Looking at the implementation], it seems to work, but
 [https://golang.org/pkg/net/http/#MaxBytesReader 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
 [https://golang.org/pkg/io/#LimitedReader io.LimitedReader] that returns
 an error other than `io.EOF` when the limit is reached. The
 [https://golang.org/pkg/io/#LimitedReader 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26348#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online