Skip to content

Commit

Permalink
Merge pull request #2418 from mtrmac/digest-unmarshal-5.29
Browse files Browse the repository at this point in the history
[release-5.29] Fix CVE-2024-3727
  • Loading branch information
mtrmac authored May 16, 2024
2 parents 534068f + 6e25805 commit e894804
Show file tree
Hide file tree
Showing 25 changed files with 371 additions and 124 deletions.
7 changes: 5 additions & 2 deletions copy/progress_bars.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,13 @@ type progressBar struct {
// As a convention, most users of progress bars should call mark100PercentComplete on full success;
// by convention, we don't leave progress bars in partial state when fully done
// (even if we copied much less data than anticipated).
func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.BlobInfo, kind string, onComplete string) *progressBar {
func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.BlobInfo, kind string, onComplete string) (*progressBar, error) {
// shortDigestLen is the length of the digest used for blobs.
const shortDigestLen = 12

if err := info.Digest.Validate(); err != nil { // digest.Digest.Encoded() panics on failure, so validate explicitly.
return nil, err
}
prefix := fmt.Sprintf("Copying %s %s", kind, info.Digest.Encoded())
// Truncate the prefix (chopping of some part of the digest) to make all progress bars aligned in a column.
maxPrefixLen := len("Copying blob ") + shortDigestLen
Expand Down Expand Up @@ -104,7 +107,7 @@ func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.
return &progressBar{
Bar: bar,
originalSize: info.Size,
}
}, nil
}

// printCopyInfo prints a "Copying ..." message on the copier if the output is
Expand Down
39 changes: 29 additions & 10 deletions copy/single.go
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,10 @@ func (ic *imageCopier) copyConfig(ctx context.Context, src types.Image) error {
destInfo, err := func() (types.BlobInfo, error) { // A scope for defer
progressPool := ic.c.newProgressPool()
defer progressPool.Wait()
bar := ic.c.createProgressBar(progressPool, false, srcInfo, "config", "done")
bar, err := ic.c.createProgressBar(progressPool, false, srcInfo, "config", "done")
if err != nil {
return types.BlobInfo{}, err
}
defer bar.Abort(false)
ic.c.printCopyInfo("config", srcInfo)

Expand Down Expand Up @@ -707,11 +710,17 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
}
if reused {
logrus.Debugf("Skipping blob %s (already present):", srcInfo.Digest)
func() { // A scope for defer
bar := ic.c.createProgressBar(pool, false, types.BlobInfo{Digest: reusedBlob.Digest, Size: 0}, "blob", "skipped: already exists")
if err := func() error { // A scope for defer
bar, err := ic.c.createProgressBar(pool, false, types.BlobInfo{Digest: reusedBlob.Digest, Size: 0}, "blob", "skipped: already exists")
if err != nil {
return err
}
defer bar.Abort(false)
bar.mark100PercentComplete()
}()
return nil
}(); err != nil {
return types.BlobInfo{}, "", err
}

// Throw an event that the layer has been skipped
if ic.c.options.Progress != nil && ic.c.options.ProgressInterval > 0 {
Expand All @@ -730,8 +739,11 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
// Attempt a partial only when the source allows to retrieve a blob partially and
// the destination has support for it.
if canAvoidProcessingCompleteLayer && ic.c.rawSource.SupportsGetBlobAt() && ic.c.dest.SupportsPutBlobPartial() {
if reused, blobInfo := func() (bool, types.BlobInfo) { // A scope for defer
bar := ic.c.createProgressBar(pool, true, srcInfo, "blob", "done")
reused, blobInfo, err := func() (bool, types.BlobInfo, error) { // A scope for defer
bar, err := ic.c.createProgressBar(pool, true, srcInfo, "blob", "done")
if err != nil {
return false, types.BlobInfo{}, err
}
hideProgressBar := true
defer func() { // Note that this is not the same as defer bar.Abort(hideProgressBar); we need hideProgressBar to be evaluated lazily.
bar.Abort(hideProgressBar)
Expand All @@ -751,18 +763,25 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
bar.mark100PercentComplete()
hideProgressBar = false
logrus.Debugf("Retrieved partial blob %v", srcInfo.Digest)
return true, updatedBlobInfoFromUpload(srcInfo, uploadedBlob)
return true, updatedBlobInfoFromUpload(srcInfo, uploadedBlob), nil
}
logrus.Debugf("Failed to retrieve partial blob: %v", err)
return false, types.BlobInfo{}
}(); reused {
return false, types.BlobInfo{}, nil
}()
if err != nil {
return types.BlobInfo{}, "", err
}
if reused {
return blobInfo, cachedDiffID, nil
}
}

// Fallback: copy the layer, computing the diffID if we need to do so
return func() (types.BlobInfo, digest.Digest, error) { // A scope for defer
bar := ic.c.createProgressBar(pool, false, srcInfo, "blob", "done")
bar, err := ic.c.createProgressBar(pool, false, srcInfo, "blob", "done")
if err != nil {
return types.BlobInfo{}, "", err
}
defer bar.Abort(false)

srcStream, srcBlobSize, err := ic.c.rawSource.GetBlob(ctx, srcInfo, ic.c.blobInfoCache)
Expand Down
22 changes: 18 additions & 4 deletions directory/directory_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,10 @@ func (d *dirImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
}
}

blobPath := d.ref.layerPath(blobDigest)
blobPath, err := d.ref.layerPath(blobDigest)
if err != nil {
return private.UploadedBlob{}, err
}
// need to explicitly close the file, since a rename won't otherwise not work on Windows
blobFile.Close()
explicitClosed = true
Expand All @@ -196,7 +199,10 @@ func (d *dirImageDestination) TryReusingBlobWithOptions(ctx context.Context, inf
if info.Digest == "" {
return false, private.ReusedBlob{}, fmt.Errorf("Can not check for a blob with unknown digest")
}
blobPath := d.ref.layerPath(info.Digest)
blobPath, err := d.ref.layerPath(info.Digest)
if err != nil {
return false, private.ReusedBlob{}, err
}
finfo, err := os.Stat(blobPath)
if err != nil && os.IsNotExist(err) {
return false, private.ReusedBlob{}, nil
Expand All @@ -216,7 +222,11 @@ func (d *dirImageDestination) TryReusingBlobWithOptions(ctx context.Context, inf
// If the destination is in principle available, refuses this manifest type (e.g. it does not recognize the schema),
// but may accept a different manifest type, the returned error must be an ManifestTypeRejectedError.
func (d *dirImageDestination) PutManifest(ctx context.Context, manifest []byte, instanceDigest *digest.Digest) error {
return os.WriteFile(d.ref.manifestPath(instanceDigest), manifest, 0644)
path, err := d.ref.manifestPath(instanceDigest)
if err != nil {
return err
}
return os.WriteFile(path, manifest, 0644)
}

// PutSignaturesWithFormat writes a set of signatures to the destination.
Expand All @@ -229,7 +239,11 @@ func (d *dirImageDestination) PutSignaturesWithFormat(ctx context.Context, signa
if err != nil {
return err
}
if err := os.WriteFile(d.ref.signaturePath(i, instanceDigest), blob, 0644); err != nil {
path, err := d.ref.signaturePath(i, instanceDigest)
if err != nil {
return err
}
if err := os.WriteFile(path, blob, 0644); err != nil {
return err
}
}
Expand Down
17 changes: 14 additions & 3 deletions directory/directory_src.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ func (s *dirImageSource) Close() error {
// If instanceDigest is not nil, it contains a digest of the specific manifest instance to retrieve (when the primary manifest is a manifest list);
// this never happens if the primary manifest is not a manifest list (e.g. if the source never returns manifest lists).
func (s *dirImageSource) GetManifest(ctx context.Context, instanceDigest *digest.Digest) ([]byte, string, error) {
m, err := os.ReadFile(s.ref.manifestPath(instanceDigest))
path, err := s.ref.manifestPath(instanceDigest)
if err != nil {
return nil, "", err
}
m, err := os.ReadFile(path)
if err != nil {
return nil, "", err
}
Expand All @@ -66,7 +70,11 @@ func (s *dirImageSource) GetManifest(ctx context.Context, instanceDigest *digest
// The Digest field in BlobInfo is guaranteed to be provided, Size may be -1 and MediaType may be optionally provided.
// May update BlobInfoCache, preferably after it knows for certain that a blob truly exists at a specific location.
func (s *dirImageSource) GetBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache) (io.ReadCloser, int64, error) {
r, err := os.Open(s.ref.layerPath(info.Digest))
path, err := s.ref.layerPath(info.Digest)
if err != nil {
return nil, -1, err
}
r, err := os.Open(path)
if err != nil {
return nil, -1, err
}
Expand All @@ -84,7 +92,10 @@ func (s *dirImageSource) GetBlob(ctx context.Context, info types.BlobInfo, cache
func (s *dirImageSource) GetSignaturesWithFormat(ctx context.Context, instanceDigest *digest.Digest) ([]signature.Signature, error) {
signatures := []signature.Signature{}
for i := 0; ; i++ {
path := s.ref.signaturePath(i, instanceDigest)
path, err := s.ref.signaturePath(i, instanceDigest)
if err != nil {
return nil, err
}
sigBlob, err := os.ReadFile(path)
if err != nil {
if os.IsNotExist(err) {
Expand Down
7 changes: 4 additions & 3 deletions directory/directory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestGetPutManifest(t *testing.T) {
func TestGetPutBlob(t *testing.T) {
computedBlob := []byte("test-blob")
providedBlob := []byte("provided-blob")
providedDigest := digest.Digest("sha256:provided-test-digest")
providedDigest := digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")

ref, _ := refToTempDir(t)
cache := memory.New()
Expand Down Expand Up @@ -113,12 +113,13 @@ func (fn readerFromFunc) Read(p []byte) (int, error) {
// TestPutBlobDigestFailure simulates behavior on digest verification failure.
func TestPutBlobDigestFailure(t *testing.T) {
const digestErrorString = "Simulated digest error"
const blobDigest = digest.Digest("sha256:test-digest")
const blobDigest = digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")

ref, _ := refToTempDir(t)
dirRef, ok := ref.(dirReference)
require.True(t, ok)
blobPath := dirRef.layerPath(blobDigest)
blobPath, err := dirRef.layerPath(blobDigest)
require.NoError(t, err)
cache := memory.New()

firstRead := true
Expand Down
25 changes: 17 additions & 8 deletions directory/directory_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,25 +161,34 @@ func (ref dirReference) DeleteImage(ctx context.Context, sys *types.SystemContex
}

// manifestPath returns a path for the manifest within a directory using our conventions.
func (ref dirReference) manifestPath(instanceDigest *digest.Digest) string {
func (ref dirReference) manifestPath(instanceDigest *digest.Digest) (string, error) {
if instanceDigest != nil {
return filepath.Join(ref.path, instanceDigest.Encoded()+".manifest.json")
if err := instanceDigest.Validate(); err != nil { // digest.Digest.Encoded() panics on failure, and could possibly result in a path with ../, so validate explicitly.
return "", err
}
return filepath.Join(ref.path, instanceDigest.Encoded()+".manifest.json"), nil
}
return filepath.Join(ref.path, "manifest.json")
return filepath.Join(ref.path, "manifest.json"), nil
}

// layerPath returns a path for a layer tarball within a directory using our conventions.
func (ref dirReference) layerPath(digest digest.Digest) string {
func (ref dirReference) layerPath(digest digest.Digest) (string, error) {
if err := digest.Validate(); err != nil { // digest.Digest.Encoded() panics on failure, and could possibly result in a path with ../, so validate explicitly.
return "", err
}
// FIXME: Should we keep the digest identification?
return filepath.Join(ref.path, digest.Encoded())
return filepath.Join(ref.path, digest.Encoded()), nil
}

// signaturePath returns a path for a signature within a directory using our conventions.
func (ref dirReference) signaturePath(index int, instanceDigest *digest.Digest) string {
func (ref dirReference) signaturePath(index int, instanceDigest *digest.Digest) (string, error) {
if instanceDigest != nil {
return filepath.Join(ref.path, fmt.Sprintf(instanceDigest.Encoded()+".signature-%d", index+1))
if err := instanceDigest.Validate(); err != nil { // digest.Digest.Encoded() panics on failure, and could possibly result in a path with ../, so validate explicitly.
return "", err
}
return filepath.Join(ref.path, fmt.Sprintf(instanceDigest.Encoded()+".signature-%d", index+1)), nil
}
return filepath.Join(ref.path, fmt.Sprintf("signature-%d", index+1))
return filepath.Join(ref.path, fmt.Sprintf("signature-%d", index+1)), nil
}

// versionPath returns a path for the version file within a directory using our conventions.
Expand Down
36 changes: 29 additions & 7 deletions directory/directory_transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,15 @@ func TestReferenceManifestPath(t *testing.T) {
ref, tmpDir := refToTempDir(t)
dirRef, ok := ref.(dirReference)
require.True(t, ok)
assert.Equal(t, tmpDir+"/manifest.json", dirRef.manifestPath(nil))
assert.Equal(t, tmpDir+"/"+dhex.Encoded()+".manifest.json", dirRef.manifestPath(&dhex))
res, err := dirRef.manifestPath(nil)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/manifest.json", res)
res, err = dirRef.manifestPath(&dhex)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/"+dhex.Encoded()+".manifest.json", res)
invalidDigest := digest.Digest("sha256:../hello")
_, err = dirRef.manifestPath(&invalidDigest)
assert.Error(t, err)
}

func TestReferenceLayerPath(t *testing.T) {
Expand All @@ -207,7 +214,11 @@ func TestReferenceLayerPath(t *testing.T) {
ref, tmpDir := refToTempDir(t)
dirRef, ok := ref.(dirReference)
require.True(t, ok)
assert.Equal(t, tmpDir+"/"+hex, dirRef.layerPath("sha256:"+hex))
res, err := dirRef.layerPath("sha256:" + hex)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/"+hex, res)
_, err = dirRef.layerPath(digest.Digest("sha256:../hello"))
assert.Error(t, err)
}

func TestReferenceSignaturePath(t *testing.T) {
Expand All @@ -216,10 +227,21 @@ func TestReferenceSignaturePath(t *testing.T) {
ref, tmpDir := refToTempDir(t)
dirRef, ok := ref.(dirReference)
require.True(t, ok)
assert.Equal(t, tmpDir+"/signature-1", dirRef.signaturePath(0, nil))
assert.Equal(t, tmpDir+"/signature-10", dirRef.signaturePath(9, nil))
assert.Equal(t, tmpDir+"/"+dhex.Encoded()+".signature-1", dirRef.signaturePath(0, &dhex))
assert.Equal(t, tmpDir+"/"+dhex.Encoded()+".signature-10", dirRef.signaturePath(9, &dhex))
res, err := dirRef.signaturePath(0, nil)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/signature-1", res)
res, err = dirRef.signaturePath(9, nil)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/signature-10", res)
res, err = dirRef.signaturePath(0, &dhex)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/"+dhex.Encoded()+".signature-1", res)
res, err = dirRef.signaturePath(9, &dhex)
require.NoError(t, err)
assert.Equal(t, tmpDir+"/"+dhex.Encoded()+".signature-10", res)
invalidDigest := digest.Digest("sha256:../hello")
_, err = dirRef.signaturePath(0, &invalidDigest)
assert.Error(t, err)
}

func TestReferenceVersionPath(t *testing.T) {
Expand Down
20 changes: 17 additions & 3 deletions docker/docker_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,8 @@ func (c *dockerClient) detectProperties(ctx context.Context) error {
return c.detectPropertiesError
}

// fetchManifest fetches a manifest for (the repo of ref) + tagOrDigest.
// The caller is responsible for ensuring tagOrDigest uses the expected format.
func (c *dockerClient) fetchManifest(ctx context.Context, ref dockerReference, tagOrDigest string) ([]byte, string, error) {
path := fmt.Sprintf(manifestPath, reference.Path(ref.ref), tagOrDigest)
headers := map[string][]string{
Expand Down Expand Up @@ -1034,6 +1036,9 @@ func (c *dockerClient) getBlob(ctx context.Context, ref dockerReference, info ty
}
}

if err := info.Digest.Validate(); err != nil { // Make sure info.Digest.String() does not contain any unexpected characters
return nil, 0, err
}
path := fmt.Sprintf(blobsPath, reference.Path(ref.ref), info.Digest.String())
logrus.Debugf("Downloading %s", path)
res, err := c.makeRequest(ctx, http.MethodGet, path, nil, nil, v2Auth, nil)
Expand Down Expand Up @@ -1097,7 +1102,10 @@ func isManifestUnknownError(err error) bool {
// digest in ref.
// It returns (nil, nil) if the manifest does not exist.
func (c *dockerClient) getSigstoreAttachmentManifest(ctx context.Context, ref dockerReference, digest digest.Digest) (*manifest.OCI1, error) {
tag := sigstoreAttachmentTag(digest)
tag, err := sigstoreAttachmentTag(digest)
if err != nil {
return nil, err
}
sigstoreRef, err := reference.WithTag(reference.TrimNamed(ref.ref), tag)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1130,6 +1138,9 @@ func (c *dockerClient) getSigstoreAttachmentManifest(ctx context.Context, ref do
// getExtensionsSignatures returns signatures from the X-Registry-Supports-Signatures API extension,
// using the original data structures.
func (c *dockerClient) getExtensionsSignatures(ctx context.Context, ref dockerReference, manifestDigest digest.Digest) (*extensionSignatureList, error) {
if err := manifestDigest.Validate(); err != nil { // Make sure manifestDigest.String() does not contain any unexpected characters
return nil, err
}
path := fmt.Sprintf(extensionsSignaturePath, reference.Path(ref.ref), manifestDigest)
res, err := c.makeRequest(ctx, http.MethodGet, path, nil, nil, v2Auth, nil)
if err != nil {
Expand All @@ -1153,8 +1164,11 @@ func (c *dockerClient) getExtensionsSignatures(ctx context.Context, ref dockerRe
}

// sigstoreAttachmentTag returns a sigstore attachment tag for the specified digest.
func sigstoreAttachmentTag(d digest.Digest) string {
return strings.Replace(d.String(), ":", "-", 1) + ".sig"
func sigstoreAttachmentTag(d digest.Digest) (string, error) {
if err := d.Validate(); err != nil { // Make sure d.String() doesn’t contain any unexpected characters
return "", err
}
return strings.Replace(d.String(), ":", "-", 1) + ".sig", nil
}

// Close removes resources associated with an initialized dockerClient, if any.
Expand Down
7 changes: 6 additions & 1 deletion docker/docker_image.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,12 @@ func GetRepositoryTags(ctx context.Context, sys *types.SystemContext, ref types.
if err = json.NewDecoder(res.Body).Decode(&tagsHolder); err != nil {
return nil, err
}
tags = append(tags, tagsHolder.Tags...)
for _, tag := range tagsHolder.Tags {
if _, err := reference.WithTag(dr.ref, tag); err != nil { // Ensure the tag does not contain unexpected values
return nil, fmt.Errorf("registry returned invalid tag %q: %w", tag, err)
}
tags = append(tags, tag)
}

link := res.Header.Get("Link")
if link == "" {
Expand Down
Loading

0 comments on commit e894804

Please sign in to comment.