pkg: hardening: disallow negative ExpectedSize #615
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cyphar
force-pushed
the
negative-expected-size
branch
2 times, most recently
from
7e5baeb to
349c9fe
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #615 +/- ##
==========================================
+ Coverage 73.68% 73.82% +0.14%
==========================================
Files 69 69
Lines 5556 5575 +19
==========================================
+ Hits 4094 4116 +22
+ Misses 1074 1072 -2
+ Partials 388 387 -1
🚀 New features to boost your workflow:
|
cyphar
force-pushed
the
negative-expected-size
branch
2 times, most recently
from
4d3bb5e to
2a9f248
Compare
The trailing data test relied on ExpectedSize < 0, but in actuality it would be better to test it with the true ExpectedSize. This is also needed because we will stop supporting ExpectedSize < 0 in a future patch. Signed-off-by: Aleksa Sarai <[email protected]>
In a future patch, ExpectedSize < 0 will no longer be supported by VerifiedReadCloser. However, this also gives us an opportunity to add a bit of extra hardening here -- if an attacker can write blobs to our store then they could in theory trigger a DoS by constantly writing more bytes (or expanding a zero section of a sparse file) if we do not have some hard limit. The current file size is as good a limit as any (and is going to be correct in all reasonable cases). This also lets us avoid double-hashing blobs in the common case where the blob size is correct (because then the VerifiedReadCloser returned by GetVerifiedBlob() will be a no-op). Signed-off-by: Aleksa Sarai <[email protected]>
cyphar
force-pushed
the
negative-expected-size
branch
from
2a9f248 to
714b09f
Compare
This was implicitly allowed by VerifiedReadCloser, and while we have closed that hole it's probably best to provide a more helpful error message. Not blocking this earlier was mostly due to a somewhat overly-permissive reading of the discussion in opencontainers/image-spec#153 which was finally clarified in opencontainers/image-spec#1285. Unknown sizes are a classic DoS vector, so allowing them (especially for descriptors where it makes little sense to have an unknown size) seems like a bad idea in general. Ref: opencontainers/image-spec#1285 Signed-off-by: Aleksa Sarai <[email protected]>
VerifiedReadCloser previously would allow for negative ExpectedSize to disable the size checking features added in commit ad66299 ("pkg: hardening: expand to verify descriptor length"). This was added partially because a somewhat overly-permissive reading of the discussion in opencontainers/image-spec#153 (which was finally clarified in opencontainers/image-spec#1285), but was also necessary for some users of VerifiedReadCloser that did not really know the proper blob size. We have now adjusted all of those callers, so there is no longer any reason to continue supporting this. Unknown sizes are a classic DoS vector, so allowing them seems like a bad idea in general. We might need to adjust this if/when umoci grows OCI distribution-spec support, but for now it isn't needed. Signed-off-by: Aleksa Sarai <[email protected]>
Previously we would return a non-nil &VerifiedReadCloser{} even if the
underlying GetBlob failed -- this is not really a great idea, as you can
get nil panics if you try to Close it.
Signed-off-by: Aleksa Sarai <[email protected]>
cyphar
force-pushed
the
negative-expected-size
branch
from
714b09f to
c5ac633
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
VerifiedReadCloser previously would allow for negative ExpectedSize to
disable the size checking features added in commit ad66299 ("pkg:
hardening: expand to verify descriptor length").
This was based on a somewhat overly-permissive reading of the discussion
in opencontainers/image-spec#153 which was finally clarified in
opencontainers/image-spec#1285. Unknown sizes are a classic DoS vector,
so allowing them seems like a bad idea in general.
Signed-off-by: Aleksa Sarai [email protected]