From c1669d732ba9ae2e51132ab130a3830b781ae287 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Wed, 2 Dec 2020 14:33:38 +0000
Subject: [PATCH] Unsafe-unzip-symlinks: reduce cost of `getAnEnclosingLoop`

This used to get the closest enclosing loops of all expressions; now it is restricted to those surrounding interesting expressions.
---
 .../UnsafeUnzipSymlinkCustomizations.qll      | 34 +++++++++++++------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/ql/src/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll b/ql/src/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll
index 20590b34ef5..020d9a323ef 100644
--- a/ql/src/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll
+++ b/ql/src/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll
@@ -111,21 +111,33 @@ module UnsafeUnzipSymlink {
   private LoopStmt getLocalEnclosingLoop(Expr e) { result = getNonLoopParent*(e).getParent() }
 
   /**
-   * Gets one of `e`'s enclosing loop statements, including looking across function calls.
-   *
-   * The returned loops are the closest loop on their particular path (i.e., this might return
-   * multiple results, but no result encloses any other).
+   * Gets a call expression that may call `node`'s enclosing function if `node` has no local enclosing loop.
    */
-  private LoopStmt getAnEnclosingLoop(Expr e) {
-    result = getLocalEnclosingLoop(e)
-    or
-    not exists(getLocalEnclosingLoop(e)) and result = getAnEnclosingLoop(getAnExprCaller(e))
+  private Expr getAnExprCallerIfNotInLoop(Expr node) {
+    not exists(getLocalEnclosingLoop(node)) and
+    result = getAnExprCaller(node)
+  }
+
+  /**
+   * Gets a call to `os.Symlink`.
+   */
+  private CallExpr getASymlinkCall() { result.getTarget().hasQualifiedName("os", "Symlink") }
+
+  /**
+   * Gets the closest enclosing loop to an `os.Symlink` call, `archive/tar.Reader.Next` call or
+   * read from `archive/zip.Reader.File`.
+   */
+  private LoopStmt getAnExtractionOrSymlinkLoop(Expr e) {
+    (e = getASymlinkCall() or e = getAnArchiveEntryAccess()) and
+    result = getLocalEnclosingLoop(getAnExprCallerIfNotInLoop*(e))
   }
 
   /**
    * Gets a loop statement that contains either a read from `archive/zip.Reader.File` or a call to `archive/tar.Reader.Next()`.
    */
-  private LoopStmt getAnExtractionLoop() { result = getAnEnclosingLoop(getAnArchiveEntryAccess()) }
+  private LoopStmt getAnExtractionLoop() {
+    result = getAnExtractionOrSymlinkLoop(getAnArchiveEntryAccess())
+  }
 
   /**
    * An argument to a call to `os.Symlink` within a loop that extracts a zip or tar archive,
@@ -133,9 +145,9 @@ module UnsafeUnzipSymlink {
    */
   class OsSymlink extends DataFlow::Node, SymlinkSink {
     OsSymlink() {
-      exists(DataFlow::CallNode n | n.getTarget().hasQualifiedName("os", "Symlink") |
+      exists(DataFlow::CallNode n | n.asExpr() = getASymlinkCall() |
         this = n.getArgument([0, 1]) and
-        getAnEnclosingLoop(n.asExpr()) = getAnExtractionLoop()
+        getAnExtractionOrSymlinkLoop(n.asExpr()) = getAnExtractionLoop()
       )
     }
   }