Call .Validate() before digest.Hex() / digest.Encoded()

... to prevent panics if the value does not contain a :, or other unexpected
values (e.g. a path traversal).

Don't bother on paths where we computed the digest ourselves, or it is already trusted
for other reasons.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

copy/progress_bars.go

@@ -49,10 +49,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
@@ -105,7 +108,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

copy/single.go

@@ -606,7 +606,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)
 
@@ -738,15 +741,21 @@ 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
+			if err := func() error { // A scope for defer
 				label := "skipped: already exists"
 				if reusedBlob.MatchedByTOCDigest {
 					label = "skipped: already exists (found by TOC)"
 				}
-				bar := ic.c.createProgressBar(pool, false, types.BlobInfo{Digest: reusedBlob.Digest, Size: 0}, "blob", label)
+				bar, err := ic.c.createProgressBar(pool, false, types.BlobInfo{Digest: reusedBlob.Digest, Size: 0}, "blob", label)
+				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 {
@@ -765,8 +774,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)
@@ -789,18 +801,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)

directory/directory_dest.go

@@ -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
@@ -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
@@ -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.
@@ -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
 		}
 	}

directory/directory_src.go

@@ -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
 	}
@@ -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
 	}
@@ -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) {

directory/directory_test.go

@@ -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()
@@ -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

directory/directory_transport.go

@@ -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.

directory/directory_transport_test.go

@@ -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) {
@@ -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) {
@@ -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) {

docker/docker_image_dest.go

@@ -635,19 +635,24 @@ func (d *dockerImageDestination) putSignaturesToLookaside(signatures []signature
 
 	// NOTE: Keep this in sync with docs/signature-protocols.md!
 	for i, signature := range signatures {
-		sigURL := lookasideStorageURL(d.c.signatureBase, manifestDigest, i)
-		err := d.putOneSignature(sigURL, signature)
+		sigURL, err := lookasideStorageURL(d.c.signatureBase, manifestDigest, i)
 		if err != nil {
 			return err
 		}
+		if err := d.putOneSignature(sigURL, signature); err != nil {
+			return err
+		}
 	}
 	// Remove any other signatures, if present.
 	// We stop at the first missing signature; if a previous deleting loop aborted
 	// prematurely, this may not clean up all of them, but one missing signature
 	// is enough for dockerImageSource to stop looking for other signatures, so that
 	// is sufficient.
 	for i := len(signatures); ; i++ {
-		sigURL := lookasideStorageURL(d.c.signatureBase, manifestDigest, i)
+		sigURL, err := lookasideStorageURL(d.c.signatureBase, manifestDigest, i)
+		if err != nil {
+			return err
+		}
 		missing, err := d.c.deleteOneSignature(sigURL)
 		if err != nil {
 			return err

docker/docker_image_src.go

@@ -462,7 +462,10 @@ func (s *dockerImageSource) getSignaturesFromLookaside(ctx context.Context, inst
 			return nil, fmt.Errorf("server provided %d signatures, assuming that's unreasonable and a server error", maxLookasideSignatures)
 		}
 
-		sigURL := lookasideStorageURL(s.c.signatureBase, manifestDigest, i)
+		sigURL, err := lookasideStorageURL(s.c.signatureBase, manifestDigest, i)
+		if err != nil {
+			return nil, err
+		}
 		signature, missing, err := s.getOneSignature(ctx, sigURL)
 		if err != nil {
 			return nil, err
@@ -660,7 +663,10 @@ func deleteImage(ctx context.Context, sys *types.SystemContext, ref dockerRefere
 	}
 
 	for i := 0; ; i++ {
-		sigURL := lookasideStorageURL(c.signatureBase, manifestDigest, i)
+		sigURL, err := lookasideStorageURL(c.signatureBase, manifestDigest, i)
+		if err != nil {
+			return err
+		}
 		missing, err := c.deleteOneSignature(sigURL)
 		if err != nil {
 			return err

docker/internal/tarfile/dest.go

@@ -111,11 +111,19 @@ func (d *Destination) PutBlobWithOptions(ctx context.Context, stream io.Reader,
 			return private.UploadedBlob{}, fmt.Errorf("reading Config file stream: %w", err)
 		}
 		d.config = buf
-		if err := d.archive.sendFileLocked(d.archive.configPath(inputInfo.Digest), inputInfo.Size, bytes.NewReader(buf)); err != nil {
+		configPath, err := d.archive.configPath(inputInfo.Digest)
+		if err != nil {
+			return private.UploadedBlob{}, err
+		}
+		if err := d.archive.sendFileLocked(configPath, inputInfo.Size, bytes.NewReader(buf)); err != nil {
 			return private.UploadedBlob{}, fmt.Errorf("writing Config file: %w", err)
 		}
 	} else {
-		if err := d.archive.sendFileLocked(d.archive.physicalLayerPath(inputInfo.Digest), inputInfo.Size, stream); err != nil {
+		layerPath, err := d.archive.physicalLayerPath(inputInfo.Digest)
+		if err != nil {
+			return private.UploadedBlob{}, err
+		}
+		if err := d.archive.sendFileLocked(layerPath, inputInfo.Size, stream); err != nil {
 			return private.UploadedBlob{}, err
 		}
 	}