Merge pull request #17177 from owen-mc/go/mad/convert-sink-3

Go: convert regex-use, url-redirection sinks to use models-as-data

go/ql/lib/ext/clevergo.tech.clevergo.model.yml

@@ -0,0 +1,12 @@
+extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: packageGrouping
+    data:
+      - ["clever-go", "clevergo.tech/clevergo"]
+      - ["clever-go", "github.com/clevergo/clevergo"]
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sinkModel
+    data:
+      - ["group:clever-go", "Context", True, "Redirect", "", "", "Argument[1]", "url-redirection[receiver]", "manual"]

go/ql/lib/ext/github.com.beego.beego.server.web.context.model.yml

@@ -10,7 +10,10 @@ extensions:
       pack: codeql/go-all
       extensible: sinkModel
     data:
+      # path-injection
       - ["group:beego-context", "BeegoOutput", False, "Download", "", "", "Argument[0]", "path-injection", "manual"]
+      # url-redirection
+      - ["group:beego-context", "Context", True, "Redirect", "", "", "Argument[1]", "url-redirection", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/ext/github.com.beego.beego.server.web.model.yml

@@ -27,6 +27,8 @@ extensions:
       - ["group:beego", "Controller", False, "SaveToFile", "", "", "Argument[1]", "path-injection", "manual"]
       - ["group:beego", "Controller", False, "SaveToFileWithBuffer", "", "", "Argument[1]", "path-injection", "manual"] # only exists in v2
       - ["group:beego", "FileSystem", False, "Open", "", "", "Argument[0]", "path-injection", "manual"]
+      # url-redirection
+      - ["group:beego", "Controller", True, "Redirect", "", "", "Argument[0]", "url-redirection", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/ext/github.com.gofiber.fiber.model.yml

@@ -3,7 +3,10 @@ extensions:
       pack: codeql/go-all
       extensible: sinkModel
     data:
+      # path-injection
       - ["github.com/gofiber/fiber", "Ctx", False, "SendFile", "", "", "Argument[0]", "path-injection", "manual"]
       - ["github.com/gofiber/fiber", "Ctx", False, "Download", "", "", "Argument[0]", "path-injection", "manual"]
       - ["github.com/gofiber/fiber", "Ctx", False, "SaveFile", "", "", "Argument[1]", "path-injection", "manual"]
       - ["github.com/gofiber/fiber", "Ctx", False, "SaveFileToStorage", "", "", "Argument[1]", "path-injection", "manual"] # does not exist in v1
+      # url-redirection
+      - ["github.com/gofiber/fiber", "Ctx", True, "Redirect", "", "", "Argument[0]", "url-redirection[receiver]", "manual"]

go/ql/lib/ext/github.com.labstack.echo.model.yml

@@ -3,8 +3,11 @@ extensions:
       pack: codeql/go-all
       extensible: sinkModel
     data:
+      # path-injection
       - ["github.com/labstack/echo", "Context", False, "Attachment", "", "", "Argument[0]", "path-injection", "manual"]
       - ["github.com/labstack/echo", "Context", False, "File", "", "", "Argument[0]", "path-injection", "manual"]
+      # url-redirection
+      - ["github.com/labstack/echo", "Context", True, "Redirect", "", "", "Argument[1]", "url-redirection", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/ext/github.com.revel.revel.model.yml

@@ -5,6 +5,11 @@ extensions:
     data:
       - ["revel", "github.com/revel/revel"]
       - ["revel", "github.com/robfig/revel"]
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sinkModel
+    data:
+      - ["group:revel", "Controller", True, "Redirect", "", "", "Argument[0]", "url-redirection", "manual"] # It is currently assumed that a tainted `value` in `Redirect(url, value)`, which calls `Sprintf(url, value)` internally, cannot lead to an open redirect vulnerability.
   - addsTo:
       pack: codeql/go-all
       extensible: sourceModel

go/ql/lib/ext/github.com.valyala.fasthttp.model.yml

@@ -38,6 +38,9 @@ extensions:
       - ["github.com/valyala/fasthttp", "RequestCtx", False, "SendFile", "", "", "Argument[0]", "path-injection", "manual"]
       - ["github.com/valyala/fasthttp", "RequestCtx", False, "SendFileBytes", "", "", "Argument[0]", "path-injection", "manual"]
       - ["github.com/valyala/fasthttp", "Response", False, "SendFile", "", "", "Argument[0]", "path-injection", "manual"]
+      # url-redirection
+      - ["github.com/valyala/fasthttp", "RequestCtx", True, "Redirect", "", "", "Argument[0]", "url-redirection", "manual"]
+      - ["github.com/valyala/fasthttp", "RequestCtx", True, "RedirectBytes", "", "", "Argument[0]", "url-redirection", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/ext/gopkg.in.macaron.model.yml

@@ -1,4 +1,9 @@
 extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sinkModel
+    data:
+      - ["gopkg.in/macaron", "Context", True, "Redirect", "", "", "Argument[0]", "url-redirection[receiver]", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: sourceModel

go/ql/lib/ext/net.http.model.yml

@@ -3,7 +3,10 @@ extensions:
       pack: codeql/go-all
       extensible: sinkModel
     data:
+      # path-injection
       - ["net/http", "", False, "ServeFile", "", "", "Argument[2]", "path-injection", "manual"]
+      # url-redirection
+      - ["net/http", "", True, "Redirect", "", "", "Argument[2]", "url-redirection[0]", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/ext/regexp.model.yml

@@ -1,4 +1,18 @@
 extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sinkModel
+    data:
+      - ["regexp", "", True, "Compile", "", "", "Argument[0]", "regex-use[c]", "manual"]
+      - ["regexp", "", True, "CompilePOSIX", "", "", "Argument[0]", "regex-use[c]", "manual"]
+      - ["regexp", "", True, "MustCompile", "", "", "Argument[0]", "regex-use[c]", "manual"]
+      - ["regexp", "", True, "MustCompilePOSIX", "", "", "Argument[0]", "regex-use[c]", "manual"]
+      - ["regexp", "", True, "Match", "", "", "Argument[0]", "regex-use[1]", "manual"]
+      - ["regexp", "", True, "MatchReader", "", "", "Argument[0]", "regex-use[1]", "manual"]
+      - ["regexp", "", True, "MatchString", "", "", "Argument[0]", "regex-use[1]", "manual"]
+      - ["regexp", "Regexp", True, "Match", "", "", "Argument[receiver]", "regex-use[0]", "manual"]
+      - ["regexp", "Regexp", True, "MatchReader", "", "", "Argument[receiver]", "regex-use[0]", "manual"]
+      - ["regexp", "Regexp", True, "MatchString", "", "", "Argument[receiver]", "regex-use[0]", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/semmle/go/concepts/HTTP.qll

@@ -305,6 +305,47 @@ module Http {
      * open redirect exploits; for example, a form field submitted in a POST request.
      */
     abstract class UnexploitableSource extends DataFlow::Node { }
+
+    private predicate sinkKindInfo(string kind, int rw) {
+      kind = "url-redirection" and
+      rw = -2
+      or
+      kind = "url-redirection[receiver]" and
+      rw = -1
+      or
+      sinkModel(_, _, _, _, _, _, _, kind, _, _) and
+      exists(string rwStr |
+        rwStr.toInt() = rw and
+        kind = "url-redirection[" + rwStr + "]"
+      )
+    }
+
+    private class DefaultHttpRedirect extends Range, DataFlow::CallNode {
+      DataFlow::ArgumentNode url;
+      int rw;
+
+      DefaultHttpRedirect() {
+        this = url.getCall() and
+        exists(string kind |
+          sinkKindInfo(kind, rw) and
+          sinkNode(url, kind)
+        )
+      }
+
+      override DataFlow::Node getUrl() {
+        not url instanceof DataFlow::ImplicitVarargsSlice and
+        result = url
+        or
+        url instanceof DataFlow::ImplicitVarargsSlice and
+        result = this.getAnImplicitVarargsArgument()
+      }
+
+      override Http::ResponseWriter getResponseWriter() {
+        rw = -1 and result.getANode() = this.getReceiver()
+        or
+        rw >= 0 and result.getANode() = this.getArgument(rw)
+      }
+    }
   }
 
   /**

go/ql/lib/semmle/go/frameworks/Beego.qll

@@ -173,29 +173,6 @@ module Beego {
     }
   }
 
-  private class RedirectMethods extends Http::Redirect::Range, DataFlow::CallNode {
-    string className;
-
-    RedirectMethods() {
-      exists(string package |
-        (
-          package = packagePath() and className = "Controller"
-          or
-          package = contextPackagePath() and className = "Context"
-        ) and
-        this = any(Method m | m.hasQualifiedName(package, className, "Redirect")).getACall()
-      )
-    }
-
-    override DataFlow::Node getUrl() {
-      className = "Controller" and result = this.getArgument(0)
-      or
-      className = "Context" and result = this.getArgument(1)
-    }
-
-    override Http::ResponseWriter getResponseWriter() { none() }
-  }
-
   private class UtilsTaintPropagators extends TaintTracking::FunctionModel {
     UtilsTaintPropagators() { this.hasQualifiedName(utilsPackagePath(), "GetDisplayString") }
 

go/ql/lib/semmle/go/frameworks/Echo.qll

@@ -54,21 +54,6 @@ private module Echo {
     override DataFlow::Node getAContentTypeNode() { result = callNode.getArgument(1) }
   }
 
-  /**
-   * The `echo.Context.Redirect` method.
-   */
-  private class EchoRedirectMethod extends Http::Redirect::Range, DataFlow::CallNode {
-    EchoRedirectMethod() {
-      exists(Method m | m.hasQualifiedName(packagePath(), "Context", "Redirect") |
-        this = m.getACall()
-      )
-    }
-
-    override DataFlow::Node getUrl() { result = this.getArgument(1) }
-
-    override Http::ResponseWriter getResponseWriter() { none() }
-  }
-
   /**
    * DEPRECATED: Use `FileSystemAccess::Range` instead.
    *

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

@@ -496,22 +496,6 @@ module Fasthttp {
       override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
     }
 
-    /**
-     * The Methods that can be dangerous if they take user controlled URL as their first argument.
-     */
-    class Redirect extends Http::Redirect::Range, DataFlow::CallNode {
-      Redirect() {
-        exists(Method m |
-          m.hasQualifiedName(packagePath(), "RequestCtx", ["Redirect", "RedirectBytes"]) and
-          this = m.getACall()
-        )
-      }
-
-      override DataFlow::Node getUrl() { result = this.getArgument(0) }
-
-      override Http::ResponseWriter getResponseWriter() { none() }
-    }
-
     /**
      * DEPRECATED: Use `RemoteFlowSource` instead.
      */

go/ql/lib/semmle/go/frameworks/Macaron.qll

@@ -17,14 +17,4 @@ private module Macaron {
 
     override DataFlow::Node getANode() { result = v.similar().getAUse().getASuccessor*() }
   }
-
-  private class RedirectCall extends Http::Redirect::Range, DataFlow::MethodCallNode {
-    RedirectCall() {
-      this.getTarget().hasQualifiedName("gopkg.in/macaron.v1", "Context", "Redirect")
-    }
-
-    override DataFlow::Node getUrl() { result = this.getArgument(0) }
-
-    override Http::ResponseWriter getResponseWriter() { result.getANode() = this.getReceiver() }
-  }
 }

go/ql/lib/semmle/go/frameworks/Revel.qll

@@ -87,24 +87,6 @@ module Revel {
     override string getAContentType() { result = contentType }
   }
 
-  /**
-   * The `revel.Controller.Redirect` method.
-   *
-   * It is currently assumed that a tainted `value` in `Redirect(url, value)`, which calls `Sprintf(url, value)`
-   * internally, cannot lead to an open redirect vulnerability.
-   */
-  private class ControllerRedirectMethod extends Http::Redirect::Range, DataFlow::CallNode {
-    ControllerRedirectMethod() {
-      exists(Method m | m.hasQualifiedName(packagePath(), "Controller", "Redirect") |
-        this = m.getACall()
-      )
-    }
-
-    override DataFlow::Node getUrl() { result = this.getArgument(0) }
-
-    override Http::ResponseWriter getResponseWriter() { none() }
-  }
-
   /**
    * A read in a Revel template that uses Revel's `raw` function.
    */

go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll

@@ -175,14 +175,6 @@ module NetHttp {
     override Http::ResponseWriter getResponseWriter() { result.getANode() = responseWriter }
   }
 
-  private class RedirectCall extends Http::Redirect::Range, DataFlow::CallNode {
-    RedirectCall() { this.getTarget().hasQualifiedName("net/http", "Redirect") }
-
-    override DataFlow::Node getUrl() { result = this.getArgument(2) }
-
-    override Http::ResponseWriter getResponseWriter() { result.getANode() = this.getArgument(0) }
-  }
-
   /** A call to a function in the `net/http` package that performs an HTTP request to a URL. */
   private class RequestCall extends Http::ClientRequest::Range, DataFlow::CallNode {
     RequestCall() {