distribution: prevent uncontrolled data used in path expression

Signed-off-by: Dorin Geman <dorin.geman@docker.com>

Author: doringeman

pkg/distribution/internal/store/blobs.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/docker/model-runner/pkg/distribution/internal/progress"
 
@@ -15,14 +16,61 @@ const (
 	blobsDir = "blobs"
 )
 
+var allowedAlgorithms = map[string]int{
+	"sha256": 64,
+	"sha512": 128,
+}
+
+func isSafeAlgorithm(a string) (int, bool) {
+	hexLength, ok := allowedAlgorithms[a]
+	return hexLength, ok
+}
+
+func isSafeHex(hexLength int, s string) bool {
+	if len(s) != hexLength {
+		return false
+	}
+	for _, c := range s {
+		if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) {
+			return false
+		}
+	}
+	return true
+}
+
+// validateHash ensures the hash components are safe for filesystem paths
+func validateHash(hash v1.Hash) error {
+	hexLength, ok := isSafeAlgorithm(hash.Algorithm)
+	if !ok {
+		return fmt.Errorf("invalid hash algorithm: %q not in allowlist", hash.Algorithm)
+	}
+	if !isSafeHex(hexLength, hash.Hex) {
+		return fmt.Errorf("invalid hash hex: contains non-hexadecimal characters or invalid length")
+	}
+	return nil
+}
+
 // blobDir returns the path to the blobs directory
 func (s *LocalStore) blobsDir() string {
 	return filepath.Join(s.rootPath, blobsDir)
 }
 
 // blobPath returns the path to the blob for the given hash.
-func (s *LocalStore) blobPath(hash v1.Hash) string {
-	return filepath.Join(s.rootPath, blobsDir, hash.Algorithm, hash.Hex)
+func (s *LocalStore) blobPath(hash v1.Hash) (string, error) {
+	if err := validateHash(hash); err != nil {
+		return "", fmt.Errorf("unsafe hash: %w", err)
+	}
+
+	path := filepath.Join(s.rootPath, blobsDir, hash.Algorithm, hash.Hex)
+
+	cleanRootPath := filepath.Clean(s.rootPath)
+	cleanPath := filepath.Clean(path)
+	relPath, err := filepath.Rel(cleanRootPath, cleanPath)
+	if err != nil || strings.HasPrefix(relPath, "..") {
+		return "", fmt.Errorf("path traversal attempt detected: %s", path)
+	}
+
+	return cleanPath, nil
 }
 
 type blob interface {
@@ -36,7 +84,11 @@ func (s *LocalStore) writeLayer(layer blob, updates chan<- v1.Update) error {
 	if err != nil {
 		return fmt.Errorf("get file hash: %w", err)
 	}
-	if s.hasBlob(hash) {
+	hasBlob, err := s.hasBlob(hash)
+	if err != nil {
+		return fmt.Errorf("check blob existence: %w", err)
+	}
+	if hasBlob {
 		// todo: write something to the progress channel (we probably need to redo progress reporting a little bit)
 		return nil
 	}
@@ -54,11 +106,18 @@ func (s *LocalStore) writeLayer(layer blob, updates chan<- v1.Update) error {
 // WriteBlob writes the blob to the store, reporting progress to the given channel.
 // If the blob is already in the store, it is a no-op and the blob is not consumed from the reader.
 func (s *LocalStore) WriteBlob(diffID v1.Hash, r io.Reader) error {
-	if s.hasBlob(diffID) {
+	hasBlob, err := s.hasBlob(diffID)
+	if err != nil {
+		return fmt.Errorf("check blob existence: %w", err)
+	}
+	if hasBlob {
 		return nil
 	}
 
-	path := s.blobPath(diffID)
+	path, err := s.blobPath(diffID)
+	if err != nil {
+		return fmt.Errorf("get blob path: %w", err)
+	}
 	f, err := createFile(incompletePath(path))
 	if err != nil {
 		return fmt.Errorf("create blob file: %w", err)
@@ -79,14 +138,22 @@ func (s *LocalStore) WriteBlob(diffID v1.Hash, r io.Reader) error {
 
 // removeBlob removes the blob with the given hash from the store.
 func (s *LocalStore) removeBlob(hash v1.Hash) error {
-	return os.Remove(s.blobPath(hash))
+	path, err := s.blobPath(hash)
+	if err != nil {
+		return fmt.Errorf("get blob path: %w", err)
+	}
+	return os.Remove(path)
 }
 
-func (s *LocalStore) hasBlob(hash v1.Hash) bool {
-	if _, err := os.Stat(s.blobPath(hash)); err == nil {
-		return true
+func (s *LocalStore) hasBlob(hash v1.Hash) (bool, error) {
+	path, err := s.blobPath(hash)
+	if err != nil {
+		return false, fmt.Errorf("get blob path: %w", err)
 	}
-	return false
+	if _, err := os.Stat(path); err == nil {
+		return true, nil
+	}
+	return false, nil
 }
 
 // createFile is a wrapper around os.Create that creates any parent directories as needed.
@@ -112,5 +179,9 @@ func (s *LocalStore) writeConfigFile(mdl v1.Image) error {
 	if err != nil {
 		return fmt.Errorf("get raw manifest: %w", err)
 	}
-	return writeFile(s.blobPath(hash), rcf)
+	path, err := s.blobPath(hash)
+	if err != nil {
+		return fmt.Errorf("get blob path: %w", err)
+	}
+	return writeFile(path, rcf)
 }

pkg/distribution/internal/store/blobs_test.go

@@ -41,7 +41,11 @@ func TestBlobs(t *testing.T) {
 		}
 
 		// ensure blob file exists
-		content, err := os.ReadFile(store.blobPath(hash))
+		blobPath, err := store.blobPath(hash)
+		if err != nil {
+			t.Fatalf("error getting blob path: %v", err)
+		}
+		content, err := os.ReadFile(blobPath)
 		if err != nil {
 			t.Fatalf("error reading blob file: %v", err)
 		}
@@ -52,7 +56,11 @@ func TestBlobs(t *testing.T) {
 		}
 
 		// ensure incomplete blob file does not exist
-		tmpFile := incompletePath(store.blobPath(hash))
+		blobPath, err = store.blobPath(hash)
+		if err != nil {
+			t.Fatalf("error getting blob path: %v", err)
+		}
+		tmpFile := incompletePath(blobPath)
 		if _, err := os.Stat(tmpFile); !errors.Is(err, os.ErrNotExist) {
 			t.Fatalf("expected incomplete blob file %s not be present", tmpFile)
 		}
@@ -61,10 +69,14 @@ func TestBlobs(t *testing.T) {
 	t.Run("WriteBlob fails", func(t *testing.T) {
 		// simulate lingering incomplete blob file (if program crashed)
 		hash := v1.Hash{
-			Algorithm: "some-alg",
-			Hex:       "some-hash",
+			Algorithm: "sha256",
+			Hex:       "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+		}
+		blobPath, err := store.blobPath(hash)
+		if err != nil {
+			t.Fatalf("error getting blob path: %v", err)
 		}
-		if err := writeFile(incompletePath(store.blobPath(hash)), []byte("incomplete")); err != nil {
+		if err := writeFile(incompletePath(blobPath), []byte("incomplete")); err != nil {
 			t.Fatalf("error creating incomplete blob file for test: %v", err)
 		}
 
@@ -73,21 +85,29 @@ func TestBlobs(t *testing.T) {
 		}
 
 		// ensure blob file does not exist
-		if _, err := os.ReadFile(store.blobPath(hash)); !errors.Is(err, os.ErrNotExist) {
+		blobPath2, err := store.blobPath(hash)
+		if err != nil {
+			t.Fatalf("error getting blob path: %v", err)
+		}
+		if _, err := os.ReadFile(blobPath2); !errors.Is(err, os.ErrNotExist) {
 			t.Fatalf("expected blob file not to exist")
 		}
 
 		// ensure incomplete file is not left behind
-		if _, err := os.ReadFile(incompletePath(store.blobPath(hash))); !errors.Is(err, os.ErrNotExist) {
+		blobPath3, err := store.blobPath(hash)
+		if err != nil {
+			t.Fatalf("error getting blob path: %v", err)
+		}
+		if _, err := os.ReadFile(incompletePath(blobPath3)); !errors.Is(err, os.ErrNotExist) {
 			t.Fatalf("expected incomplete blob file not to exist")
 		}
 	})
 
 	t.Run("WriteBlob reuses existing blob", func(t *testing.T) {
 		// simulate existing blob
 		hash := v1.Hash{
-			Algorithm: "some-alg",
-			Hex:       "some-hash",
+			Algorithm: "sha256",
+			Hex:       "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
 		}
 
 		if err := store.WriteBlob(hash, bytes.NewReader([]byte("some-data"))); err != nil {
@@ -99,7 +119,11 @@ func TestBlobs(t *testing.T) {
 		}
 
 		// ensure blob file exists
-		content, err := os.ReadFile(store.blobPath(hash))
+		blobPath4, err := store.blobPath(hash)
+		if err != nil {
+			t.Fatalf("error getting blob path: %v", err)
+		}
+		content, err := os.ReadFile(blobPath4)
 		if err != nil {
 			t.Fatalf("error reading blob file: %v", err)
 		}

pkg/distribution/internal/store/manifests.go

@@ -2,7 +2,6 @@ package store
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -26,8 +25,12 @@ func (s *LocalStore) WriteManifest(hash v1.Hash, raw []byte) error {
 		return fmt.Errorf("parse manifest: %w", err)
 	}
 	for _, layer := range manifest.Layers {
-		if !s.hasBlob(layer.Digest) {
-			return errors.New("missing blob %q for manifest - refusing to write unless all blobs exist")
+		hasBlob, err := s.hasBlob(layer.Digest)
+		if err != nil {
+			return fmt.Errorf("check blob existence: %w", err)
+		}
+		if !hasBlob {
+			return fmt.Errorf("missing blob %q for manifest - refusing to write unless all blobs exist", layer.Digest)
 		}
 	}
 	if err := writeFile(s.manifestPath(hash), raw); err != nil {

pkg/distribution/internal/store/model.go

@@ -35,15 +35,23 @@ func (s *LocalStore) newModel(digest v1.Hash, tags []string) (*Model, error) {
 		return nil, fmt.Errorf("parse manifest: %w", err)
 	}
 
-	rawConfigFile, err := os.ReadFile(s.blobPath(manifest.Config.Digest))
+	configPath, err := s.blobPath(manifest.Config.Digest)
+	if err != nil {
+		return nil, fmt.Errorf("get config blob path: %w", err)
+	}
+	rawConfigFile, err := os.ReadFile(configPath)
 	if err != nil {
 		return nil, fmt.Errorf("read config file: %w", err)
 	}
 
 	layers := make([]v1.Layer, len(manifest.Layers))
 	for i, ld := range manifest.Layers {
+		layerPath, err := s.blobPath(ld.Digest)
+		if err != nil {
+			return nil, fmt.Errorf("get layer blob path: %w", err)
+		}
 		layers[i] = &mdpartial.Layer{
-			Path:       s.blobPath(ld.Digest),
+			Path:       layerPath,
 			Descriptor: ld,
 		}
 	}