From 500d78dafa64552da4f9eb611346afde1381ebeb Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 10 Nov 2020 16:34:01 +0000 Subject: [PATCH] Include `os.Readlink` as a probable sanitiser. A couple of projects seem to walk links one unit at a time, rather than just throwing `EvalSymlinks` at the whole potentially suspect path. --- ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql | 14 +++++---- .../CWE-022/UnsafeUnzipSymlinkGood.go | 30 +++++++++++++++++++ .../Security/CWE-022/ZipSlip.expected | 6 ++++ 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql b/ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql index 7e2a6de5a6c..fa2151edf09 100644 --- a/ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql +++ b/ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql @@ -93,14 +93,18 @@ class SymlinkSink extends DataFlow::Node { } /** - * An argument to `path/filepath.EvalSymlinks`, taken as a sink for detecting target paths - * that are likely safe to extract to. + * An argument to `path/filepath.EvalSymlinks` or `os.Readlink`, taken as a sink for detecting target + * paths that are likely safe to extract to. */ class EvalSymlinksSink extends DataFlow::Node { EvalSymlinksSink() { - this = - any(DataFlow::CallNode n | n.getTarget().hasQualifiedName("path/filepath", "EvalSymlinks")) - .getArgument(0) + exists(DataFlow::CallNode n | + n.getTarget().hasQualifiedName("path/filepath", "EvalSymlinks") + or + n.getTarget().hasQualifiedName("os", "Readlink") + | + this = n.getArgument(0) + ) } } diff --git a/ql/test/query-tests/Security/CWE-022/UnsafeUnzipSymlinkGood.go b/ql/test/query-tests/Security/CWE-022/UnsafeUnzipSymlinkGood.go index 7d69eb75362..dde03db263d 100644 --- a/ql/test/query-tests/Security/CWE-022/UnsafeUnzipSymlinkGood.go +++ b/ql/test/query-tests/Security/CWE-022/UnsafeUnzipSymlinkGood.go @@ -48,3 +48,33 @@ func unzipSymlinkGoodZip(f io.ReaderAt, target string) { } } } + +func isRelGoodReadlink(candidate, target string) bool { + // GOOD: resolves symbolic links before checking + // that `candidate` does not escape from `target`. + // Note this is not actually safe (using Readlink + // to resolve everything is not simple), so I just + // make some token use of it here. + if filepath.IsAbs(candidate) { + return false + } + realpath, err := os.Readlink(filepath.Join(target, candidate)) + if err != nil { + return false + } + relpath, err := filepath.Rel(target, realpath) + return err == nil && !strings.HasPrefix(filepath.Clean(relpath), "..") +} + +func unzipSymlinkGoodReadlink(f io.Reader, target string) { + r := tar.NewReader(f) + for { + header, err := r.Next() + if err != nil { + break + } + if isRelGoodReadlink(header.Linkname, target) && isRelGoodReadlink(header.Name, target) { + os.Symlink(header.Linkname, header.Name) + } + } +} diff --git a/ql/test/query-tests/Security/CWE-022/ZipSlip.expected b/ql/test/query-tests/Security/CWE-022/ZipSlip.expected index d26767d52bf..b1206a6cd48 100644 --- a/ql/test/query-tests/Security/CWE-022/ZipSlip.expected +++ b/ql/test/query-tests/Security/CWE-022/ZipSlip.expected @@ -1,8 +1,13 @@ edges +| UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | +| UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string | UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string | | ZipSlip.go:12:24:12:29 | selection of Name : string | ZipSlip.go:14:20:14:20 | p | | tarslip.go:14:23:14:33 | selection of Name : string | tarslip.go:14:14:14:34 | call to Dir | | tst.go:24:11:24:16 | selection of Name : string | tst.go:29:20:29:23 | path | nodes +| UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string | semmle.label | definition of candidate : string | +| UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | semmle.label | call to Join | +| UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string | semmle.label | selection of Name : string | | ZipSlip.go:12:24:12:29 | selection of Name : string | semmle.label | selection of Name : string | | ZipSlip.go:14:20:14:20 | p | semmle.label | p | | tarslip.go:14:14:14:34 | call to Dir | semmle.label | call to Dir | @@ -10,6 +15,7 @@ nodes | tst.go:24:11:24:16 | selection of Name : string | semmle.label | selection of Name : string | | tst.go:29:20:29:23 | path | semmle.label | path | #select +| UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name | UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | Unsanitized archive entry, which may contain '..', is used in a $@. | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | file system operation | | ZipSlip.go:12:24:12:29 | selection of Name | ZipSlip.go:12:24:12:29 | selection of Name : string | ZipSlip.go:14:20:14:20 | p | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.go:14:20:14:20 | p | file system operation | | tarslip.go:14:23:14:33 | selection of Name | tarslip.go:14:23:14:33 | selection of Name : string | tarslip.go:14:14:14:34 | call to Dir | Unsanitized archive entry, which may contain '..', is used in a $@. | tarslip.go:14:14:14:34 | call to Dir | file system operation | | tst.go:24:11:24:16 | selection of Name | tst.go:24:11:24:16 | selection of Name : string | tst.go:29:20:29:23 | path | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:29:20:29:23 | path | file system operation |