Merge pull request #9467 from icecrime/9401-restrict_add_chown_scope

Reduce permissions changes scope after ADD/COPY

builder/internals.go

@@ -615,7 +615,7 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec
 	}
 
 	if fi.IsDir() {
-		return copyAsDirectory(origPath, destPath, destExists)
+		return copyAsDirectory(origPath, destPath)
 	}
 
 	// If we are adding a remote file (or we've been told not to decompress), do not try to untar it
@@ -649,37 +649,31 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec
 		resPath = path.Join(destPath, path.Base(origPath))
 	}
 
-	return fixPermissions(resPath, 0, 0)
+	return fixPermissions(origPath, resPath, 0, 0)
 }
 
-func copyAsDirectory(source, destination string, destinationExists bool) error {
+func copyAsDirectory(source, destination string) error {
 	if err := chrootarchive.CopyWithTar(source, destination); err != nil {
 		return err
 	}
+	return fixPermissions(source, destination, 0, 0)
+}
 
-	if destinationExists {
+func fixPermissions(source, destination string, uid, gid int) error {
-		files, err := ioutil.ReadDir(source)
+	// We Walk on the source rather than on the destination because we don't
+	// want to change permissions on things we haven't created or modified.
+	return filepath.Walk(source, func(fullpath string, info os.FileInfo, err error) error {
+		// Do not alter the walk root itself as it potentially existed before.
+		if source == fullpath {
+			return nil
+		}
+		// Path is prefixed by source: substitute with destination instead.
+		cleaned, err := filepath.Rel(source, fullpath)
 		if err != nil {
 			return err
 		}
-
+		fullpath = path.Join(destination, cleaned)
-		for _, file := range files {
+		return os.Lchown(fullpath, uid, gid)
-			if err := fixPermissions(filepath.Join(destination, file.Name()), 0, 0); err != nil {
-				return err
-			}
-		}
-		return nil
-	}
-
-	return fixPermissions(destination, 0, 0)
-}
-
-func fixPermissions(destination string, uid, gid int) error {
-	return filepath.Walk(destination, func(path string, info os.FileInfo, err error) error {
-		if err := os.Lchown(path, uid, gid); err != nil && !os.IsNotExist(err) {
-			return err
-		}
-		return nil
 	})
 }
 

integration-cli/docker_cli_build_test.go

@@ -1066,6 +1066,31 @@ ADD . /`,
 	logDone("build - add etc directory to root")
 }
 
+// Testing #9401
+func TestBuildAddPreservesFilesSpecialBits(t *testing.T) {
+	name := "testaddpreservesfilesspecialbits"
+	defer deleteImages(name)
+	ctx, err := fakeContext(`FROM busybox
+ADD suidbin /usr/bin/suidbin
+RUN chmod 4755 /usr/bin/suidbin
+RUN [ $(ls -l /usr/bin/suidbin | awk '{print $1}') = '-rwsr-xr-x' ]
+ADD ./data/ /
+RUN [ $(ls -l /usr/bin/suidbin | awk '{print $1}') = '-rwsr-xr-x' ]`,
+		map[string]string{
+			"suidbin":             "suidbin",
+			"/data/usr/test_file": "test1",
+		})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer ctx.Close()
+
+	if _, err := buildImageFromContext(name, ctx, true); err != nil {
+		t.Fatal(err)
+	}
+	logDone("build - add preserves files special bits")
+}
+
 func TestBuildCopySingleFileToRoot(t *testing.T) {
 	name := "testcopysinglefiletoroot"
 	defer deleteImages(name)