Merge pull request #402 from smowton/smowton/feature/zipslip-more-generous-sanitisers

ZipSlip: redefine sources closer to their origin, and make sanitizers more generous
2026-05-04 13:15:21 +02:00 · 2020-11-27 18:25:07 +00:00
parent 627241aaa5 70015b2c32
commit 3338a0b10d
6 changed files with 288 additions and 21 deletions
--- a/change-notes/2020-11-12-zipslip-sanitizers.md
+++ b/change-notes/2020-11-12-zipslip-sanitizers.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Improved recongition of sanitizer functions for the `go/zipslip` query. This may reduce false-positives (but also perhaps false-negatives) when application code attempts to check a zip header entry does not contain an illegal path traversal attempt.
--- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
+++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
@@ -920,6 +920,35 @@ class TypeCastNode extends ExprNode {
  }
 }

+/**
+ * A data-flow node representing an element of an array, map, slice or string defined from `range` statement.
+ *
+ * Example: in `_, x := range y { ... }`, this represents the `Node` that extracts the element from the
+ * range statement, which will flow to `x`.
+ */
+class RangeElementNode extends Node {
+  DataFlow::Node base;
+  IR::ExtractTupleElementInstruction extract;
+
+  RangeElementNode() {
+    this.asInstruction() = extract and
+    extract.extractsElement(_, 1) and
+    extract.getBase().(IR::GetNextEntryInstruction).getDomain() = base.asInstruction()
+  }
+
+  /** Gets the data-flow node representing the base from which the element is read. */
+  DataFlow::Node getBase() { result = base }
+}
+
+/**
+ * Holds if `node` reads an element from `base`, either via an element-read (`base[y]`) expression
+ * or via a range statement `_, node := range base`.
+ */
+predicate readsAnElement(DataFlow::Node node, DataFlow::Node base) {
+  node.(ElementReadNode).readsElement(base, _) or
+  node.(RangeElementNode).getBase() = base
+}
+
 /**
 * A model of a function specifying that the function copies input values from
 * a parameter or qualifier to a result.
--- a/ql/src/semmle/go/security/ZipSlipCustomizations.qll
+++ b/ql/src/semmle/go/security/ZipSlipCustomizations.qll
@@ -32,11 +32,41 @@ module ZipSlip {
   */
  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }

-  /** A file name from a zip or tar entry, as a source for zip slip. */
-  class FileNameSource extends Source, DataFlow::FieldReadNode {
-    FileNameSource() {
-      getField().hasQualifiedName("archive/zip", "File", "Name") or
-      getField().hasQualifiedName("archive/tar", "Header", "Name")
+  /**
+   * A tar file header, as a source for zip slip.
+   */
+  class TarHeaderSource extends Source, DataFlow::Node {
+    TarHeaderSource() {
+      this =
+        any(DataFlow::MethodCallNode mcn |
+          mcn.getTarget().hasQualifiedName("archive/tar", "Reader", "Next")
+        ).getResult(0)
+    }
+  }
+
+  /**
+   * A zip file header, as a source for zip slip.
+   */
+  class ZipHeaderSource extends Source {
+    ZipHeaderSource() {
+      exists(DataFlow::FieldReadNode frn, DataFlow::Node elbase |
+        frn.getField().hasQualifiedName("archive/zip", "Reader", "File") and
+        DataFlow::localFlow(frn, elbase)
+      |
+        DataFlow::readsAnElement(this, elbase)
+      )
+    }
+  }
+
+  /**
+   * Excludes zipped file data from consideration for zip slip.
+   */
+  class ZipFileOpen extends Sanitizer {
+    ZipFileOpen() {
+      this =
+        any(DataFlow::MethodCallNode mcn |
+          mcn.getTarget().hasQualifiedName("archive/zip", "File", "Open")
+        ).getResult(0)
    }
  }

@@ -56,12 +86,31 @@ module ZipSlip {
    TaintedPathSanitizerAsSanitizer() { this instanceof TaintedPath::Sanitizer }
  }

-  /** A path-traversal sanitizer guard, considered as a sanitizer guard for zip slip. */
-  class TaintedPathSanitizerGuardAsSanitizerGuard extends SanitizerGuard {
-    TaintedPath::SanitizerGuard self;
+  /**
+   * A sanitizer guard for zip-slip vulnerabilities which backtracks to sanitize expressions
+   * that locally flow into a guarded expression. For example, an ordinary sanitizer guard
+   * might say that in `if x { z := y }` the reference to `y` is sanitized because of the guard
+   * `x`; these guards say that if the function begins
+   * `f(p string) { w := filepath.Join(p); y := filepath.Dir(w) }` then both `p` and `w` are also
+   * sanitized as expressions that contributed taint to `y`.
+   *
+   * This is useful because many sanitizers don't directly check the filename included in an archive
+   * header, but some derived expression. It also propagates back from a field reference to its parent
+   * (e.g. `hdr.Filename` to `hdr`), increasing the chances that a future reference to `hdr.Filename`
+   * will also be regarded as clean (though SSA catches some cases of this).
+   */
+  class TaintedPathSanitizerGuardAsBacktrackingSanitizerGuard extends SanitizerGuard {
+    TaintedPath::SanitizerGuard taintedPathGuard;

-    TaintedPathSanitizerGuardAsSanitizerGuard() { self = this }
+    TaintedPathSanitizerGuardAsBacktrackingSanitizerGuard() { this = taintedPathGuard }

-    override predicate checks(Expr e, boolean branch) { self.checks(e, branch) }
+    override predicate checks(Expr e, boolean branch) {
+      exists(DataFlow::Node source, DataFlow::Node checked |
+        taintedPathGuard.checks(checked.asExpr(), branch) and
+        TaintTracking::localTaint(source, checked)
+      |
+        e = source.asExpr()
+      )
+    }
  }
 }
--- a/ql/test/query-tests/Security/CWE-022/ZipSlip.expected
+++ b/ql/test/query-tests/Security/CWE-022/ZipSlip.expected
@@ -1,21 +1,50 @@
 edges
 | UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join |
+| UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] : pointer type | UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header |
+| UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] : pointer type | UnsafeUnzipSymlinkGood.go:76:24:76:38 | selection of Linkname : string |
+| UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] : pointer type | UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header |
+| UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] : pointer type | UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string |
+| UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header |
+| UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:24:76:38 | selection of Linkname : string |
+| UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header |
+| UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string |
+| UnsafeUnzipSymlinkGood.go:76:24:76:38 | selection of Linkname : string | UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string |
+| UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header |
+| UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:24:76:38 | selection of Linkname : string |
+| UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header |
+| UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header | UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string |
 | 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 |
+| ZipSlip.go:11:2:15:2 | range statement[1] : pointer type | ZipSlip.go:12:24:12:24 | implicit dereference : File |
+| ZipSlip.go:11:2:15:2 | range statement[1] : pointer type | ZipSlip.go:14:20:14:20 | p |
+| ZipSlip.go:12:24:12:24 | implicit dereference : File | ZipSlip.go:12:24:12:24 | implicit dereference : File |
+| ZipSlip.go:12:24:12:24 | implicit dereference : File | ZipSlip.go:14:20:14:20 | p |
+| tarslip.go:15:2:15:30 | ... := ...[0] : pointer type | tarslip.go:16:14:16:34 | call to Dir |
+| tarslip.go:15:2:15:30 | ... := ...[0] : pointer type | tarslip.go:16:23:16:28 | implicit dereference : Header |
+| tarslip.go:16:23:16:28 | implicit dereference : Header | tarslip.go:16:14:16:34 | call to Dir |
+| tarslip.go:16:23:16:28 | implicit dereference : Header | tarslip.go:16:23:16:28 | implicit dereference : Header |
+| tst.go:23:2:43:2 | range statement[1] : pointer type | tst.go:24:11:24:11 | implicit dereference : File |
+| tst.go:23:2:43:2 | range statement[1] : pointer type | tst.go:29:20:29:23 | path |
+| tst.go:24:11:24:11 | implicit dereference : File | tst.go:24:11:24:11 | implicit dereference : File |
+| tst.go:24:11:24:11 | implicit dereference : File | 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:72:3:72:25 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |
+| UnsafeUnzipSymlinkGood.go:76:24:76:29 | implicit dereference : Header | semmle.label | implicit dereference : Header |
+| UnsafeUnzipSymlinkGood.go:76:24:76:38 | selection of Linkname : string | semmle.label | selection of Linkname : string |
+| UnsafeUnzipSymlinkGood.go:76:70:76:75 | implicit dereference : Header | semmle.label | implicit dereference : Header |
 | 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:11:2:15:2 | range statement[1] : pointer type | semmle.label | range statement[1] : pointer type |
+| ZipSlip.go:12:24:12:24 | implicit dereference : File | semmle.label | implicit dereference : File |
 | ZipSlip.go:14:20:14:20 | p | semmle.label | p |
-| tarslip.go:14:14:14:34 | call to Dir | semmle.label | call to Dir |
-| tarslip.go:14:23:14:33 | selection of Name : string | semmle.label | selection of Name : string |
-| tst.go:24:11:24:16 | selection of Name : string | semmle.label | selection of Name : string |
+| tarslip.go:15:2:15:30 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |
+| tarslip.go:16:14:16:34 | call to Dir | semmle.label | call to Dir |
+| tarslip.go:16:23:16:28 | implicit dereference : Header | semmle.label | implicit dereference : Header |
+| tst.go:23:2:43:2 | range statement[1] : pointer type | semmle.label | range statement[1] : pointer type |
+| tst.go:24:11:24:11 | implicit dereference : File | semmle.label | implicit dereference : File |
 | 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 |
+| UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] | UnsafeUnzipSymlinkGood.go:72:3:72:25 | ... := ...[0] : pointer type | 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:11:2:15:2 | range statement[1] | ZipSlip.go:11:2:15:2 | range statement[1] : pointer type | 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:15:2:15:30 | ... := ...[0] | tarslip.go:15:2:15:30 | ... := ...[0] : pointer type | tarslip.go:16:14:16:34 | call to Dir | Unsanitized archive entry, which may contain '..', is used in a $@. | tarslip.go:16:14:16:34 | call to Dir | file system operation |
+| tst.go:23:2:43:2 | range statement[1] | tst.go:23:2:43:2 | range statement[1] : pointer type | 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 |
--- a/ql/test/query-tests/Security/CWE-022/ZipSlipGood2.go
+++ b/ql/test/query-tests/Security/CWE-022/ZipSlipGood2.go
@@ -0,0 +1,92 @@
+package main
+
+import (
+	"archive/zip"
+	"errors"
+	"io/ioutil"
+	"path/filepath"
+	"strings"
+)
+
+func unzipGood2(f string) {
+	r, _ := zip.OpenReader(f)
+	for _, f := range r.File {
+		// GOOD: file contents should not be flagged for zip slip
+		reader, _ := f.Open()
+		buf := make([]byte, f.UncompressedSize64)
+		reader.Read(buf)
+		ioutil.WriteFile("somefile.txt", buf, 0644)
+	}
+}
+
+// GOOD: checks `f.Name` indirectly.
+func unzipGoodIndirect(f string) {
+	r, _ := zip.OpenReader(f)
+	for _, f := range r.File {
+		p, _ := filepath.Abs(f.Name)
+		// GOOD: Check that path does not contain ".." before using it
+		if !strings.Contains(filepath.Clean(p), "..") {
+			ioutil.WriteFile(p, []byte("present"), 0666)
+		}
+	}
+}
+
+func checkPath(p string) error {
+	if strings.Contains(filepath.Clean(p), "..") {
+		return errors.New("zip contents corrupted")
+	}
+	return nil
+}
+
+// GOOD: uses a function that returns an error
+// when the header is not safe to use. Flagged because we can't currently
+// recognise that `errors.New` always returns non-nil.
+func unzipGoodIndirectUsingFunction(f string) {
+	r, _ := zip.OpenReader(f)
+	for _, f := range r.File {
+		p, _ := filepath.Abs(f.Name)
+		// GOOD: Check that path does not contain ".." before using it
+		if checkPath(p) == nil {
+			ioutil.WriteFile(p, []byte("present"), 0666)
+		}
+	}
+}
+
+func checkPathBool(p string) bool {
+	return strings.Contains(filepath.Clean(p), "..")
+}
+
+// GOOD: uses a checker function returning a boolean
+// to ensure `header` is safe to unpack. Currently flagged because the callee
+// has no 'return true' or 'return false' statements, but rather directly
+// returns a sanitizer guard function.
+func unzipGoodIndirectUsingBoolFunction(f string) {
+	r, _ := zip.OpenReader(f)
+	for _, f := range r.File {
+		p, _ := filepath.Abs(f.Name)
+		// GOOD: Check that path does not contain ".." before using it
+		if !checkPathBool(p) {
+			ioutil.WriteFile(p, []byte("present"), 0666)
+		}
+	}
+}
+
+func checkPathBoolStmt(p string) bool {
+	if strings.Contains(filepath.Clean(p), "..") {
+		return true
+	}
+	return false
+}
+
+// GOOD: uses a checker function returning a boolean to ensure `header`
+// is safe to unpack.
+func unzipGoodIndirectUsingBoolStmtFunction(f string) {
+	r, _ := zip.OpenReader(f)
+	for _, f := range r.File {
+		p, _ := filepath.Abs(f.Name)
+		// GOOD: Check that path does not contain ".." before using it
+		if !checkPathBoolStmt(p) {
+			ioutil.WriteFile(p, []byte("present"), 0666)
+		}
+	}
+}
--- a/ql/test/query-tests/Security/CWE-022/tarslip.go
+++ b/ql/test/query-tests/Security/CWE-022/tarslip.go
@@ -2,9 +2,11 @@ package main

 import (
 	"archive/tar"
+	"errors"
 	"io"
 	"os"
 	"path"
+	"path/filepath"
 	"strings"
 )

@@ -22,3 +24,67 @@ func untarGood(reader io.Reader, prefix string) {
 	}
 	os.MkdirAll(path.Dir(header.Name), 0755) // OK
 }
+
+// GOOD: checks `header.Name` indirectly.
+func untarGoodIndirect(reader io.Reader, prefix string) {
+	tarReader := tar.NewReader(reader)
+	header, _ := tarReader.Next()
+	if !strings.HasPrefix(filepath.Join(prefix, header.Name), prefix) {
+		panic("tar contents corrupted")
+	}
+	os.MkdirAll(path.Dir(header.Name), 0755) // OK
+}
+
+func checkPathTar(s, prefix string) error {
+	if !strings.HasPrefix(filepath.Join(prefix, s), prefix) {
+		return errors.New("tar contents corrupted")
+	}
+	return nil
+}
+
+// GOOD: uses a function that returns an error
+// when the header is not safe to use. Flagged because we can't currently
+// recognise that `errors.New` always returns non-nil.
+func untarGoodIndirectUsingFunction(reader io.Reader, prefix string) {
+	tarReader := tar.NewReader(reader)
+	header, _ := tarReader.Next()
+	if checkPathTar(header.Name, prefix) != nil {
+		panic("tar contents corrupted")
+	}
+	os.MkdirAll(path.Dir(header.Name), 0755) // OK
+}
+
+func checkPathBoolTar(s, prefix string) bool {
+	return strings.HasPrefix(filepath.Join(prefix, s), prefix)
+}
+
+// GOOD: uses a checker function returning a boolean
+// to ensure `header` is safe to unpack. Currently flagged because the callee
+// has no 'return true' or 'return false' statements, but rather directly
+// returns a sanitizer guard function.
+func untarGoodIndirectUsingBoolFunction(reader io.Reader, prefix string) {
+	tarReader := tar.NewReader(reader)
+	header, _ := tarReader.Next()
+	if !checkPathBoolTar(header.Name, prefix) {
+		panic("tar contents corrupted")
+	}
+	os.MkdirAll(path.Dir(header.Name), 0755) // OK
+}
+
+func checkPathBoolStmtTar(s, prefix string) bool {
+	if strings.HasPrefix(filepath.Join(prefix, s), prefix) {
+		return true
+	}
+	return false
+}
+
+// GOOD: uses a checker function returning a boolean to ensure `header`
+// is safe to unpack.
+func untarGoodIndirectUsingBoolStmtFunction(reader io.Reader, prefix string) {
+	tarReader := tar.NewReader(reader)
+	header, _ := tarReader.Next()
+	if !checkPathBoolStmtTar(header.Name, prefix) {
+		panic("tar contents corrupted")
+	}
+	os.MkdirAll(path.Dir(header.Name), 0755) // OK
+}