Apply LimitedReader structs to all io.ReadAll calls. - #8834
Conversation
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.
| } 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.
There was a problem hiding this comment.
This is important and still incoming in additional commit(s).
Fixes #8823