[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #26348 [Obfuscation/Snowflake]: Guard against large reads
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs