Ensure umask is unset when extracting archive (#727)

* Ensure umask is unset when extracting archive Signed-off-by: Natalie Arellano <[email protected]> * Add test Signed-off-by: Natalie Arellano <[email protected]> * Fix Signed-off-by: Natalie Arellano <[email protected]> * Get the current umask without changing it Signed-off-by: Natalie Arellano <[email protected]> * Fix Signed-off-by: Natalie Arellano <[email protected]> * Fix windows Signed-off-by: Natalie Arellano <[email protected]> * Fix windows Signed-off-by: Natalie Arellano <[email protected]> * Update per review comments Signed-off-by: Natalie Arellano <[email protected]> * Less confusing wording Signed-off-by: Natalie Arellano <[email protected]> * Reduce the diff Signed-off-by: Natalie Arellano <[email protected]> * Fix Signed-off-by: Natalie Arellano <[email protected]> * Added comments Signed-off-by: Natalie Arellano <[email protected]> * Better wording Signed-off-by: Natalie Arellano <[email protected]> * Add test that system umask is used to create non existent directory not in tar file Signed-off-by: Natalie Arellano <[email protected]> * Variable names and formatting Signed-off-by: Natalie Arellano <[email protected]> * Try to fix windows Signed-off-by: Natalie Arellano <[email protected]> * Avoid direct dependency on archive Signed-off-by: Natalie Arellano <[email protected]> * Make test setup simpler and update comment Signed-off-by: Natalie Arellano <[email protected]> * Add build directive Signed-off-by: Natalie Arellano <[email protected]> * Apply suggestions from code review Signed-off-by: Natalie Arellano <[email protected]> * Fix Codecov Signed-off-by: Natalie Arellano <[email protected]> * Fix lint Signed-off-by: Natalie Arellano <[email protected]>
buildpacks · Sep 30, 2021 · 3051985 · 3051985
1 parent 0de4cff
commit 3051985
Show file tree

Hide file tree

Showing 10 changed files with 86 additions and 29 deletions.
diff --git a/archive/extract.go b/archive/extract.go
@@ -10,29 +10,23 @@ import (
 	"github.com/pkg/errors"
 )
 
-var systemUmask int
-
-func init() {
-	// Avoid umask from changing the file permissions in the tar file.
-	systemUmask = setUmask(0)
-	_ = setUmask(systemUmask)
-}
-
 type PathMode struct {
 	Path string
 	Mode os.FileMode
 }
 
-// Extract reads all entries from TarReader and extracts them to the filesystem
-func Extract(tr TarReader) error {
+// Extract reads all entries from TarReader and extracts them to the filesystem.
+// The umask must be unset before calling this function on unix, to ensure that files have the correct file mode. SetUmask can be used to set and unset the umask.
+// The provided umask will be applied to new directories that are created as parent directories of files in the tar, that do not themselves have headers in the tar.
+func Extract(tr TarReader, procUmask int) error {
 	buf := make([]byte, 32*32*1024)
 	dirsFound := make(map[string]bool)
 
 	var pathModes []PathMode
 	for {
 		hdr, err := tr.Next()
 		if err == io.EOF {
-			for _, pathMode := range pathModes {
+			for _, pathMode := range pathModes { // directories that are newly created and for which there is a header in the tar should have the right permissions
 				if err := os.Chmod(pathMode.Path, pathMode.Mode); err != nil {
 					return err
 				}
@@ -58,7 +52,7 @@ func Extract(tr TarReader) error {
 			dirPath := filepath.Dir(hdr.Name)
 			if !dirsFound[dirPath] {
 				if _, err := os.Stat(dirPath); os.IsNotExist(err) {
-					if err := os.MkdirAll(dirPath, applyUmask(os.ModePerm, systemUmask)); err != nil {
+					if err := os.MkdirAll(dirPath, applyUmask(os.ModePerm, procUmask)); err != nil { // if there is no header for the parent directory in the tar, apply the provided umask
 						return errors.Wrapf(err, "failed to create parent dir %q for file %q", dirPath, hdr.Name)
 					}
 					dirsFound[dirPath] = true

diff --git a/archive/extract_test.go b/archive/extract_test.go
@@ -50,6 +50,7 @@ func testExtract(t *testing.T, when spec.G, it spec.S) {
 			{"root/readonly/readonlysub/somefile", 0444},
 			{"root/standarddir", os.ModeDir + 0755},
 			{"root/standarddir/somefile", 0644},
+			{"root/nonexistdirnotintar", os.ModeDir + os.FileMode(int(os.ModePerm)&^h.ProvidedUmask)},
 			{"root/symlinkdir", os.ModeSymlink + 0777},  //symlink permissions are not preserved from archive
 			{"root/symlinkfile", os.ModeSymlink + 0777}, //symlink permissions are not preserved from archive
 		}
@@ -63,6 +64,7 @@ func testExtract(t *testing.T, when spec.G, it spec.S) {
 				{`root\readonly\readonlysub\somefile`, 0444},
 				{`root\standarddir`, os.ModeDir + 0777},
 				{`root\standarddir\somefile`, 0666},
+				{`root\nonexistdirnotintar`, os.ModeDir + 0777},
 				{`root\symlinkdir`, os.ModeSymlink + 0666},
 				{`root\symlinkfile`, os.ModeSymlink + 0666},
 			}
@@ -81,6 +83,11 @@ func testExtract(t *testing.T, when spec.G, it spec.S) {
 				Linkname: filepath.FromSlash("../not-in-archive-file"),
 				Mode:     int64(os.ModeSymlink | 0755),
 			})
+			ftr.pushHeader(&tar.Header{
+				Name:     "root/nonexistdirnotintar/somefile",
+				Typeflag: tar.TypeReg,
+				Mode:     int64(0644),
+			})
 			ftr.pushHeader(&tar.Header{
 				Name:     "root/standarddir/somefile",
 				Typeflag: tar.TypeReg,
@@ -129,32 +136,36 @@ func testExtract(t *testing.T, when spec.G, it spec.S) {
 		})
 
 		it("extracts a tar file", func() {
-			h.AssertNil(t, archive.Extract(tr))
-
-			for _, pathMode := range pathModes {
-				extractedFile := filepath.Join(tmpDir, pathMode.Path)
+			oldUmask := archive.SetUmask(0)
+			defer archive.SetUmask(oldUmask)
 
-				fileInfo, err := os.Lstat(extractedFile)
-				h.AssertNil(t, err)
+			h.AssertNil(t, archive.Extract(tr, h.ProvidedUmask))
 
-				h.AssertEq(t, fileInfo.Mode(), pathMode.Mode)
+			for _, pathMode := range pathModes {
+				testPathPerms(t, tmpDir, pathMode.Path, pathMode.Mode)
 			}
 		})
 
 		it("fails if file exists where directory needs to be created", func() {
+			oldUmask := archive.SetUmask(0)
+			defer archive.SetUmask(oldUmask)
+
 			file, err := os.Create(filepath.Join(tmpDir, "root"))
 			h.AssertNil(t, err)
 			h.AssertNil(t, file.Close())
 
-			h.AssertError(t, archive.Extract(tr), "failed to create directory")
+			h.AssertError(t, archive.Extract(tr, oldUmask), "failed to create directory")
 		})
 
 		it("doesn't alter permissions of existing folders", func() {
+			oldUmask := archive.SetUmask(0)
+			defer archive.SetUmask(oldUmask)
+
 			h.AssertNil(t, os.Mkdir(filepath.Join(tmpDir, "root"), 0744))
 			// Update permissions in case umask was applied.
 			h.AssertNil(t, os.Chmod(filepath.Join(tmpDir, "root"), 0744))
 
-			h.AssertNil(t, archive.Extract(tr))
+			h.AssertNil(t, archive.Extract(tr, oldUmask))
 			fileInfo, err := os.Stat(filepath.Join(tmpDir, "root"))
 			h.AssertNil(t, err)
 
@@ -167,3 +178,12 @@ func testExtract(t *testing.T, when spec.G, it spec.S) {
 		})
 	})
 }
+
+func testPathPerms(t *testing.T, parentDir, path string, expectedMode os.FileMode) {
+	extractedFile := filepath.Join(parentDir, path)
+
+	fileInfo, err := os.Lstat(extractedFile)
+	h.AssertNil(t, err)
+
+	h.AssertEq(t, fileInfo.Mode(), expectedMode)
+}
diff --git a/archive/tar_unix.go b/archive/tar_unix.go
@@ -9,7 +9,7 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-func setUmask(newMask int) (oldMask int) {
+func SetUmask(newMask int) (oldMask int) {
 	return unix.Umask(newMask)
 }
 

diff --git a/archive/tar_windows.go b/archive/tar_windows.go
@@ -17,7 +17,7 @@ const (
 	hdrFileAttributes  = hdrMSWindowsPrefix + "fileattr"
 )
 
-func setUmask(newMask int) (oldMask int) {
+func SetUmask(newMask int) (oldMask int) {
 	// Not implemented on Windows
 	return 0
 }

diff --git a/archive/tar_windows_test.go b/archive/tar_windows_test.go
@@ -62,7 +62,7 @@ func testTarWindows(t *testing.T, when spec.G, it spec.S) {
 		})
 
 		it("sets dir attribute on windows directory symlinks", func() {
-			h.AssertNil(t, archive.Extract(tr))
+			h.AssertNil(t, archive.Extract(tr, 0))
 
 			extractedFile := filepath.Join(tmpDir, "root", "symlinkdir")
 			t.Log("asserting on", extractedFile)

diff --git a/layers/extract.go b/layers/extract.go
@@ -11,9 +11,15 @@ import (
 // Extract extracts entries from r to the dest directory
 // Contents of r should be an OCI layer.
 // If dest is an empty string files with be extracted to `/` or `c:\` on unix and windows filesystems respectively.
-func Extract(r io.Reader, dest string) error {
+// The umask must be unset before calling this function on unix, to ensure that files have the correct file mode. SetUmask can be used to set and unset the umask.
+// The provided umask will be applied to new directories that are created as parent directories of files in the tar, that do not themselves have headers in the tar.
+func Extract(r io.Reader, dest string, procUmask int) error {
 	tr := tarReader(r, dest)
-	return archive.Extract(tr)
+	return archive.Extract(tr, procUmask)
+}
+
+func SetUmask(newMask int) (oldMask int) {
+	return archive.SetUmask(newMask)
 }
 
 func tarReader(r io.Reader, dest string) archive.TarReader {

diff --git a/layers/extract_test.go b/layers/extract_test.go
@@ -0,0 +1,25 @@
+package layers_test
+
+import (
+	"testing"
+
+	"github.com/sclevine/spec"
+	"github.com/sclevine/spec/report"
+
+	"github.com/buildpacks/lifecycle/layers"
+	h "github.com/buildpacks/lifecycle/testhelpers"
+)
+
+func TestExtract(t *testing.T) {
+	spec.Run(t, "Extract", testExtract, spec.Parallel(), spec.Report(report.Terminal{}))
+}
+
+func testExtract(t *testing.T, when spec.G, it spec.S) {
+	when("#SetUmask", func() {
+		it("returns the old umask", func() {
+			first := layers.SetUmask(0)
+			second := layers.SetUmask(first)
+			h.AssertEq(t, second, 0)
+		})
+	})
+}
diff --git a/restorer.go b/restorer.go
@@ -39,6 +39,10 @@ func (r *Restorer) Restore(cache Cache) error {
 		}
 	}
 
+	// Avoid umask from changing the file permissions in the tar file.
+	previousUmask := layers.SetUmask(0)
+	defer layers.SetUmask(previousUmask)
+
 	var g errgroup.Group
 	for _, buildpack := range r.Buildpacks {
 		cachedLayers := cacheMeta.MetadataForBuildpack(buildpack.ID).Layers
@@ -89,7 +93,7 @@ func (r *Restorer) Restore(cache Cache) error {
 			} else {
 				r.Logger.Infof("Restoring data for %q from cache", bpLayer.Identifier())
 				g.Go(func() error {
-					return r.restoreLayer(cache, cachedLayer.SHA)
+					return r.restoreLayer(cache, cachedLayer.SHA, previousUmask)
 				})
 			}
 		}
@@ -104,7 +108,7 @@ func (r *Restorer) restoresLayerMetadata() bool {
 	return api.MustParse(r.Platform.API()).AtLeast("0.7")
 }
 
-func (r *Restorer) restoreLayer(cache Cache, sha string) error {
+func (r *Restorer) restoreLayer(cache Cache, sha string, procUmask int) error {
 	// Sanity check to prevent panic.
 	if cache == nil {
 		return errors.New("restoring layer: cache not provided")
@@ -116,5 +120,5 @@ func (r *Restorer) restoreLayer(cache Cache, sha string) error {
 	}
 	defer rc.Close()
 
-	return layers.Extract(rc, "")
+	return layers.Extract(rc, "", procUmask)
 }
diff --git a/testhelpers/vars_unix.go b/testhelpers/vars_unix.go
@@ -0,0 +1,5 @@
+//+build linux darwin
+
+package testhelpers
+
+const ProvidedUmask = 0022
diff --git a/testhelpers/vars_windows.go b/testhelpers/vars_windows.go
@@ -0,0 +1,3 @@
+package testhelpers
+
+const ProvidedUmask = 0000
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		package testhelpers

		const ProvidedUmask = 0000