Repository navigation
vendor: github.com/containerd/containerd/v2 v2.4.2 - #7278
Conversation
github-actions
Bot
added
the
area/dependencies
|
Failure could be related; is it returning a special |
|
Failures were on Nydus and Stargz - pretty possible those are not tested in containerd upstream, so could be genuine. (restarted CI to see if it was a flake) |
| func Copy(ctx context.Context, cw Writer, or io.Reader, size int64, expected digest.Digest, opts ...Opt) error { | ||
| r := or | ||
| if size < 0 { | ||
| return fmt.Errorf("invalid content size %d: %w", size, errdefs.ErrInvalidArgument) | ||
| } |
There was a problem hiding this comment.
@samuelkarp Is there a reason the size is mandatory now? This is just a helper function between Reader and Writer. If you want to verify that nobody puts -1 inside Descriptor.Size in a JSON manifest, then I think that should happen at the level that deals with JSON manifests, not in this copy helper.
There was a problem hiding this comment.
interestingly; compose also failed on this patch (containerd/containerd@7f0c0b8); docker/compose#14339 (comment)
But looks like compose had a local mitigation for size-mismatch; https://github.com/docker/compose/blob/171f9d0767f7e0c43a3d2fa2949cde9ae3d679d8/internal/oci/resolver.go#L112-L120
// bound the read by the declared size so a rogue registry can't cause
// unbounded allocation; the extra byte detects oversized responses.
content, err := io.ReadAll(io.LimitReader(fetch, descriptor.Size+1))
if err != nil {
return nil, fmt.Errorf("reading blob %s: %w", descriptor.Digest, err)
}
if int64(len(content)) != descriptor.Size {
return nil, fmt.Errorf("blob %s size mismatch: expected %d bytes, got %d", descriptor.Digest, descriptor.Size, len(content))
}
There was a problem hiding this comment.
I think it's pretty reasonable for a copy helper that takes size as input to validate the size.
There was a problem hiding this comment.
So we should write our own helpers just to avoid this? If you want to add extra functionality into copy helper then it should be called CopyAndValidateSize(). Additionally, the next parameter in this function is expected, but it is NOT required and ignores empty value same way as size did before.
There was a problem hiding this comment.
@tonistiigi @samuelkarp wondering if something in between would work; explicitly make -1 a sentinel value, and reject other negative values.
For the advisory that was fixed; I ... somewhat assume that had validation as well before this utility is even hit (which would mean that -1 would be rejected earlier if it's coming from a remote response?
There was a problem hiding this comment.
The advisories that were fixed were all exploitable. We generally try not to issue spurious advisories.
There was a problem hiding this comment.
The advisories that were fixed were all exploitable. We generally try not to issue spurious advisories.
Oh, sorry, didn't mean to imply it wasn't a real issue, just was curious if there's checks before this utility is called (because the utility itself documents "unknown sizes" as supported, but validation of the manifest-size may be before that).
@tonistiigi @samuelkarp I opened a PR to address the change in behavior;
|
Hm.. it's not all of them this time; could it be some race? I can try once more. |
d8dfd9d to
4ece5fd
Compare
|
Pushed a temporary commit with the tip of v2.4 branch, without the changes from #7285 in Buildkit itself. |
26bdb13 to
d99b905
Compare
Security Updates: - [CVE-2026-95839](GHSA-vwhp-8p66-mq4f) - [GHSA-8647-2jxj-p2rc](GHSA-8647-2jxj-p2rc) - [GHSA-hc94-p972-ch66](GHSA-hc94-p972-ch66) containerd/containerd@v2.4.1...v2.4.2 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
d99b905 to
d9a55f2
Compare
Security Updates:
containerd/containerd@v2.4.1...v2.4.2