Convert url-redirection sinks to MaD

owen-mc · owen-mc · commit 49f3959405d3 · 2024-08-11T00:29:53.000+01:00
diff --git a/go/ql/lib/ext/clevergo.tech.clevergo.model.yml b/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"]
diff --git a/go/ql/lib/ext/github.com.beego.beego.server.web.context.model.yml b/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
diff --git a/go/ql/lib/ext/github.com.beego.beego.server.web.model.yml b/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
diff --git a/go/ql/lib/ext/github.com.gofiber.fiber.model.yml b/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"]
diff --git a/go/ql/lib/ext/github.com.labstack.echo.model.yml b/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
diff --git a/go/ql/lib/ext/github.com.revel.revel.model.yml b/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
diff --git a/go/ql/lib/ext/github.com.valyala.fasthttp.model.yml b/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
diff --git a/go/ql/lib/ext/gopkg.in.macaron.model.yml b/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
diff --git a/go/ql/lib/ext/net.http.model.yml b/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
diff --git a/go/ql/lib/semmle/go/concepts/HTTP.qll b/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)
+      }
+    }
   }
 
   /**
diff --git a/go/ql/lib/semmle/go/frameworks/Beego.qll b/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") }
 
diff --git a/go/ql/lib/semmle/go/frameworks/Echo.qll b/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.
    *
diff --git a/go/ql/lib/semmle/go/frameworks/Fasthttp.qll b/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.
      */
diff --git a/go/ql/lib/semmle/go/frameworks/Macaron.qll b/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() }
-  }
 }
diff --git a/go/ql/lib/semmle/go/frameworks/Revel.qll b/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.
    */
diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll b/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() {
diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/Regexp.qll b/go/ql/lib/semmle/go/frameworks/stdlib/Regexp.qll
@@ -20,20 +20,18 @@ module Regexp {
    * function does not compile the regex.
    */
   private predicate regexSinkKindInfo(string kind, int strArg) {
+    strArg = -3 and
+    kind = "regex-use"
+    or
     sinkModel(_, _, _, _, _, _, _, kind, _, _) and
     exists(string strArgStr |
-      (
-        strArgStr.toInt() = strArg
-        or
-        strArg = -2 and
-        strArgStr = "c"
-      )
+      strArgStr.toInt() = strArg
+      or
+      strArg = -2 and
+      strArgStr = "c"
     |
       kind = "regex-use[" + strArgStr + "]"
     )
-    or
-    strArg = -3 and
-    kind = "regex-use"
   }
 
   private class DefaultRegexpPattern extends RegexpPattern::Range, DataFlow::ArgumentNode {
diff --git a/go/ql/src/experimental/frameworks/CleverGo.qll b/go/ql/src/experimental/frameworks/CleverGo.qll
@@ -171,25 +171,6 @@ private module CleverGo {
     }
   }
 
-  /**
-   * Models HTTP redirects.
-   */
-  private class HttpRedirect extends Http::Redirect::Range, DataFlow::CallNode {
-    DataFlow::Node urlNode;
-
-    HttpRedirect() {
-      // HTTP redirect models for package: clevergo.tech/clevergo@v0.5.2
-      // Receiver type: Context
-      // signature: func (*Context) Redirect(code int, url string) error
-      this = any(Method m | m.hasQualifiedName(packagePath(), "Context", "Redirect")).getACall() and
-      urlNode = this.getArgument(1)
-    }
-
-    override DataFlow::Node getUrl() { result = urlNode }
-
-    override Http::ResponseWriter getResponseWriter() { result.getANode() = this.getReceiver() }
-  }
-
   /**
    * Models HTTP ResponseBody where the content-type is static and non-modifiable.
    */
diff --git a/go/ql/src/experimental/frameworks/Fiber.qll b/go/ql/src/experimental/frameworks/Fiber.qll
@@ -126,25 +126,6 @@ private module Fiber {
     }
   }
 
-  /**
-   * Models HTTP redirects.
-   */
-  private class Redirect extends Http::Redirect::Range, DataFlow::CallNode {
-    DataFlow::Node urlNode;
-
-    Redirect() {
-      // HTTP redirect models for package: github.com/gofiber/fiber@v1.14.6
-      // Receiver type: Ctx
-      // signature: func (*Ctx) Redirect(location string, status ...int)
-      this = any(Method m | m.hasQualifiedName(fiberPackagePath(), "Ctx", "Redirect")).getACall() and
-      urlNode = this.getArgument(0)
-    }
-
-    override DataFlow::Node getUrl() { result = urlNode }
-
-    override Http::ResponseWriter getResponseWriter() { result.getANode() = this.getReceiver() }
-  }
-
   /**
    * Models HTTP header writers.
    * The write is done with a call where you can set both the key and the value of the header.
diff --git a/go/ql/test/library-tests/semmle/go/frameworks/Echo/OpenRedirect.expected b/go/ql/test/library-tests/semmle/go/frameworks/Echo/OpenRedirect.expected
@@ -2,16 +2,17 @@
 | test.go:173:20:173:24 | param | test.go:172:11:172:32 | call to Param | test.go:173:20:173:24 | param | This path to an untrusted URL redirection depends on a $@. | test.go:172:11:172:32 | call to Param | user-provided value |
 | test.go:182:20:182:28 | ...+... | test.go:178:11:178:32 | call to Param | test.go:182:20:182:28 | ...+... | This path to an untrusted URL redirection depends on a $@. | test.go:178:11:178:32 | call to Param | user-provided value |
 edges
-| test.go:172:11:172:32 | call to Param | test.go:173:20:173:24 | param | provenance | Src:MaD:1  |
-| test.go:178:11:178:32 | call to Param | test.go:182:24:182:28 | param | provenance | Src:MaD:1  |
-| test.go:182:24:182:28 | param | test.go:182:20:182:28 | ...+... | provenance | Config |
+| test.go:172:11:172:32 | call to Param | test.go:173:20:173:24 | param | provenance | Src:MaD:2  Sink:MaD:1 |
+| test.go:178:11:178:32 | call to Param | test.go:182:24:182:28 | param | provenance | Src:MaD:2  |
+| test.go:182:24:182:28 | param | test.go:182:20:182:28 | ...+... | provenance | Config Sink:MaD:1 |
 | test.go:190:9:190:26 | star expression | test.go:190:10:190:26 | selection of URL | provenance | Config |
 | test.go:190:9:190:26 | star expression | test.go:193:21:193:23 | url | provenance |  |
-| test.go:190:10:190:26 | selection of URL | test.go:190:9:190:26 | star expression | provenance | Src:MaD:2 Config |
-| test.go:193:21:193:23 | url | test.go:193:21:193:32 | call to String | provenance | Config |
+| test.go:190:10:190:26 | selection of URL | test.go:190:9:190:26 | star expression | provenance | Src:MaD:3 Config |
+| test.go:193:21:193:23 | url | test.go:193:21:193:32 | call to String | provenance | Config Sink:MaD:1 |
 models
-| 1 | Source: github.com/labstack/echo; Context; true; Param; ; ; ReturnValue[0]; remote; manual |
-| 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual |
+| 1 | Sink: github.com/labstack/echo; Context; true; Redirect; ; ; Argument[1]; url-redirection; manual |
+| 2 | Source: github.com/labstack/echo; Context; true; Param; ; ; ReturnValue[0]; remote; manual |
+| 3 | Source: net/http; Request; true; URL; ; ; ; remote; manual |
 nodes
 | test.go:172:11:172:32 | call to Param | semmle.label | call to Param |
 | test.go:173:20:173:24 | param | semmle.label | param |
diff --git a/go/ql/test/library-tests/semmle/go/frameworks/Revel/OpenRedirect.expected b/go/ql/test/library-tests/semmle/go/frameworks/Revel/OpenRedirect.expected
@@ -3,11 +3,12 @@
 edges
 | EndToEnd.go:94:20:94:27 | implicit dereference | EndToEnd.go:94:20:94:27 | selection of Params | provenance | Config |
 | EndToEnd.go:94:20:94:27 | implicit dereference | EndToEnd.go:94:20:94:32 | selection of Form | provenance | Config |
-| EndToEnd.go:94:20:94:27 | selection of Params | EndToEnd.go:94:20:94:27 | implicit dereference | provenance | Src:MaD:1 Config |
-| EndToEnd.go:94:20:94:27 | selection of Params | EndToEnd.go:94:20:94:32 | selection of Form | provenance | Src:MaD:1 Config |
-| EndToEnd.go:94:20:94:32 | selection of Form | EndToEnd.go:94:20:94:49 | call to Get | provenance | Config |
+| EndToEnd.go:94:20:94:27 | selection of Params | EndToEnd.go:94:20:94:27 | implicit dereference | provenance | Src:MaD:2 Config |
+| EndToEnd.go:94:20:94:27 | selection of Params | EndToEnd.go:94:20:94:32 | selection of Form | provenance | Src:MaD:2 Config |
+| EndToEnd.go:94:20:94:32 | selection of Form | EndToEnd.go:94:20:94:49 | call to Get | provenance | Config Sink:MaD:1 |
 models
-| 1 | Source: group:revel; Controller; true; Params; ; ; ; remote; manual |
+| 1 | Sink: group:revel; Controller; true; Redirect; ; ; Argument[0]; url-redirection; manual |
+| 2 | Source: group:revel; Controller; true; Params; ; ; ; remote; manual |
 nodes
 | EndToEnd.go:94:20:94:27 | implicit dereference | semmle.label | implicit dereference |
 | EndToEnd.go:94:20:94:27 | selection of Params | semmle.label | selection of Params |
diff --git a/go/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected b/go/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected
diff --git a/go/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected b/go/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected
