CVE-2024-7387: Prevent Build Inputs "Zip-Slip"

BuildConfig-driven builds allow developers to specify additional
`Secrets` and/or `ConfigMaps` whose file contents can be referenced
during the build. For `Docker` strategy builds, this is accoplished
by copying the file contents of the `Secret` or `ConfigMap` directly
to a path within the build's source code using the `cp` Linux command.

Previously, an attacker could maliciously craft their source code and
build `Secrets` or `ConfigMaps` in such a way that they could overwrite
the `cp` command and execute arbitrary code. Due to the way build pods
are constructed, these commands then run as root in a privileged
container, with full Linux capabilies and `unconfined` Seccomp policy.
An attacker can then escalate their privileges through multiple attack
vectors - for example, by obtaining the credentials of the host node's
kubelet.

This change blocks the primary attack vector by requiring the
ultimate destination of the referenced `Secret` or `ConfigMap` to be a
child directory of the source code root, thereby preventing the
overwrite of the `cp` Linux command. This only applies to builds which
use the `Docker` strategy during execution. Builds which use the
`Source` or `Custom` build strategy are not affected.

This patch also uses methods that may be considered out of date or
deprecated in golang 1.22 or later. This is done deliberately to allow
clean cherrypicks to OpenShift 3.11 and other, older versions of
OpenShift 4.x.

Signed-off-by: Adam Kaplan <adam.kaplan@redhat.com>

pkg/build/builder/docker.go

@@ -238,14 +238,33 @@ func (d *DockerBuilder) copySecrets(secrets []buildapiv1.SecretBuildSource, targ
 }
 
 func (d *DockerBuilder) copyLocalObject(s localObjectBuildSource, sourceDir, targetDir string) error {
+	if !filepath.IsAbs(sourceDir) {
+		return fmt.Errorf("cannot copy local object - source directory %q must be an absolute path", sourceDir)
+
+	}
+	if !filepath.IsAbs(targetDir) {
+		return fmt.Errorf("cannot copy local object - target directory %q must be an absolute path", targetDir)
+	}
 	dstDir := filepath.Join(targetDir, s.DestinationPath())
 	if err := os.MkdirAll(dstDir, 0777); err != nil {
 		return err
 	}
+	// Evaluate symlinks at the destination dir. EvalSymlinks calls filepath.Clean, ensuring the
+	// returned path is an absolute path (we checked targetDir and thus dstDir is absolute above).
+	dstDir, err := filepath.EvalSymlinks(dstDir)
+	if err != nil {
+		return err
+	}
+	// sourceDir and targetDir should always be absolute paths, therefore HasPrefix can verify if
+	// dstDir is a subdirectory of targetDir.
+	if !strings.HasPrefix(dstDir, targetDir) {
+		return fmt.Errorf("destination path %q is outside of the target directory %q", dstDir, targetDir)
+	}
+
 	log.V(3).Infof("Copying files from the build source %q to %q", s.LocalObjectRef().Name, dstDir)
 
-	// Build sources contain nested directories and fairly baroque links. To prevent extra data being
-	// copied, perform the following steps:
+	// Build sources from Secrets or ConfigMaps contain nested directories and fairly baroque links.
+	// To prevent extra data being copied, perform the following steps:
 	//
 	// 1. Only top level files and directories within the secret directory are candidates
 	// 2. Any item starting with '..' is ignored

pkg/build/builder/docker_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -594,3 +595,215 @@ USER 1001`
 		}
 	}
 }
+
+// TestCopyLocalObject verifies that we are able to copy mounted Kubernetes Secret or ConfigMap
+// data to the build directory. The build directory is typically where git source code is cloned,
+// though other sources of code may be used as well.
+func TestCopyLocalObject(t *testing.T) {
+	// Test scenario consists of a file tree that simulates the git clone environment
+	// /tmp/source-code-xxxx
+	// ├── git-data
+	// │   ├── Dockerfile
+	// │   └── hello.txt
+	// │   └── linkSrc
+	// │       └── link -> ../linkDst
+	// |   └── linkDst
+	// ├── secrets
+	// │   └── test
+	// │       └── secret.txt
+	// TODO: Update the structure of the `secrets` directory so it more closely matches what
+	// Kubernetes does when it mounts secrets/configmaps.
+	buildDir, err := os.MkdirTemp("", "source-code")
+	t.Logf("buildDir: %s", buildDir)
+	if err != nil {
+		t.Fatalf("failed to create temp dir: %v", err)
+	}
+	defer func() {
+		if err := os.RemoveAll(buildDir); err != nil {
+			t.Fatalf("failed to clean up temp dir: %v", err)
+		}
+	}()
+	// Set up secrets/test directory, with "secret.txt" file
+	secretRoot := filepath.Join(buildDir, "secrets")
+	testSecretFile := filepath.Join(secretRoot, "test", "sourceSecret.txt")
+	err = fsCopyFile("testdata/fakesecret/sourceSecret.txt", testSecretFile)
+	if err != nil {
+		t.Fatalf("failed to copy secret data: %v", err)
+	}
+	if _, err := os.Stat(testSecretFile); err != nil {
+		t.Fatalf("failed to stat %q: %v", testSecretFile, err)
+	}
+	// Set up "git-data" directory as fake "git source"
+	gitRoot := filepath.Join(buildDir, "git-data")
+
+	err = filepath.Walk("testdata/fakesource", func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if info.IsDir() {
+			return nil
+		}
+		dstBase := filepath.Base(path)
+		return fsCopyFile(path, filepath.Join(gitRoot, dstBase))
+	})
+	if err != nil {
+		t.Fatalf("failed to copy test data: %v", err)
+	}
+	// Git source may contain symlinks. This is OK as long as the symlink is relative and remains
+	// within the git source tree.
+	linkSrc := filepath.Join(gitRoot, "linkSrc")
+	err = os.MkdirAll(linkSrc, os.ModePerm)
+	if err != nil {
+		t.Fatalf("failed to create test directory %q: %v", linkSrc, err)
+	}
+	linkDst := filepath.Join(gitRoot, "linkDst")
+	err = os.MkdirAll(linkDst, os.ModePerm)
+	if err != nil {
+		t.Fatalf("failed to create test directory %q: %v", linkDst, err)
+	}
+	// Create symlink from linkSrc/link to linkDst (directory)
+	err = os.Symlink("../linkDst", filepath.Join(linkSrc, "link"))
+	if err != nil {
+		t.Fatalf("failed to create symlink to linkDst directory: %v", err)
+	}
+	dockerBuilder := &DockerBuilder{
+		inputDir: buildDir,
+	}
+	localObj := buildapiv1.SecretBuildSource{
+		Secret: corev1.LocalObjectReference{
+			Name: "test",
+		},
+		DestinationDir: "secrets",
+	}
+	err = dockerBuilder.copyLocalObject(secretSource(localObj), secretRoot, gitRoot)
+	if err != nil {
+		t.Errorf("unexpected error copying local secret: %v", err)
+	}
+	// sourceSecret.txt should be copied to git-data/secrets/sourceSecret.txt
+	expectedFile := filepath.Join(gitRoot, "secrets", "sourceSecret.txt")
+	if _, err := os.Stat(expectedFile); err != nil {
+		t.Errorf("failed to stat %q: %v", expectedFile, err)
+	}
+	// Try again, this time copying to a destination that happens to be a symlink to another
+	// directory
+	localObj.DestinationDir = filepath.Join("linkSrc", "link")
+	err = dockerBuilder.copyLocalObject(secretSource(localObj), secretRoot, gitRoot)
+	if err != nil {
+		t.Errorf("unexpected error copying local secret: %v", err)
+	}
+	// sourceSecret.txt should be copied to git-data/linkSrc/link/sourceSecret.txt
+	expectedFile = filepath.Join(gitRoot, "linkSrc", "link", "sourceSecret.txt")
+	if _, err := os.Stat(expectedFile); err != nil {
+		t.Errorf("failed to stat %q: %v", expectedFile, err)
+	}
+	// sourceSecret.txt should also show up in git-data/linkDst/sourceSecret.txt
+	expectedFile = filepath.Join(gitRoot, "linkDst", "sourceSecret.txt")
+	if _, err := os.Stat(expectedFile); err != nil {
+		t.Errorf("failed to stat %q: %v", expectedFile, err)
+	}
+
+}
+
+// TestCopyLocalObjectZipSlip verifies that any copied Secret or ConfigMap is not able to be placed
+// outside of the target directory. A well-crafted symbolic link can otherwise allow secret data
+// to be placed in a location outside of our trusted target dirctory.
+func TestCopyLocalObjectZipSlip(t *testing.T) {
+	// Test scenario consists of a file tree that simulates the git clone environment
+	// /tmp/source-code-xxxx
+	// ├── git-data
+	// │   ├── Dockerfile
+	// │   └── hello.txt
+	// │   └── pwn-link -> /tmp/source-code-xxxx/pwned
+	// ├── secrets
+	// │   └── test
+	// │       └── pwned.txt
+	// └── pwned
+	//
+	// If data ends up in the "pwned" directory, we have a "zip-slip" vulnerability.
+	buildDir, err := os.MkdirTemp("", "source-code")
+	if err != nil {
+		t.Fatalf("failed to create temp dir: %v", err)
+	}
+	defer func() {
+		if err := os.RemoveAll(buildDir); err != nil {
+			t.Fatalf("failed to clean up temp dir: %v", err)
+		}
+	}()
+	// Set up secrets/test directory, with "pwned.txt" file
+	secretRoot := filepath.Join(buildDir, "secrets")
+	testPwnedFile := filepath.Join(secretRoot, "test", "pwned.txt")
+	err = fsCopyFile("testdata/fakesecret/pwned.txt", testPwnedFile)
+	if err != nil {
+		t.Fatalf("failed to copy secret data: %v", err)
+	}
+	if _, err := os.Stat(testPwnedFile); err != nil {
+		t.Fatalf("failed to stat pwned.txt: %v", err)
+	}
+	pwnedPath := filepath.Join(buildDir, "pwned")
+	err = os.MkdirAll(pwnedPath, os.ModePerm)
+	if err != nil {
+		t.Fatalf("failed to create test directory %q: %v", pwnedPath, err)
+	}
+	// Set up "git-data" directory as fake "git source"
+	gitRoot := filepath.Join(buildDir, "git-data")
+	err = filepath.Walk("testdata/fakesource", func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if info.IsDir() {
+			return nil
+		}
+		dstBase := filepath.Base(path)
+		return fsCopyFile(path, filepath.Join(gitRoot, dstBase))
+	})
+	if err != nil {
+		t.Fatalf("failed to copy test data: %v", err)
+	}
+	// Add symlink to "pwned" directory in the git root
+	err = os.Symlink(pwnedPath, filepath.Join(gitRoot, "pwn-link"))
+	if err != nil {
+		t.Fatalf("failed to create symlink to pwned directory: %v", err)
+	}
+	dockerBuilder := &DockerBuilder{
+		inputDir: buildDir,
+	}
+	localObj := buildapiv1.SecretBuildSource{
+		Secret: corev1.LocalObjectReference{
+			Name: "test",
+		},
+		DestinationDir: "pwn-link",
+	}
+	err = dockerBuilder.copyLocalObject(secretSource(localObj), secretRoot, gitRoot)
+	if err == nil {
+		t.Error("expected copyLocalObject to fail if the secret tries to write its contents outside the destination directory")
+	}
+	if err != nil && !strings.Contains(err.Error(), "outside of the target directory") {
+		t.Errorf("expected error message to contain %q, got: %v", "outside of the target directory", err)
+	}
+	// Can we stat the pwned.txt file?
+	badPwnedFile := filepath.Join(pwnedPath, "pwned.txt")
+	if _, err := os.Stat(badPwnedFile); err == nil {
+		t.Errorf("secret file %q was copied outside of the trusted build directory to %q", testPwnedFile, badPwnedFile)
+	}
+}
+
+func fsCopyFile(srcPath, dstPath string) error {
+	srcFile, err := os.Open(srcPath)
+	if err != nil {
+		return err
+	}
+	defer srcFile.Close()
+	parentDir := filepath.Dir(dstPath)
+	if err := os.MkdirAll(parentDir, os.ModePerm); err != nil {
+		return err
+	}
+	dstFile, err := os.Create(dstPath)
+	if err != nil {
+		return err
+	}
+	defer dstFile.Close()
+	if _, err := io.Copy(dstFile, srcFile); err != nil {
+		return err
+	}
+	return nil
+}

pkg/build/builder/testdata/fakesecret/pwned.txt

@@ -0,0 +1 @@
+Womp womp - you've been pwned!

pkg/build/builder/testdata/fakesecret/sourceSecret.txt

@@ -0,0 +1,2 @@
+Top secret data!
+This should be copied into the same directory as the source code.

pkg/build/builder/testdata/fakesource/Dockerfile

@@ -0,0 +1,7 @@
+FROM registry.access.redhat.com/ubi9/ubi-minimal:latest
+
+WORKDIR /opt/app-root/src
+
+COPY hello.txt hello.txt
+
+CMD ["cat", "hello.txt"]

pkg/build/builder/testdata/fakesource/hello.txt

@@ -0,0 +1 @@
+Hello OpenShift!