[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
-----------------------------------------------+---------------------------
Changes (by dcf):

 * status:  needs_review => needs_revision


Comment:

 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'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`? 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).

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