Add safe path join helper

common/util/file_helper.go

@@ -1,6 +1,7 @@
 package util
 
 import (
+	"fmt"
 	gopath "path"
 	"path/filepath"
 	"strings"
@@ -37,3 +38,12 @@ func AbsolutifyPath(pathToManifest string, pathToFile string, fs boshsys.FileSys
 
 	return absPath, nil
 }
+
+// SafeJoinPath returns filepath.Join(base, untrusted), or an error if
+// untrusted is not a local path.
+func SafeJoinPath(base, untrusted string) (string, error) {
+	if !filepath.IsLocal(untrusted) {
+		return "", fmt.Errorf("path '%s' is not a safe local path", untrusted)
+	}
+	return filepath.Join(base, untrusted), nil
+}

common/util/file_helper_test.go

@@ -11,6 +11,38 @@ import (
 	"github.com/cloudfoundry/bosh-cli/v7/common/util"
 )
 
+var _ = Describe("SafeJoinPath", func() {
+	It("returns the joined path for a normal relative path", func() {
+		result, err := util.SafeJoinPath(filepath.Join("/", "base"), filepath.Join("subdir", "file.tgz"))
+		Expect(err).ToNot(HaveOccurred())
+		Expect(result).To(Equal(filepath.Join("/", "base", "subdir", "file.tgz")))
+	})
+
+	It("rejects a single .. component", func() {
+		_, err := util.SafeJoinPath(filepath.Join("/", "base"), "../file")
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("safe local path"))
+	})
+
+	It("rejects a deeply nested path traversal", func() {
+		_, err := util.SafeJoinPath(filepath.Join("/", "base"), "../../etc/file")
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("safe local path"))
+	})
+
+	It("rejects an absolute path", func() {
+		_, err := util.SafeJoinPath(filepath.Join("/", "base"), "/etc/file")
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("safe local path"))
+	})
+
+	It("rejects an empty path", func() {
+		_, err := util.SafeJoinPath(filepath.Join("/", "base"), "")
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("safe local path"))
+	})
+})
+
 var _ = Describe("AbsolutifyPath", func() {
 	var realfs boshsys.FileSystem
 	var fakeManifestPath, fakeFilePath string

installation/installer.go

@@ -7,6 +7,7 @@ import (
 	bosherr "github.com/cloudfoundry/bosh-utils/errors"
 	boshlog "github.com/cloudfoundry/bosh-utils/logger"
 
+	util "github.com/cloudfoundry/bosh-cli/v7/common/util"
 	"github.com/cloudfoundry/bosh-cli/v7/installation/blobextract"
 	biinstallmanifest "github.com/cloudfoundry/bosh-cli/v7/installation/manifest"
 	biui "github.com/cloudfoundry/bosh-cli/v7/ui"
@@ -117,8 +118,11 @@ func (i *installer) Cleanup(installation Installation) error {
 
 func (i *installer) installPackages(compiledPackages []CompiledPackageRef) error {
 	for _, pkg := range compiledPackages {
-		err := i.blobExtractor.Extract(pkg.BlobstoreID, pkg.SHA1, filepath.Join(i.target.PackagesPath(), pkg.Name))
+		targetDir, err := util.SafeJoinPath(i.target.PackagesPath(), pkg.Name)
 		if err != nil {
+			return bosherr.Errorf("Invalid package name '%s': must be a safe local path", pkg.Name)
+		}
+		if err = i.blobExtractor.Extract(pkg.BlobstoreID, pkg.SHA1, targetDir); err != nil {
 			return bosherr.WrapErrorf(err, "Installing package '%s'", pkg.Name)
 		}
 	}
@@ -127,17 +131,17 @@ func (i *installer) installPackages(compiledPackages []CompiledPackageRef) error
 
 func (i *installer) installJob(renderedJobRef RenderedJobRef, stage biui.Stage) (installedJob InstalledJob, err error) {
 	err = stage.Perform(fmt.Sprintf("Installing job '%s'", renderedJobRef.Name), func() error {
-		var stageErr error
-		jobDir := filepath.Join(i.target.JobsPath(), renderedJobRef.Name)
+		jobDir, err := util.SafeJoinPath(i.target.JobsPath(), renderedJobRef.Name)
+		if err != nil {
+			return bosherr.Errorf("Invalid job name '%s': must be a safe local path", renderedJobRef.Name)
+		}
 
-		stageErr = i.blobExtractor.Extract(renderedJobRef.BlobstoreID, renderedJobRef.SHA1, jobDir)
-		if stageErr != nil {
-			return bosherr.WrapErrorf(stageErr, "Extracting blob with ID '%s'", renderedJobRef.BlobstoreID)
+		if err = i.blobExtractor.Extract(renderedJobRef.BlobstoreID, renderedJobRef.SHA1, jobDir); err != nil {
+			return bosherr.WrapErrorf(err, "Extracting blob with ID '%s'", renderedJobRef.BlobstoreID)
 		}
 
-		stageErr = i.blobExtractor.ChmodExecutables(filepath.Join(jobDir, "bin", "*"))
-		if stageErr != nil {
-			return bosherr.WrapErrorf(stageErr, "Chmoding binaries for '%s'", jobDir)
+		if err = i.blobExtractor.ChmodExecutables(filepath.Join(jobDir, "bin", "*")); err != nil {
+			return bosherr.WrapErrorf(err, "Chmoding binaries for '%s'", jobDir)
 		}
 
 		installedJob = NewInstalledJob(renderedJobRef, jobDir)

installation/installer_test.go

@@ -140,6 +140,43 @@ var _ = Describe("Installer", func() {
 		})
 	})
 
+	Describe("Install path traversal protection", func() {
+		var fakeStage *fakebiui.FakeStage
+
+		BeforeEach(func() {
+			fakeStage = fakebiui.NewFakeStage()
+		})
+
+		It("returns error when a compiled package name contains path traversal", func() {
+			maliciousRef := CompiledPackageRef{
+				Name:        "../../path",
+				Version:     "v1",
+				BlobstoreID: "bid",
+				SHA1:        "sha",
+			}
+			releaseJobs := []bireljob.Job{}
+			mockJobResolver.EXPECT().From(installationManifest).Return(releaseJobs, nil)
+			mockPackageCompiler.EXPECT().For(releaseJobs, fakeStage).Return([]CompiledPackageRef{maliciousRef}, nil)
+
+			_, err := installer.Install(installationManifest, fakeStage)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
+
+		It("returns error when a rendered job name contains path traversal", func() {
+			releaseJobs := []bireljob.Job{}
+			compiledPackages := []CompiledPackageRef{}
+			jobRef := NewRenderedJobRef("../../path", "fp", "blob-id", "sha")
+			mockJobResolver.EXPECT().From(installationManifest).Return(releaseJobs, nil)
+			mockPackageCompiler.EXPECT().For(releaseJobs, fakeStage).Return(compiledPackages, nil)
+			mockJobRenderer.EXPECT().RenderAndUploadFrom(installationManifest, releaseJobs, fakeStage).Return([]RenderedJobRef{jobRef}, nil)
+
+			_, err := installer.Install(installationManifest, fakeStage)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
+	})
+
 	Describe("Cleanup", func() {
 		var installation Installation
 

releasedir/fs_blobs_dir.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cloudfoundry/bosh-utils/work"
 	"gopkg.in/yaml.v2"
 
+	util "github.com/cloudfoundry/bosh-cli/v7/common/util"
 	bicrypto "github.com/cloudfoundry/bosh-cli/v7/crypto"
 )
 
@@ -101,6 +102,9 @@ func (d FSBlobsDir) Blobs() ([]Blob, error) {
 	var blobs []Blob
 
 	for recPath, rec := range schema {
+		if _, err := util.SafeJoinPath(d.dirPath, recPath); err != nil {
+			return nil, bosherr.Errorf("Invalid blob path '%s': must be a safe local path", recPath)
+		}
 		blobs = append(blobs, Blob{
 			Path:        recPath,
 			Size:        rec.Size,
@@ -193,6 +197,11 @@ func (d FSBlobsDir) removeUnknownBlobs(blobs []Blob) error {
 }
 
 func (d FSBlobsDir) TrackBlob(path string, src io.ReadCloser) (Blob, error) {
+	safePath, err := util.SafeJoinPath(d.dirPath, path)
+	if err != nil {
+		return Blob{}, bosherr.Errorf("Invalid blob path '%s': must be a safe local path", path)
+	}
+
 	tempFile, err := d.fs.TempFile("track-blob")
 	if err != nil {
 		return Blob{}, bosherr.WrapErrorf(err, "Creating temp blob")
@@ -239,7 +248,7 @@ func (d FSBlobsDir) TrackBlob(path string, src io.ReadCloser) (Blob, error) {
 
 	tempFile.Close() //nolint:errcheck
 
-	err = d.moveBlobLocally(tempFile.Name(), filepath.Join(d.dirPath, path))
+	err = d.moveBlobLocally(tempFile.Name(), safePath)
 	if err != nil {
 		return Blob{}, err
 	}
@@ -248,12 +257,17 @@ func (d FSBlobsDir) TrackBlob(path string, src io.ReadCloser) (Blob, error) {
 }
 
 func (d FSBlobsDir) UntrackBlob(path string) error {
+	safePath, err := util.SafeJoinPath(d.dirPath, path)
+	if err != nil {
+		return bosherr.Errorf("Invalid blob path '%s': must be a safe local path", path)
+	}
+
 	blobs, err := d.Blobs()
 	if err != nil {
 		return err
 	}
 
-	err = d.fs.RemoveAll(filepath.Join(d.dirPath, path))
+	err = d.fs.RemoveAll(safePath)
 	if err != nil {
 		return bosherr.WrapErrorf(err, "Removing blob from blobs/")
 	}

releasedir/fs_blobs_dir_test.go

@@ -106,6 +106,34 @@ file2.tgz:
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("Unmarshalling blobs index"))
 		})
+
+		It("returns error for a blob path that escapes the blobs directory via ..", func() {
+			err := fs.WriteFileString(filepath.Join("/", "dir", "config", "blobs.yml"), `
+../../../../home/file:
+  size: 100
+  object_id: id
+  sha: sha
+`)
+			Expect(err).ToNot(HaveOccurred())
+
+			_, err = act()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
+
+		It("returns error for an absolute blob path", func() {
+			err := fs.WriteFileString(filepath.Join("/", "dir", "config", "blobs.yml"), `
+/etc/file:
+  size: 100
+  object_id: id
+  sha: sha
+`)
+			Expect(err).ToNot(HaveOccurred())
+
+			_, err = act()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
 	})
 
 	Describe("SyncBlobs", func() {
@@ -600,6 +628,20 @@ file2.tgz:
 
 			Expect(blobsDir.Blobs()).To(BeEmpty())
 		})
+
+		It("returns error for a blob path that escapes the blobs directory via ..", func() {
+			content := io.NopCloser(strings.NewReader("content"))
+			_, err := blobsDir.TrackBlob("../../etc/file", content)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
+
+		It("returns error for an absolute blob path", func() {
+			content := io.NopCloser(strings.NewReader("content"))
+			_, err := blobsDir.TrackBlob("/etc/file", content)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
 	})
 
 	Describe("UntrackBlob", func() {
@@ -714,6 +756,18 @@ bosh-116.tgz:
 				{Path: filepath.Join("dir", "file.tgz"), Size: 133, SHA1: "13e"},
 			}))
 		})
+
+		It("returns error for a blob path that escapes the blobs directory via ..", func() {
+			err := blobsDir.UntrackBlob("../../etc/file")
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
+
+		It("returns error for an absolute blob path", func() {
+			err := blobsDir.UntrackBlob("/etc/file")
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safe local path"))
+		})
 	})
 
 	Describe("UploadBlobs", func() {

releasedir/index/fs_index_blobs.go

@@ -3,7 +3,6 @@ package index
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"runtime"
 	"strings"
 
@@ -12,6 +11,8 @@ import (
 	bosherr "github.com/cloudfoundry/bosh-utils/errors"
 	boshfu "github.com/cloudfoundry/bosh-utils/fileutil"
 	boshsys "github.com/cloudfoundry/bosh-utils/system"
+
+	util "github.com/cloudfoundry/bosh-cli/v7/common/util"
 )
 
 type FSIndexBlobs struct {
@@ -141,5 +142,10 @@ func (c FSIndexBlobs) blobPath(sha1 string) (string, error) {
 	if runtime.GOOS == "windows" {
 		sha1 = strings.ReplaceAll(sha1, ":", "_")
 	}
-	return filepath.Join(absDirPath, sha1), nil
+
+	candidatePath, err := util.SafeJoinPath(absDirPath, sha1)
+	if err != nil {
+		return "", bosherr.Errorf("Invalid blob SHA1 '%s': must be a safe local path", sha1)
+	}
+	return candidatePath, nil
 }

releasedir/index/fs_index_blobs_test.go

@@ -4,7 +4,6 @@ import (
 	"errors"
 	"os"
 	"path/filepath"
-	"strings"
 	"syscall"
 
 	fakeblob "github.com/cloudfoundry/bosh-utils/blobstore/fakes"
@@ -115,6 +114,12 @@ var _ = Describe("FSIndexBlobs", func() {
 				Expect(err).To(HaveOccurred())
 				Expect(err.Error()).To(Equal("Cannot find blob named 'name' with SHA1 'sha1'"))
 			})
+
+			It("returns error if sha1 is a path", func() {
+				_, err := blobs.Get("name", "blob-id", "../../file")
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("safe local path"))
+			})
 		})
 
 		Context("when configured with a blobstore", func() {
@@ -160,12 +165,10 @@ var _ = Describe("FSIndexBlobs", func() {
 				Expect(reporter.IndexEntryDownloadFinishedCallCount()).To(Equal(1))
 			})
 
-			It("returns error if parsing digest string fails", func() {
-				//currently, the only way to cause a digest parse failure is with an empty string
+			It("returns error if sha1 is empty", func() {
 				_, err := blobs.Get("name", "blob-id", "")
 				Expect(err).To(HaveOccurred())
-				Expect(strings.ToLower(err.Error())).To(ContainSubstring(
-					"no digest algorithm found. supported algorithms: sha1, sha256, sha512"))
+				Expect(err.Error()).To(ContainSubstring("safe local path"))
 			})
 
 			Context("when downloading blob fails", func() {
@@ -284,6 +287,24 @@ var _ = Describe("FSIndexBlobs", func() {
 			Expect(err).ToNot(HaveOccurred())
 		})
 
+		Context("when sha1 is a path", func() {
+			BeforeEach(func() {
+				blobs = boshidx.NewFSIndexBlobs(filepath.Join("/", "dir", "sub-dir"), reporter, nil, fs)
+			})
+
+			It("returns error for a path with ..", func() {
+				_, _, err := blobs.Add("name", filepath.Join("/", "tmp", "payload"), "../../.file")
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("safe local path"))
+			})
+
+			It("returns error for an absolute path sha1", func() {
+				_, _, err := blobs.Add("name", filepath.Join("/", "tmp", "payload"), "/etc/file")
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("safe local path"))
+			})
+		})
+
 		itCopiesFileIntoDir := func() {
 			It("copies file into cache dir", func() {
 				blobID, path, err := blobs.Add("name", filepath.Join("/", "tmp", "sha1"), "sha1")