Apply LimitedReader structs to all io.ReadAll calls.#8834
Conversation
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
High-level comments:
- Let's not bother in all the test files (definitely foo_test.go, and maybe test/blah at your discretion)
- Let's crank down the limit for most of these (100MB is too many)
- Let's crank up the limit specifically for anything reading CRLs, since those could exceed 100MB in a mass revocation event
- Maybe a
core.ReadAtMost(reader, limit)helper with accompanyingcore.DefaultMaxReadconst? Also up to you.
Reduce to still very generous ~300K normal reader size. Increase to ~1G CRL reader size.
|
This reduced set of changes gets us good coverage for production code that we can deploy with lower impact risks. And we still have the option to put up a second iteration with a core helper. |
There was a problem hiding this comment.
This change is looking pretty good! My big overarching comment is that io.LimitedReader is weird. On its face you would assume that a read over the maximum bytes would result in an error, but it just returns an EOF to io.ReadAll() and since io.ReadAll() correctly treats EOF as "the input ended" an oversized body is silently truncated.
This is problematic in a few cases. A lot of the time the bytes we're reading are used as input to the next step. Best case is that the next call detects this and some class of "x is malformed" error ends up in our logs. Not terrible but not a strong signal that someone is attempting to exploit us. The worse case is when the data itself is still valid when truncated (YAML, CSV, etc.), here, if we're extremely unlucky, we'll silently continue with bad data.
Instead, we should use this as a chance set up a tripwire. Write a new core package function; a limited reader that detects when the input is expected-max-size + 1 and returns an error that we can fail on.
| } | ||
|
|
||
| b, err := io.ReadAll(resp.Body) | ||
| b, err := io.ReadAll(&io.LimitedReader{R: resp.Body, N: 300_000}) |
There was a problem hiding this comment.
Per https://datatracker.ietf.org/doc/html/rfc8484#section-6, I think anything over 65_535 would be malformed.
| b, err := io.ReadAll(&io.LimitedReader{R: resp.Body, N: 300_000}) | |
| b, err := io.ReadAll(&io.LimitedReader{R: resp.Body, N: 65_535}) |
| validate.RegisterCustomTypeFunc(config.DurationCustomTypeFunc, config.Duration{}) | ||
|
|
||
| inBytes, err := io.ReadAll(in) | ||
| inBytes, err := io.ReadAll(&io.LimitedReader{R: in, N: 300_000}) |
There was a problem hiding this comment.
I'm struggling to see what the attack vector would be with loading one of our own configuration files. It's probably fine to keep this, but the interesting thought I had was that some of our configuration files are YAML and if you cut off YAML at the right boundary, it's still valid YAML. That said 300KB of ~20 char YAML would result in >14,000 lines.
| body, err := io.ReadAll(&io.LimitedReader{R: resp.Body, N: 1_000_000_000}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| // Truncate the response body in case it's too big to be useful in logs. | ||
| if len(body) > 400 { | ||
| body = body[:400] | ||
| } | ||
| return nil, fmt.Errorf("http status %d for %q: %s", resp.StatusCode, url, string(body)) | ||
| } | ||
|
|
||
| return body, nil |
There was a problem hiding this comment.
There's an interesting pattern here that we should probably fix. We're reading up to 1 GB response body into memory before we check the http status of the response and then truncating to dump it into text. Instead we should branch on the status: If !OK we can read a smaller number of bytes to dump as text into the error. If OK then we can read up to 1 GB.
| } | ||
|
|
||
| var respJSON oauthTokenResp | ||
| err = json.NewDecoder(resp.Body).Decode(&respJSON) |
There was a problem hiding this comment.
I believe this line is also an unbounded read from the response body. We'll have to replace the call to json.NewDecoder() with a limited read followed by a json.Unmarshal().
| } else { | ||
| defer prevObj.Body.Close() | ||
| prevBytes, err := io.ReadAll(prevObj.Body) | ||
| prevBytes, err := io.ReadAll(&io.LimitedReader{R: prevObj.Body, N: 1_000_000_000}) |
There was a problem hiding this comment.
Bounding the reader here and in the crl-checker above could lead to an interesting scenario. If we upload a CRL larger than 1 GB we can technically stop ourselves from reading a CRL that we uploaded. So I think the right call here would be:
-
Make this configurable for the CRL components so that if we ever step on this rake we can simply update our configuration instead of building and deploying a new version of Boulder, and
-
Do a check against the configurable maxSize value where we accumulate the CRL bytes (near
case *cspb.UploadCRLRequest_CrlChunk:). That way we don't silently upload a file we can't read only to find out about it later.
3 participants
Fixes #8823