From 7c0a3618c3c655488a0d4ab6675ccdfed11347a1 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 18 Mar 2022 10:20:08 -0400 Subject: [PATCH] fix: Improve CMP zipslip protection (#8789) * fix: Improve CMP zipslip protection Signed-off-by: Leonardo Luz Almeida * Add files.Inbound unit tests Signed-off-by: Leonardo Luz Almeida * test codeql false positive Signed-off-by: Leonardo Luz Almeida * add lgtm tag to ignore false-positive Signed-off-by: Leonardo Luz Almeida * Remove lgtm tag from the code Signed-off-by: Leonardo Luz Almeida --- util/io/files/tar.go | 19 +++++++------- util/io/files/util.go | 21 +++++++++++++++ util/io/files/util_test.go | 54 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/util/io/files/tar.go b/util/io/files/tar.go index 42a16f8d04570..0033788ea5bba 100644 --- a/util/io/files/tar.go +++ b/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) } diff --git a/util/io/files/util.go b/util/io/files/util.go index 594d6ca420bc3..6bbcaa751d395 100644 --- a/util/io/files/util.go +++ b/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)) +} diff --git a/util/io/files/util_test.go b/util/io/files/util_test.go index a3b3bb5adb20d..d9bdfa74a09a7 100644 --- a/util/io/files/util_test.go +++ b/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) + }) + } +}