From 28bfb75a3156bf785958f816b51e3bf4e365327a Mon Sep 17 00:00:00 2001
From: Daisuke Taniwaki <daisuketaniwaki@gmail.com>
Date: Tue, 19 Mar 2019 03:28:24 +0900
Subject: [PATCH] Fix file mode bug (#618)

* Fix file mode

* Add Dockerfile for special file permission test
---
 .../dockerfiles/Dockerfile_test_cache_perm    |  8 ++++
 integration/images.go                         |  1 +
 pkg/util/fs_util.go                           | 47 ++++++++++---------
 pkg/util/fs_util_test.go                      | 24 ++++++++++
 4 files changed, 59 insertions(+), 21 deletions(-)
 create mode 100644 integration/dockerfiles/Dockerfile_test_cache_perm

diff --git a/integration/dockerfiles/Dockerfile_test_cache_perm b/integration/dockerfiles/Dockerfile_test_cache_perm
new file mode 100644
index 000000000..06c311fe6
--- /dev/null
+++ b/integration/dockerfiles/Dockerfile_test_cache_perm
@@ -0,0 +1,8 @@
+# Test to make sure the cache works with special file permissions properly.
+# If the image is built twice, directory foo should have the sticky bit,
+# and file bar should have the setuid and setgid bits.
+
+FROM busybox
+
+RUN mkdir foo && chmod +t foo
+RUN touch bar && chmod u+s,g+s bar
diff --git a/integration/images.go b/integration/images.go
index 27d54501c..2eb00be60 100644
--- a/integration/images.go
+++ b/integration/images.go
@@ -134,6 +134,7 @@ func NewDockerFileBuilder(dockerfiles []string) *DockerFileBuilder {
 	d.TestCacheDockerfiles = map[string]struct{}{
 		"Dockerfile_test_cache":         {},
 		"Dockerfile_test_cache_install": {},
+		"Dockerfile_test_cache_perm":    {},
 	}
 	return &d
 }
diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go
index 38b25bc51..5ef48845f 100644
--- a/pkg/util/fs_util.go
+++ b/pkg/util/fs_util.go
@@ -216,27 +216,16 @@ func extractFile(dest string, hdr *tar.Header, tr io.Reader) error {
 		if err != nil {
 			return err
 		}
-		// manually set permissions on file, since the default umask (022) will interfere
-		if err = os.Chmod(path, mode); err != nil {
-			return err
-		}
 		if _, err = io.Copy(currFile, tr); err != nil {
 			return err
 		}
-		if err = currFile.Chown(uid, gid); err != nil {
+		if err = setFilePermissions(path, mode, uid, gid); err != nil {
 			return err
 		}
 		currFile.Close()
 	case tar.TypeDir:
 		logrus.Debugf("creating dir %s", path)
-		if err := os.MkdirAll(path, mode); err != nil {
-			return err
-		}
-		// In some cases, MkdirAll doesn't change the permissions, so run Chmod
-		if err := os.Chmod(path, mode); err != nil {
-			return err
-		}
-		if err := os.Chown(path, uid, gid); err != nil {
+		if err := mkdirAllWithPermissions(path, mode, uid, gid); err != nil {
 			return err
 		}
 
@@ -429,10 +418,7 @@ func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid
 	if _, err := io.Copy(dest, reader); err != nil {
 		return err
 	}
-	if err := dest.Chmod(perm); err != nil {
-		return err
-	}
-	return dest.Chown(int(uid), int(gid))
+	return setFilePermissions(path, perm, int(uid), int(gid))
 }
 
 // AddVolumePath adds the given path to the volume whitelist.
@@ -492,13 +478,11 @@ func CopyDir(src, dest, buildcontext string) ([]string, error) {
 		if fi.IsDir() {
 			logrus.Debugf("Creating directory %s", destPath)
 
+			mode := fi.Mode()
 			uid := int(fi.Sys().(*syscall.Stat_t).Uid)
 			gid := int(fi.Sys().(*syscall.Stat_t).Gid)
 
-			if err := os.MkdirAll(destPath, fi.Mode()); err != nil {
-				return nil, err
-			}
-			if err := os.Chown(destPath, uid, gid); err != nil {
+			if err := mkdirAllWithPermissions(destPath, mode, uid, gid); err != nil {
 				return nil, err
 			}
 		} else if fi.Mode()&os.ModeSymlink != 0 {
@@ -614,3 +598,24 @@ func HasFilepathPrefix(path, prefix string, prefixMatchOnly bool) bool {
 func Volumes() []string {
 	return volumes
 }
+
+func mkdirAllWithPermissions(path string, mode os.FileMode, uid, gid int) error {
+	if err := os.MkdirAll(path, mode); err != nil {
+		return err
+	}
+	if err := os.Chown(path, uid, gid); err != nil {
+		return err
+	}
+	// In some cases, MkdirAll doesn't change the permissions, so run Chmod
+	// Must chmod after chown because chown resets the file mode.
+	return os.Chmod(path, mode)
+}
+
+func setFilePermissions(path string, mode os.FileMode, uid, gid int) error {
+	if err := os.Chown(path, uid, gid); err != nil {
+		return err
+	}
+	// manually set permissions on file, since the default umask (022) will interfere
+	// Must chmod after chown because chown resets the file mode.
+	return os.Chmod(path, mode)
+}
diff --git a/pkg/util/fs_util_test.go b/pkg/util/fs_util_test.go
index 4f91343f8..eff39d5d9 100644
--- a/pkg/util/fs_util_test.go
+++ b/pkg/util/fs_util_test.go
@@ -503,6 +503,30 @@ func TestExtractFile(t *testing.T) {
 				filesAreHardlinks("/bin/uncompress", "/bin/gzip"),
 			},
 		},
+		{
+			name:     "file with setuid bit",
+			contents: []byte("helloworld"),
+			hdrs:     []*tar.Header{fileHeader("./bar", "helloworld", 04644)},
+			checkers: []checker{
+				fileExists("/bar"),
+				fileMatches("/bar", []byte("helloworld")),
+				permissionsMatch("/bar", 0644|os.ModeSetuid),
+			},
+		},
+		{
+			name:     "dir with sticky bit",
+			contents: []byte("helloworld"),
+			hdrs: []*tar.Header{
+				dirHeader("./foo", 01755),
+				fileHeader("./foo/bar", "helloworld", 0644),
+			},
+			checkers: []checker{
+				fileExists("/foo/bar"),
+				fileMatches("/foo/bar", []byte("helloworld")),
+				permissionsMatch("/foo/bar", 0644),
+				permissionsMatch("/foo", 0755|os.ModeDir|os.ModeSticky),
+			},
+		},
 	}
 
 	for _, tc := range tcs {