fix: Improve CMP zipslip protection (argoproj#8789)

* fix: Improve CMP zipslip protection

Signed-off-by: Leonardo Luz Almeida <[email protected]>

* Add files.Inbound unit tests

Signed-off-by: Leonardo Luz Almeida <[email protected]>

* test codeql false positive

Signed-off-by: Leonardo Luz Almeida <[email protected]>

* add lgtm tag to ignore false-positive

Signed-off-by: Leonardo Luz Almeida <[email protected]>

* Remove lgtm tag from the code

Signed-off-by: Leonardo Luz Almeida <[email protected]>

util/io/files/tar.go

@@ -8,7 +8,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"strings"
 )
 
 type tgz struct {
@@ -73,9 +72,8 @@ func Untgz(dstPath string, r io.Reader) error {
 		}
 
 		target := filepath.Join(dstPath, header.Name)
-
 		// Sanity check to protect against zip-slip
-		if !strings.HasPrefix(target, filepath.Clean(dstPath)+string(os.PathSeparator)) {
+		if !Inbound(target, dstPath) {
 			return fmt.Errorf("illegal filepath in archive: %s", target)
 		}
 
@@ -87,16 +85,17 @@ func Untgz(dstPath string, r io.Reader) error {
 			}
 		case tar.TypeSymlink:
 			// Sanity check to protect against symlink exploit
-			var linkTarget string
-			if filepath.IsAbs(header.Linkname) {
-				linkTarget = filepath.Clean(header.Linkname)
-			} else {
-				linkTarget = filepath.Join(filepath.Dir(target), header.Linkname)
+			linkTarget := filepath.Join(filepath.Dir(target), header.Linkname)
+			realPath, err := filepath.EvalSymlinks(linkTarget)
+			if os.IsNotExist(err) {
+				realPath = linkTarget
+			} else if err != nil {
+				return fmt.Errorf("error checking symlink realpath: %s", err)
 			}
-			if !strings.HasPrefix(linkTarget, filepath.Clean(dstPath)+string(os.PathSeparator)) {
+			if !Inbound(realPath, dstPath) {
 				return fmt.Errorf("illegal filepath in symlink: %s", linkTarget)
 			}
-			err := os.Symlink(linkTarget, target)
+			err = os.Symlink(realPath, target)
 			if err != nil {
 				return fmt.Errorf("error creating symlink: %s", err)
 			}

util/io/files/util.go

@@ -68,3 +68,24 @@ func CreateTempDir(baseDir string) (string, error) {
 func IsSymlink(fi os.FileInfo) bool {
 	return fi.Mode()&fs.ModeSymlink == fs.ModeSymlink
 }
+
+// Inbound will validate if the given candidate path is inside the
+// baseDir. This is useful to make sure that malicious candidates
+// are not targeting a file outside of baseDir boundaries.
+// Considerations:
+// - baseDir must be absolute path. Will return false otherwise
+// - candidate can be absolute or relative path
+// - candidate should not be symlink as only syntatic validation is
+// applied by this function
+func Inbound(candidate, baseDir string) bool {
+	if !filepath.IsAbs(baseDir) {
+		return false
+	}
+	var target string
+	if filepath.IsAbs(candidate) {
+		target = filepath.Clean(candidate)
+	} else {
+		target = filepath.Join(baseDir, candidate)
+	}
+	return strings.HasPrefix(target, filepath.Clean(baseDir)+string(os.PathSeparator))
+}

util/io/files/util_test.go

@@ -89,3 +89,57 @@ func TestRelativePath(t *testing.T) {
 		})
 	}
 }
+
+func TestInbound(t *testing.T) {
+	type testcase struct {
+		name      string
+		candidate string
+		basedir   string
+		expected  bool
+	}
+	cases := []testcase{
+		{
+			name:      "will return true if candidate is inbound",
+			candidate: "/home/test/app/readme.md",
+			basedir:   "/home/test",
+			expected:  true,
+		},
+		{
+			name:      "will return false if candidate is not inbound",
+			candidate: "/home/test/../readme.md",
+			basedir:   "/home/test",
+			expected:  false,
+		},
+		{
+			name:      "will return true if candidate is relative inbound",
+			candidate: "./readme.md",
+			basedir:   "/home/test",
+			expected:  true,
+		},
+		{
+			name:      "will return false if candidate is relative outbound",
+			candidate: "../readme.md",
+			basedir:   "/home/test",
+			expected:  false,
+		},
+		{
+			name:      "will return false if basedir is relative",
+			candidate: "/home/test/app/readme.md",
+			basedir:   "./test",
+			expected:  false,
+		},
+	}
+	for _, c := range cases {
+		c := c
+		t.Run(c.name, func(t *testing.T) {
+			// given
+			t.Parallel()
+
+			// when
+			inbound := files.Inbound(c.candidate, c.basedir)
+
+			// then
+			assert.Equal(t, c.expected, inbound)
+		})
+	}
+}