oci: cas*: add blob verification

[cyphar]

Previously we didn't really check that blobs matched their hashes
directly -- most of the checks we did were based on layer DiffIDs and
similar checks.

Here we add a new casext.GetVerifiedBlob() API which wraps the
underlying reader with a VerifiedReadCloser that will verify that the
digest matches on EOF (or on Close). Users should be very careful to
ensure that they actually check all errors (such as when using
ioutil.Discard or when they do Close).

In addition, we add it to the underlying oci/cas/dir implementation
(just for safety). This adds effectively no overhead since
VerifiedReadCloser will not double-up on verification if the digest is
the same. We also add a basic verification of this as an integration and
some unit tests -- so that we can have some confidence it actually works
to protect against bad blobs.

This is an improvement to the hardened VerifiedReadCloser, such that it
now also acts as a verified LimitedReader. First of all, it actually
means we now check the length of blobs according to their descriptors.
It also allows us to avoid reading further than strictly necessary, if
we already know that the read will fail. Unfortunately, the standard
cas/dir cannot really handle this new verification properly.

Fixes #278
Signed-off-by: Aleksa Sarai asarai@suse.de

[cyphar]

/cc @tych0 -- You should probably be aware of this change (since it improves overall security).

[cyphar]

LGTM.

[tych0]

Just for my own edification, what's the threat that this protects against that just using DiffIDs doesn't?

…

On Fri, Dec 28, 2018 at 6:51 AM Aleksa Sarai [see §317C(6)] < ***@***.***> wrote: Merged #282 <https://github.com/openSUSE/umoci/pull/282> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/openSUSE/umoci/pull/282#event-2047274371>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAv6147KXdG1TXGDdTTNrcnsZ-z_SLQ_ks5u9iHegaJpZM4ZjFCm> .

[cyphar]

A few things:

1. It checks the digests of everything (not just layer archives -- JSON blobs are included). DiffIDs are only for layer archives.

2. In the case of archives, it protects against having a different compressed archive that has the same uncompressed form (DiffIDs are the hash of the uncompressed payload -- meaning you have to uncompresss it in order to check it and also opens you up to archive bombs but that's only partially protected against here). This is somewhat more of a sanity thing, but not a bad thing to have.

3. It protects against endless-read (and thus DoS) attacks from blobs which are much longer than advertised. This is a bit of a half-point since they could in theory change the length of whatever descriptor you used too, but the point is that you won't read more bytes than the descriptor says. It also potentially protects against future collision attacks by requiring pre-images to have the same length.

[tych0]

On Fri, Dec 28, 2018 at 09:24:20AM -0800, Aleksa Sarai [see §317C(6)] wrote: A few things: 1. It checks the digests of everything (not just layer archives -- JSON blobs are included).

Oh, yeah, that does seem good :)

2. In the case of archives, it protects against having a different archive that has the same uncompressed form (DiffIDs are the hash of the uncompressed payload -- meaning you have to uncompresss it in order to check it and also opens you up to archive bombs but that's only partially protected against here).

Yeah, I have to be honest that I've never really understood the point of the DiffID vs. just hashing the layer itself. Anyway, thanks for the heads up.