Merge pull request #344 from smowton/smowton/feature/echo-models

Add models for the Echo framework

Author: max-schaefer

change-notes/2020-09-17-echo.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Added support for the Echo web framework

ql/src/go.qll

@@ -27,6 +27,7 @@ import semmle.go.dataflow.GlobalValueNumbering
 import semmle.go.dataflow.SSA
 import semmle.go.dataflow.TaintTracking
 import semmle.go.frameworks.Chi
+import semmle.go.frameworks.Echo
 import semmle.go.frameworks.Email
 import semmle.go.frameworks.Encoding
 import semmle.go.frameworks.Gin

ql/src/semmle/go/Concepts.qll

@@ -509,6 +509,17 @@ module HTTP {
     abstract class Range extends DataFlow::Node {
       /** Gets the response writer associated with this header write, if any. */
       abstract ResponseWriter getResponseWriter();
+
+      /** Gets a content-type associated with this body. */
+      string getAContentType() { result = getAContentTypeNode().getStringValue() }
+
+      /** Gets a dataflow node for a content-type associated with this body. */
+      DataFlow::Node getAContentTypeNode() {
+        exists(HTTP::HeaderWrite hw | hw = getResponseWriter().getAHeaderWrite() |
+          hw.getName().getStringValue().toLowerCase() = "content-type" and
+          result = hw.getValue()
+        )
+      }
     }
   }
 
@@ -525,6 +536,12 @@ module HTTP {
 
     /** Gets the response writer associated with this header write, if any. */
     ResponseWriter getResponseWriter() { result = self.getResponseWriter() }
+
+    /** Gets a content-type associated with this body. */
+    string getAContentType() { result = self.getAContentType() }
+
+    /** Gets a dataflow node for a content-type associated with this body. */
+    DataFlow::Node getAContentTypeNode() { result = self.getAContentTypeNode() }
   }
 
   /** Provides a class for modeling new HTTP client request APIs. */
@@ -576,7 +593,7 @@ module HTTP {
       /** Gets the data-flow node representing the URL being redirected to. */
       abstract DataFlow::Node getUrl();
 
-      /** Gets the response writer that this redirect is sent on. */
+      /** Gets the response writer that this redirect is sent on, if any. */
       abstract ResponseWriter getResponseWriter();
     }
 
@@ -591,6 +608,12 @@ module HTTP {
 
       override ResponseWriter getResponseWriter() { result = HeaderWrite.super.getResponseWriter() }
     }
+
+    /**
+     * An HTTP request attribute that is generally not attacker-controllable for
+     * open redirect exploits; for example, a form field submitted in a POST request.
+     */
+    abstract class UnexploitableSource extends DataFlow::Node { }
   }
 
   /**
@@ -607,7 +630,7 @@ module HTTP {
     /** Gets the data-flow node representing the URL being redirected to. */
     DataFlow::Node getUrl() { result = self.getUrl() }
 
-    /** Gets the response writer that this redirect is sent on. */
+    /** Gets the response writer that this redirect is sent on, if any. */
     ResponseWriter getResponseWriter() { result = self.getResponseWriter() }
   }
 }

ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll

@@ -51,6 +51,20 @@ class FunctionInput extends TFunctionInput {
   abstract string toString();
 }
 
+module FunctionInput {
+  /** Gets a `FunctionInput` representing the `i`th parameter. */
+  FunctionInput parameter(int i) { result.isParameter(i) }
+
+  /** Gets a `FunctionInput` representing the receiver. */
+  FunctionInput receiver() { result.isReceiver() }
+
+  /** Gets a `FunctionInput` representing the result of a single-result function. */
+  FunctionInput functionResult() { result.isResult() }
+
+  /** Gets a `FunctionInput` representing the `i`th result. */
+  FunctionInput functionResult(int i) { result.isParameter(i) }
+}
+
 /** A parameter position of a function, viewed as a source of input. */
 private class ParameterInput extends FunctionInput, TInParameter {
   int index;
@@ -172,6 +186,20 @@ class FunctionOutput extends TFunctionOutput {
   abstract string toString();
 }
 
+module FunctionOutput {
+  /** Gets a `FunctionOutput` representing the result of a single-result function. */
+  FunctionOutput functionResult() { result.isResult() }
+
+  /** Gets a `FunctionOutput` representing the `i`th result. */
+  FunctionOutput functionResult(int i) { result.isParameter(i) }
+
+  /** Gets a `FunctionOutput` representing the receiver after a function returns. */
+  FunctionOutput receiver() { result.isReceiver() }
+
+  /** Gets a `FunctionOutput` representing the `i`th parameter after a function returns. */
+  FunctionOutput parameter(int i) { result.isParameter(i) }
+}
+
 /** A result position of a function, viewed as an output. */
 private class OutResult extends FunctionOutput, TOutResult {
   int index;

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

@@ -0,0 +1,122 @@
+/**
+ * Provides classes for working with untrusted flow sources, taint propagators, and HTTP sinks
+ * from the `github.com/labstack/echo` package.
+ */
+
+import go
+
+private module Echo {
+  /** Gets an Echo package name. */
+  bindingset[result]
+  private string packagePath() { result = package("github.com/labstack/echo", "") }
+
+  /**
+   * Data from a `Context` interface method, considered as a source of untrusted flow.
+   */
+  private class EchoContextSource extends UntrustedFlowSource::Range {
+    EchoContextSource() {
+      exists(DataFlow::MethodCallNode call, string methodName |
+        methodName =
+          ["Param", "ParamValues", "QueryParam", "QueryParams", "QueryString", "FormValue",
+              "FormParams", "FormFile", "MultipartForm", "Cookie", "Cookies"] and
+        call.getTarget().hasQualifiedName(packagePath(), "Context", methodName) and
+        this = call.getResult(0)
+      )
+    }
+  }
+
+  /**
+   * Data from a `Context` interface method that is not generally exploitable for open-redirect attacks.
+   */
+  private class EchoContextRedirectUnexploitableSource extends HTTP::Redirect::UnexploitableSource {
+    EchoContextRedirectUnexploitableSource() {
+      exists(DataFlow::MethodCallNode call, string methodName |
+        methodName = ["FormValue", "FormParams", "FormFile", "MultipartForm", "Cookie", "Cookies"] and
+        call.getTarget().hasQualifiedName(packagePath(), "Context", methodName) and
+        this = call.getResult(0)
+      )
+    }
+  }
+
+  /**
+   * Models of `Context.Get/Set`. `Context` behaves like a map, with corresponding taint propagation.
+   */
+  private class ContextMapModels extends TaintTracking::FunctionModel, Method {
+    string methodName;
+    FunctionInput input;
+    FunctionOutput output;
+
+    ContextMapModels() {
+      (
+        methodName = "Get" and input.isReceiver() and output.isResult()
+        or
+        methodName = "Set" and input.isParameter(1) and output.isReceiver()
+      ) and
+      this.hasQualifiedName(packagePath(), "Context", methodName)
+    }
+
+    override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
+      inp = input and outp = output
+    }
+  }
+
+  /**
+   * A call to a method on `Context` struct that unmarshals data into a target.
+   */
+  private class EchoContextBinder extends UntrustedFlowSource::Range {
+    EchoContextBinder() {
+      exists(DataFlow::MethodCallNode call |
+        call.getTarget().hasQualifiedName(packagePath(), "Context", "Bind")
+      |
+        this = FunctionOutput::parameter(0).getExitNode(call)
+      )
+    }
+  }
+
+  /**
+   * `echo.Context` methods which set the content-type to `text/html` and write a result in one operation.
+   */
+  private class EchoHtmlOutputs extends HTTP::ResponseBody::Range {
+    EchoHtmlOutputs() {
+      exists(Method m | m.hasQualifiedName(packagePath(), "Context", ["HTML", "HTMLBlob"]) |
+        this = m.getACall().getArgument(1)
+      )
+    }
+
+    override HTTP::ResponseWriter getResponseWriter() { none() }
+
+    override string getAContentType() { result = "text/html" }
+  }
+
+  /**
+   * `echo.Context` methods which take a content-type as a parameter.
+   */
+  private class EchoParameterizedOutputs extends HTTP::ResponseBody::Range {
+    DataFlow::CallNode callNode;
+
+    EchoParameterizedOutputs() {
+      exists(Method m | m.hasQualifiedName(packagePath(), "Context", ["Blob", "Stream"]) |
+        callNode = m.getACall() and this = callNode.getArgument(2)
+      )
+    }
+
+    override HTTP::ResponseWriter getResponseWriter() { none() }
+
+    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() }
+  }
+}

ql/src/semmle/go/frameworks/Gin.qll

@@ -128,7 +128,7 @@ private module Gin {
             methodName = "ShouldBindYAML"
           )
         |
-          this = any(FunctionOutput output | output.isParameter(0)).getExitNode(call)
+          this = FunctionOutput::parameter(0).getExitNode(call)
         )
       )
     }

ql/src/semmle/go/frameworks/HTTP.qll

@@ -37,7 +37,7 @@ private module GoRestfulHttp {
             .getTarget()
             .hasQualifiedName(package("github.com/emicklei/go-restful", ""), "Request", "ReadEntity")
       |
-        this = any(FunctionOutput output | output.isParameter(0)).getExitNode(call)
+        this = FunctionOutput::parameter(0).getExitNode(call)
       )
     }
   }

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

@@ -216,6 +216,25 @@ module NetHttp {
     }
   }
 
+  /** Fields and methods of `net/http.Request` that are not generally exploitable in an open-redirect attack. */
+  private class RedirectUnexploitableRequestFields extends HTTP::Redirect::UnexploitableSource {
+    RedirectUnexploitableRequestFields() {
+      exists(Field f, string fieldName |
+        f.hasQualifiedName("net/http", "Request", fieldName) and
+        this = f.getARead()
+      |
+        fieldName = ["Body", "GetBody", "PostForm", "MultipartForm", "Header", "Trailer"]
+      )
+      or
+      exists(Method m, string methName |
+        m.hasQualifiedName("net/http", "Request", methName) and
+        this = m.getACall()
+      |
+        methName = ["Cookie", "Cookies", "MultipartReader", "PostFormValue", "Referer", "UserAgent"]
+      )
+    }
+  }
+
   private class FunctionModels extends TaintTracking::FunctionModel {
     FunctionInput inp;
     FunctionOutput outp;

ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll

@@ -41,28 +41,7 @@ module OpenUrlRedirect {
     UntrustedFlowAsSource() {
       // exclude some fields and methods of URLs that are generally not attacker-controllable for
       // open redirect exploits
-      not exists(Field f, string fieldName |
-        f.hasQualifiedName("net/http", "Request", fieldName) and
-        this = f.getARead()
-      |
-        fieldName = "Body" or
-        fieldName = "GetBody" or
-        fieldName = "PostForm" or
-        fieldName = "MultipartForm" or
-        fieldName = "Header" or
-        fieldName = "Trailer"
-      ) and
-      not exists(Method m, string methName |
-        m.hasQualifiedName("net/http", "Request", methName) and
-        this = m.getACall()
-      |
-        methName = "Cookie" or
-        methName = "Cookies" or
-        methName = "MultipartReader" or
-        methName = "PostFormValues" or
-        methName = "Referer" or
-        methName = "UserAgent"
-      )
+      not this instanceof HTTP::Redirect::UnexploitableSource
     }
   }
 

ql/src/semmle/go/security/ReflectedXssCustomizations.qll

@@ -47,9 +47,7 @@ module ReflectedXss {
    * Holds if `body` specifies the response's content type to be HTML.
    */
   private predicate htmlTypeSpecified(HTTP::ResponseBody body) {
-    exists(HTTP::HeaderWrite hw, string tp | hw = body.getResponseWriter().getAHeaderWrite() |
-      hw.definesHeader("content-type", tp) and tp.regexpMatch("(?i).*html.*")
-    )
+    body.getAContentType().regexpMatch("(?i).*html.*")
   }
 
   /**
@@ -58,9 +56,7 @@ module ReflectedXss {
   private predicate nonHtmlContentType(HTTP::ResponseBody body) {
     not htmlTypeSpecified(body) and
     (
-      exists(HTTP::HeaderWrite hw | hw = body.getResponseWriter().getAHeaderWrite() |
-        hw.getName().getStringValue().toLowerCase() = "content-type"
-      )
+      exists(body.getAContentTypeNode())
       or
       exists(DataFlow::CallNode call | call.getTarget().hasQualifiedName("fmt", "Fprintf") |
         body = call.getAnArgument() and