Skip to content

Commit

Permalink
Fix issue with non-writable sub-directories
Browse files Browse the repository at this point in the history
There are file system setups where there is a file in a directory, but
the directory does not have the write bit set (anymore). This kind of
setup is perfect fine, however, the bundle logic is unable to unpack it.

Fix unpack logic so that it is not setting the file permissions of
directories in the moment it creates the directory, but later when all
other files are unpacked already and no more write operations are
required.

Signed-off-by: Matthias Diester <[email protected]>
  • Loading branch information
HeavyWombat committed Oct 28, 2024
1 parent a2b53df commit 89d56f0
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 2 deletions.
31 changes: 30 additions & 1 deletion pkg/bundle/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,12 +231,34 @@ func Pack(directory string) (io.ReadCloser, error) {
// Unpack reads a tar stream and writes the content into the local file system
// with all files and directories.
func Unpack(in io.Reader, targetPath string) (*UnpackDetails, error) {
type chmod struct {
name string
mode os.FileMode
}

// Make sure the target path exists and is a directory
if stat, err := os.Stat(targetPath); err != nil {
if err := os.MkdirAll(targetPath, os.FileMode(0755)); err != nil {
return nil, err
}
} else if !stat.IsDir() {
return nil, fmt.Errorf("target %q exists, but it's not a directory", targetPath)
}

var chmods []chmod
var details = UnpackDetails{}
var tr = tar.NewReader(in)
for {
header, err := tr.Next()

Check failure

Code scanning / CodeQL

Arbitrary file access during archive extraction ("Zip Slip") High

Unsanitized archive entry, which may contain '..', is used in a
file system operation
.
Unsanitized archive entry, which may contain '..', is used in a
file system operation
.
Unsanitized archive entry, which may contain '..', is used in a
file system operation
.
Unsanitized archive entry, which may contain '..', is used in a
file system operation
.
Unsanitized archive entry, which may contain '..', is used in a
file system operation
.
switch {
case err == io.EOF:
// before leaving, make sure to set the file permissions to the ones specified in the tar stream
for _, chmod := range chmods {
if err := os.Chmod(chmod.name, chmod.mode); err != nil {
return nil, err
}
}

return &details, nil

case err != nil:
Expand All @@ -253,10 +275,17 @@ func Unpack(in io.Reader, targetPath string) (*UnpackDetails, error) {

switch header.Typeflag {
case tar.TypeDir:
if err := os.MkdirAll(target, fileMode(header)); err != nil {
// Skip the root directory, since it already exists
if target == targetPath {
continue
}

if err := os.MkdirAll(target, os.FileMode(0777)); err != nil {
return nil, err
}

chmods = append(chmods, chmod{name: target, mode: fileMode(header)})

case tar.TypeReg:
// Edge case in which that tarball did not have a directory entry
dir, _ := filepath.Split(target)
Expand Down
23 changes: 22 additions & 1 deletion pkg/bundle/bundle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,36 @@ var _ = Describe("Bundle", func() {
Expect(r).ToNot(BeNil())

details, err := Unpack(r, tempDir)
Expect(details).ToNot(BeNil())
Expect(err).ToNot(HaveOccurred())
Expect(details).ToNot(BeNil())

Expect(filepath.Join(tempDir, "README.md")).To(BeAnExistingFile())
Expect(filepath.Join(tempDir, ".someToolDir", "config.json")).ToNot(BeAnExistingFile())
Expect(filepath.Join(tempDir, "somefile")).To(BeAnExistingFile())
Expect(filepath.Join(tempDir, "linktofile")).To(BeAnExistingFile())
})
})

It("should pack and unpack a directory including all files even if a sub-directory is not writable", func() {
withTempDir(func(source string) {
// Setup a case where there are files in a directory, which is not writtable
Expect(os.Mkdir(filepath.Join(source, "some-dir"), os.FileMode(0777))).To(Succeed())
Expect(os.WriteFile(filepath.Join(source, "some-dir", "some-file"), []byte(`foobar`), os.FileMode(0644))).To(Succeed())
Expect(os.Chmod(filepath.Join(source, "some-dir"), os.FileMode(0555))).To(Succeed())

r, err := Pack(source)
Expect(err).ToNot(HaveOccurred())
Expect(r).ToNot(BeNil())

withTempDir(func(target string) {
details, err := Unpack(r, target)
Expect(err).ToNot(HaveOccurred())
Expect(details).ToNot(BeNil())

Expect(filepath.Join(target, "some-dir", "some-file")).To(BeAnExistingFile())
})
})
})
})

Context("packing/pushing and pulling/unpacking", func() {
Expand Down

0 comments on commit 89d56f0

Please sign in to comment.