Skip to content

Commit c280128

Browse files
selzocselzoc
authored
Merge pull request #726 from cloudfoundry/refactor-path-traversal
Add safe path join helper
2 parents 3752eea + 513ecef commit c280128

10 files changed

Lines changed: 226 additions & 23 deletions

File tree

‎common/util/file_helper.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package util
22

33
import (
4+
"fmt"
45
gopath "path"
56
"path/filepath"
67
"strings"
@@ -37,3 +38,12 @@ func AbsolutifyPath(pathToManifest string, pathToFile string, fs boshsys.FileSys
3738

3839
return absPath, nil
3940
}
41+
42+
// SafeJoinPath returns filepath.Join(base, untrusted), or an error if
43+
// untrusted is not a local path.
44+
func SafeJoinPath(base, untrusted string) (string, error) {
45+
if !filepath.IsLocal(untrusted) {
46+
return "", fmt.Errorf("path '%s' is not a safe local path", untrusted)
47+
}
48+
return filepath.Join(base, untrusted), nil
49+
}

‎common/util/file_helper_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,38 @@ import (
1111
"github.com/cloudfoundry/bosh-cli/v7/common/util"
1212
)
1313

14+
var _ = Describe("SafeJoinPath", func() {
15+
It("returns the joined path for a normal relative path", func() {
16+
result, err := util.SafeJoinPath(filepath.Join("/", "base"), filepath.Join("subdir", "file.tgz"))
17+
Expect(err).ToNot(HaveOccurred())
18+
Expect(result).To(Equal(filepath.Join("/", "base", "subdir", "file.tgz")))
19+
})
20+
21+
It("rejects a single .. component", func() {
22+
_, err := util.SafeJoinPath(filepath.Join("/", "base"), "../file")
23+
Expect(err).To(HaveOccurred())
24+
Expect(err.Error()).To(ContainSubstring("safe local path"))
25+
})
26+
27+
It("rejects a deeply nested path traversal", func() {
28+
_, err := util.SafeJoinPath(filepath.Join("/", "base"), "../../etc/file")
29+
Expect(err).To(HaveOccurred())
30+
Expect(err.Error()).To(ContainSubstring("safe local path"))
31+
})
32+
33+
It("rejects an absolute path", func() {
34+
_, err := util.SafeJoinPath(filepath.Join("/", "base"), "/etc/file")
35+
Expect(err).To(HaveOccurred())
36+
Expect(err.Error()).To(ContainSubstring("safe local path"))
37+
})
38+
39+
It("rejects an empty path", func() {
40+
_, err := util.SafeJoinPath(filepath.Join("/", "base"), "")
41+
Expect(err).To(HaveOccurred())
42+
Expect(err.Error()).To(ContainSubstring("safe local path"))
43+
})
44+
})
45+
1446
var _ = Describe("AbsolutifyPath", func() {
1547
var realfs boshsys.FileSystem
1648
var fakeManifestPath, fakeFilePath string

‎installation/installer.go‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
bosherr "github.com/cloudfoundry/bosh-utils/errors"
88
boshlog "github.com/cloudfoundry/bosh-utils/logger"
99

10+
util "github.com/cloudfoundry/bosh-cli/v7/common/util"
1011
"github.com/cloudfoundry/bosh-cli/v7/installation/blobextract"
1112
biinstallmanifest "github.com/cloudfoundry/bosh-cli/v7/installation/manifest"
1213
biui "github.com/cloudfoundry/bosh-cli/v7/ui"
@@ -117,8 +118,11 @@ func (i *installer) Cleanup(installation Installation) error {
117118

118119
func (i *installer) installPackages(compiledPackages []CompiledPackageRef) error {
119120
for _, pkg := range compiledPackages {
120-
err := i.blobExtractor.Extract(pkg.BlobstoreID, pkg.SHA1, filepath.Join(i.target.PackagesPath(), pkg.Name))
121+
targetDir, err := util.SafeJoinPath(i.target.PackagesPath(), pkg.Name)
121122
if err != nil {
123+
return bosherr.Errorf("Invalid package name '%s': must be a safe local path", pkg.Name)
124+
}
125+
if err = i.blobExtractor.Extract(pkg.BlobstoreID, pkg.SHA1, targetDir); err != nil {
122126
return bosherr.WrapErrorf(err, "Installing package '%s'", pkg.Name)
123127
}
124128
}
@@ -127,17 +131,17 @@ func (i *installer) installPackages(compiledPackages []CompiledPackageRef) error
127131

128132
func (i *installer) installJob(renderedJobRef RenderedJobRef, stage biui.Stage) (installedJob InstalledJob, err error) {
129133
err = stage.Perform(fmt.Sprintf("Installing job '%s'", renderedJobRef.Name), func() error {
130-
var stageErr error
131-
jobDir := filepath.Join(i.target.JobsPath(), renderedJobRef.Name)
134+
jobDir, err := util.SafeJoinPath(i.target.JobsPath(), renderedJobRef.Name)
135+
if err != nil {
136+
return bosherr.Errorf("Invalid job name '%s': must be a safe local path", renderedJobRef.Name)
137+
}
132138

133-
stageErr = i.blobExtractor.Extract(renderedJobRef.BlobstoreID, renderedJobRef.SHA1, jobDir)
134-
if stageErr != nil {
135-
return bosherr.WrapErrorf(stageErr, "Extracting blob with ID '%s'", renderedJobRef.BlobstoreID)
139+
if err = i.blobExtractor.Extract(renderedJobRef.BlobstoreID, renderedJobRef.SHA1, jobDir); err != nil {
140+
return bosherr.WrapErrorf(err, "Extracting blob with ID '%s'", renderedJobRef.BlobstoreID)
136141
}
137142

138-
stageErr = i.blobExtractor.ChmodExecutables(filepath.Join(jobDir, "bin", "*"))
139-
if stageErr != nil {
140-
return bosherr.WrapErrorf(stageErr, "Chmoding binaries for '%s'", jobDir)
143+
if err = i.blobExtractor.ChmodExecutables(filepath.Join(jobDir, "bin", "*")); err != nil {
144+
return bosherr.WrapErrorf(err, "Chmoding binaries for '%s'", jobDir)
141145
}
142146

143147
installedJob = NewInstalledJob(renderedJobRef, jobDir)

‎installation/installer_test.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,43 @@ var _ = Describe("Installer", func() {
140140
})
141141
})
142142

143+
Describe("Install path traversal protection", func() {
144+
var fakeStage *fakebiui.FakeStage
145+
146+
BeforeEach(func() {
147+
fakeStage = fakebiui.NewFakeStage()
148+
})
149+
150+
It("returns error when a compiled package name contains path traversal", func() {
151+
maliciousRef := CompiledPackageRef{
152+
Name: "../../path",
153+
Version: "v1",
154+
BlobstoreID: "bid",
155+
SHA1: "sha",
156+
}
157+
releaseJobs := []bireljob.Job{}
158+
mockJobResolver.EXPECT().From(installationManifest).Return(releaseJobs, nil)
159+
mockPackageCompiler.EXPECT().For(releaseJobs, fakeStage).Return([]CompiledPackageRef{maliciousRef}, nil)
160+
161+
_, err := installer.Install(installationManifest, fakeStage)
162+
Expect(err).To(HaveOccurred())
163+
Expect(err.Error()).To(ContainSubstring("safe local path"))
164+
})
165+
166+
It("returns error when a rendered job name contains path traversal", func() {
167+
releaseJobs := []bireljob.Job{}
168+
compiledPackages := []CompiledPackageRef{}
169+
jobRef := NewRenderedJobRef("../../path", "fp", "blob-id", "sha")
170+
mockJobResolver.EXPECT().From(installationManifest).Return(releaseJobs, nil)
171+
mockPackageCompiler.EXPECT().For(releaseJobs, fakeStage).Return(compiledPackages, nil)
172+
mockJobRenderer.EXPECT().RenderAndUploadFrom(installationManifest, releaseJobs, fakeStage).Return([]RenderedJobRef{jobRef}, nil)
173+
174+
_, err := installer.Install(installationManifest, fakeStage)
175+
Expect(err).To(HaveOccurred())
176+
Expect(err.Error()).To(ContainSubstring("safe local path"))
177+
})
178+
})
179+
143180
Describe("Cleanup", func() {
144181
var installation Installation
145182

‎releasedir/fs_blobs_dir.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/cloudfoundry/bosh-utils/work"
1717
"gopkg.in/yaml.v2"
1818

19+
util "github.com/cloudfoundry/bosh-cli/v7/common/util"
1920
bicrypto "github.com/cloudfoundry/bosh-cli/v7/crypto"
2021
)
2122

@@ -101,6 +102,9 @@ func (d FSBlobsDir) Blobs() ([]Blob, error) {
101102
var blobs []Blob
102103

103104
for recPath, rec := range schema {
105+
if _, err := util.SafeJoinPath(d.dirPath, recPath); err != nil {
106+
return nil, bosherr.Errorf("Invalid blob path '%s': must be a safe local path", recPath)
107+
}
104108
blobs = append(blobs, Blob{
105109
Path: recPath,
106110
Size: rec.Size,
@@ -193,6 +197,11 @@ func (d FSBlobsDir) removeUnknownBlobs(blobs []Blob) error {
193197
}
194198

195199
func (d FSBlobsDir) TrackBlob(path string, src io.ReadCloser) (Blob, error) {
200+
safePath, err := util.SafeJoinPath(d.dirPath, path)
201+
if err != nil {
202+
return Blob{}, bosherr.Errorf("Invalid blob path '%s': must be a safe local path", path)
203+
}
204+
196205
tempFile, err := d.fs.TempFile("track-blob")
197206
if err != nil {
198207
return Blob{}, bosherr.WrapErrorf(err, "Creating temp blob")
@@ -239,7 +248,7 @@ func (d FSBlobsDir) TrackBlob(path string, src io.ReadCloser) (Blob, error) {
239248

240249
tempFile.Close() //nolint:errcheck
241250

242-
err = d.moveBlobLocally(tempFile.Name(), filepath.Join(d.dirPath, path))
251+
err = d.moveBlobLocally(tempFile.Name(), safePath)
243252
if err != nil {
244253
return Blob{}, err
245254
}
@@ -248,12 +257,17 @@ func (d FSBlobsDir) TrackBlob(path string, src io.ReadCloser) (Blob, error) {
248257
}
249258

250259
func (d FSBlobsDir) UntrackBlob(path string) error {
260+
safePath, err := util.SafeJoinPath(d.dirPath, path)
261+
if err != nil {
262+
return bosherr.Errorf("Invalid blob path '%s': must be a safe local path", path)
263+
}
264+
251265
blobs, err := d.Blobs()
252266
if err != nil {
253267
return err
254268
}
255269

256-
err = d.fs.RemoveAll(filepath.Join(d.dirPath, path))
270+
err = d.fs.RemoveAll(safePath)
257271
if err != nil {
258272
return bosherr.WrapErrorf(err, "Removing blob from blobs/")
259273
}

‎releasedir/fs_blobs_dir_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,34 @@ file2.tgz:
106106
Expect(err).To(HaveOccurred())
107107
Expect(err.Error()).To(ContainSubstring("Unmarshalling blobs index"))
108108
})
109+
110+
It("returns error for a blob path that escapes the blobs directory via ..", func() {
111+
err := fs.WriteFileString(filepath.Join("/", "dir", "config", "blobs.yml"), `
112+
../../../../home/file:
113+
size: 100
114+
object_id: id
115+
sha: sha
116+
`)
117+
Expect(err).ToNot(HaveOccurred())
118+
119+
_, err = act()
120+
Expect(err).To(HaveOccurred())
121+
Expect(err.Error()).To(ContainSubstring("safe local path"))
122+
})
123+
124+
It("returns error for an absolute blob path", func() {
125+
err := fs.WriteFileString(filepath.Join("/", "dir", "config", "blobs.yml"), `
126+
/etc/file:
127+
size: 100
128+
object_id: id
129+
sha: sha
130+
`)
131+
Expect(err).ToNot(HaveOccurred())
132+
133+
_, err = act()
134+
Expect(err).To(HaveOccurred())
135+
Expect(err.Error()).To(ContainSubstring("safe local path"))
136+
})
109137
})
110138

111139
Describe("SyncBlobs", func() {
@@ -600,6 +628,20 @@ file2.tgz:
600628

601629
Expect(blobsDir.Blobs()).To(BeEmpty())
602630
})
631+
632+
It("returns error for a blob path that escapes the blobs directory via ..", func() {
633+
content := io.NopCloser(strings.NewReader("content"))
634+
_, err := blobsDir.TrackBlob("../../etc/file", content)
635+
Expect(err).To(HaveOccurred())
636+
Expect(err.Error()).To(ContainSubstring("safe local path"))
637+
})
638+
639+
It("returns error for an absolute blob path", func() {
640+
content := io.NopCloser(strings.NewReader("content"))
641+
_, err := blobsDir.TrackBlob("/etc/file", content)
642+
Expect(err).To(HaveOccurred())
643+
Expect(err.Error()).To(ContainSubstring("safe local path"))
644+
})
603645
})
604646

605647
Describe("UntrackBlob", func() {
@@ -714,6 +756,18 @@ bosh-116.tgz:
714756
{Path: filepath.Join("dir", "file.tgz"), Size: 133, SHA1: "13e"},
715757
}))
716758
})
759+
760+
It("returns error for a blob path that escapes the blobs directory via ..", func() {
761+
err := blobsDir.UntrackBlob("../../etc/file")
762+
Expect(err).To(HaveOccurred())
763+
Expect(err.Error()).To(ContainSubstring("safe local path"))
764+
})
765+
766+
It("returns error for an absolute blob path", func() {
767+
err := blobsDir.UntrackBlob("/etc/file")
768+
Expect(err).To(HaveOccurred())
769+
Expect(err.Error()).To(ContainSubstring("safe local path"))
770+
})
717771
})
718772

719773
Describe("UploadBlobs", func() {

‎releasedir/index/fs_index_blobs.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package index
33
import (
44
"fmt"
55
"os"
6-
"path/filepath"
76
"runtime"
87
"strings"
98

@@ -12,6 +11,8 @@ import (
1211
bosherr "github.com/cloudfoundry/bosh-utils/errors"
1312
boshfu "github.com/cloudfoundry/bosh-utils/fileutil"
1413
boshsys "github.com/cloudfoundry/bosh-utils/system"
14+
15+
util "github.com/cloudfoundry/bosh-cli/v7/common/util"
1516
)
1617

1718
type FSIndexBlobs struct {
@@ -141,5 +142,10 @@ func (c FSIndexBlobs) blobPath(sha1 string) (string, error) {
141142
if runtime.GOOS == "windows" {
142143
sha1 = strings.ReplaceAll(sha1, ":", "_")
143144
}
144-
return filepath.Join(absDirPath, sha1), nil
145+
146+
candidatePath, err := util.SafeJoinPath(absDirPath, sha1)
147+
if err != nil {
148+
return "", bosherr.Errorf("Invalid blob SHA1 '%s': must be a safe local path", sha1)
149+
}
150+
return candidatePath, nil
145151
}

‎releasedir/index/fs_index_blobs_test.go‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"errors"
55
"os"
66
"path/filepath"
7-
"strings"
87
"syscall"
98

109
fakeblob "github.com/cloudfoundry/bosh-utils/blobstore/fakes"
@@ -115,6 +114,12 @@ var _ = Describe("FSIndexBlobs", func() {
115114
Expect(err).To(HaveOccurred())
116115
Expect(err.Error()).To(Equal("Cannot find blob named 'name' with SHA1 'sha1'"))
117116
})
117+
118+
It("returns error if sha1 is a path", func() {
119+
_, err := blobs.Get("name", "blob-id", "../../file")
120+
Expect(err).To(HaveOccurred())
121+
Expect(err.Error()).To(ContainSubstring("safe local path"))
122+
})
118123
})
119124

120125
Context("when configured with a blobstore", func() {
@@ -160,12 +165,10 @@ var _ = Describe("FSIndexBlobs", func() {
160165
Expect(reporter.IndexEntryDownloadFinishedCallCount()).To(Equal(1))
161166
})
162167

163-
It("returns error if parsing digest string fails", func() {
164-
//currently, the only way to cause a digest parse failure is with an empty string
168+
It("returns error if sha1 is empty", func() {
165169
_, err := blobs.Get("name", "blob-id", "")
166170
Expect(err).To(HaveOccurred())
167-
Expect(strings.ToLower(err.Error())).To(ContainSubstring(
168-
"no digest algorithm found. supported algorithms: sha1, sha256, sha512"))
171+
Expect(err.Error()).To(ContainSubstring("safe local path"))
169172
})
170173

171174
Context("when downloading blob fails", func() {
@@ -284,6 +287,24 @@ var _ = Describe("FSIndexBlobs", func() {
284287
Expect(err).ToNot(HaveOccurred())
285288
})
286289

290+
Context("when sha1 is a path", func() {
291+
BeforeEach(func() {
292+
blobs = boshidx.NewFSIndexBlobs(filepath.Join("/", "dir", "sub-dir"), reporter, nil, fs)
293+
})
294+
295+
It("returns error for a path with ..", func() {
296+
_, _, err := blobs.Add("name", filepath.Join("/", "tmp", "payload"), "../../.file")
297+
Expect(err).To(HaveOccurred())
298+
Expect(err.Error()).To(ContainSubstring("safe local path"))
299+
})
300+
301+
It("returns error for an absolute path sha1", func() {
302+
_, _, err := blobs.Add("name", filepath.Join("/", "tmp", "payload"), "/etc/file")
303+
Expect(err).To(HaveOccurred())
304+
Expect(err.Error()).To(ContainSubstring("safe local path"))
305+
})
306+
})
307+
287308
itCopiesFileIntoDir := func() {
288309
It("copies file into cache dir", func() {
289310
blobID, path, err := blobs.Add("name", filepath.Join("/", "tmp", "sha1"), "sha1")

0 commit comments

Comments
 (0)