Digest references to images don’t enforce that the digest matches

[mtrmac]

As discovered in osbuild/osbuild#1252 , if, somehow, a repo@sha256:digest is written in the Name field of a c/storage image (but the digest does not match any actual manifest of the image we have seen), Podman uses of that name will succeed.

Compare:

$ jq .[].names storage/overlay-images/images.json
[
  "some-kind-of-digest",
  "whatever-sha256-fff",
  "image@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
  "quay.io/fedora/fedora@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"
]
$ podman inspect quay.io/fedora/fedora@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff | head
# works

vs.

# skopeo inspect containers-storage:[overlay@…]quay.io/fedora/fedora@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
FATA[0000] Error retrieving manifest for image: locating item named "manifest-sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" for image with ID "7acd9e55e8752c8a64e8569ea3e2736ceb89bb4d099bae597d202ef882e103e6" (consider removing the image to resolve the issue): file does not exist

The cause seems to be that once https://github.com/containers/common/blob/18c4568e8ee051fd8ded1d9d47cfa0453b1d6a0c/libimage/runtime.go#L324 , finds a name match, it captures the image’s ID, and from that point on, all of libimage’s references to that image use the image ID, not the user’s input; so, c/image never sees a digested reference and never triggers the “ensure the manifest matches the digest” enforcement.

---

I’m not sure what, if anything, to do about this. In ordinary operation, repo@sha256:digest values end up in Image.Names only by a pull that does validate a digest, and we certainly don’t make any promises if the c/storage image store is manually modified.

Also, once an image is identified, referring to it using an immutable ID is clearly valuable; I don’t want to suggest at all that that should change.

Still, I think it’s at least worth discussing to see if we should do something else.

One possible option would be for lookupImageInLocalStorage to do something to explicitly trigger the digest enforcement code, maybe Reference().NewImage().Manifest(). But that would be an extra cost for little benefit for most users.

@vrothberg WDYT?

[mtrmac]

One possibility would be for lookupImageInLocalStorage, and the rest of libimage, to arrange to use ${users-input}@${image-ID}, e.g. example.com/foo:bar@abcdef… or example.com/foo@sha256:…@abcdef… .

That would (per storageReference.resolveImage) always use the provided c/storage image ID, so it would be as safe, with the difference that:

• the original reference would be visible to c/image, which would enforce digest matches
• c/image would also enforce that the image still contains the right repo in Image.Names, i.e. a concurrent untag of the image would break future references.

I’m not at all sure it is worth it at this point (and it’s somewhat possible that starting to enforce digest matches could break some existing code path).

[vrothberg]

Thanks for the nice summary, @mtrmac.

A fix that I have in mind would be to jump directly to the digest-matching-lookup when the input is digested. That may even speed up digested lookups.

BUT it looks like the digested name has been forcefully added to images.json, so it may be a reasonable assumption that the offender knew what they where doing. My personal take is that changing such internal and state is entirely unsupported and should be discouraged by all means.

Certainly, some changes to some code would help to detect some cases where internal data and state has been "corrupted" but I still see it as a corruption forced by the user. So osbuild/osbuild#1252 is worrying me.

[mtrmac]

Thanks for the nice summary, @mtrmac.

A fix that I have in mind would be to jump directly to the digest-matching-lookup when the input is digested.

Yes, that would fix this case where .Names are manually-manually-edited; OTOH it would fail similarly if BigDataDigests were edited as well (and we definitely don’t want to recompute all of BigDataDigests for all images).

That may even speed up digested lookups.

(BTW containers/common#1352 .)

---

BUT it looks like the digested name has been forcefully added to images.json, so it may be a reasonable assumption that the offender knew what they where doing. My personal take is that changing such internal and state is entirely unsupported and should be discouraged by all means.

I 100% agree, this is completely unsupported. I wanted to bring this up in case there were some costless way to avoid that — and also to double-check that this does not seem to allow bypassing any digest enforcement where users might rely on it happening.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[mtrmac]

It seems that we don’t have any trivially-cheap ideas to protect ourselves in this case of invalid writes, so let’s have the users doing those invalid writes deal with it as best they can…