Merge pull request #21015 from aschackmull/go/mad-barriers

Go: Support for MaD barriers and barrier guards.

Author: owen-mc

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

@@ -50,3 +50,8 @@ extensions:
       - ["group:beego", "Controller", True, "GetString", "", "", "ReturnValue[0]", "remote", "manual"]
       - ["group:beego", "Controller", True, "GetStrings", "", "", "ReturnValue[0]", "remote", "manual"]
       - ["group:beego", "Controller", True, "Input", "", "", "ReturnValue[0]", "remote", "manual"]
+  - addsTo:
+      pack: codeql/go-all
+      extensible: barrierModel
+    data:
+      - ["group:beego", "", True, "Htmlquote", "", "", "ReturnValue", "html-injection", "manual"]

go/ql/lib/ext/mime.multipart.model.yml

@@ -1,4 +1,21 @@
 extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: barrierModel
+    data:
+      #  The only way to create a `mime/multipart.FileHeader` is to create a
+      #  `mime/multipart.Form`, which creates the `Filename` field of each
+      #  `mime/multipart.FileHeader` by calling `Part.FileName`, which calls
+      #  `path/filepath.Base` on its return value. In general `path/filepath.Base`
+      #  is not a sanitizer for path traversal, but in this specific case where the
+      #  output is going to be used as a filename rather than a directory name, it
+      #  is adequate.
+      - ["mime/multipart", "FileHeader", False, "Filename", "", "", "", "path-injection", "manual"]
+      # `Part.FileName` calls `path/filepath.Base` on its return value. In
+      # general `path/filepath.Base` is not a sanitizer for path traversal, but in
+      # this specific case where the output is going to be used as a filename
+      # rather than a directory name, it is adequate.
+      - ["mime/multipart", "Part", False, "FileName", "", "", "ReturnValue", "path-injection", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/ext/path.filepath.model.yml

@@ -1,4 +1,9 @@
 extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: barrierModel
+    data:
+      - ["path/filepath", "", False, "Rel", "", "", "ReturnValue", "path-injection", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: summaryModel

go/ql/lib/semmle/go/Concepts.qll

@@ -116,10 +116,10 @@ module FileSystemAccess {
   }
 }
 
-private class DefaultFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
+private class ExternalFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
   DataFlow::ArgumentNode pathArgument;
 
-  DefaultFileSystemAccess() {
+  ExternalFileSystemAccess() {
     sinkNode(pathArgument, "path-injection") and
     this = pathArgument.getCall()
   }
@@ -394,10 +394,10 @@ module LoggerCall {
   }
 }
 
-private class DefaultLoggerCall extends LoggerCall::Range, DataFlow::CallNode {
+private class ExternalLoggerCall extends LoggerCall::Range, DataFlow::CallNode {
   DataFlow::ArgumentNode messageComponent;
 
-  DefaultLoggerCall() {
+  ExternalLoggerCall() {
     sinkNode(messageComponent, "log-injection") and
     this = messageComponent.getCall()
   }

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

@@ -320,11 +320,11 @@ module Http {
       )
     }
 
-    private class DefaultHttpRedirect extends Range, DataFlow::CallNode {
+    private class ExternalHttpRedirect extends Range, DataFlow::CallNode {
       DataFlow::ArgumentNode url;
       int rw;
 
-      DefaultHttpRedirect() {
+      ExternalHttpRedirect() {
         this = url.getCall() and
         exists(string kind |
           sinkKindInfo(kind, rw) and

go/ql/lib/semmle/go/dataflow/ExternalFlow.qll

@@ -129,7 +129,9 @@ module ModelValidation {
     summaryModel(_, _, _, _, _, _, path, _, _, _, _) or
     summaryModel(_, _, _, _, _, _, _, path, _, _, _) or
     sinkModel(_, _, _, _, _, _, path, _, _, _) or
-    sourceModel(_, _, _, _, _, _, path, _, _, _)
+    sourceModel(_, _, _, _, _, _, path, _, _, _) or
+    barrierModel(_, _, _, _, _, _, path, _, _, _) or
+    barrierGuardModel(_, _, _, _, _, _, path, _, _, _, _)
   }
 
   private module MkAccessPath = AccessPathSyntax::AccessPath<getRelevantAccessPath/1>;
@@ -142,6 +144,8 @@ module ModelValidation {
     exists(string pred, AccessPath input, AccessPathToken part |
       sinkModel(_, _, _, _, _, _, input, _, _, _) and pred = "sink"
       or
+      barrierGuardModel(_, _, _, _, _, _, input, _, _, _, _) and pred = "barrier guard"
+      or
       summaryModel(_, _, _, _, _, _, input, _, _, _, _) and pred = "summary"
     |
       (
@@ -164,6 +168,8 @@ module ModelValidation {
     exists(string pred, AccessPath output, AccessPathToken part |
       sourceModel(_, _, _, _, _, _, output, _, _, _) and pred = "source"
       or
+      barrierModel(_, _, _, _, _, _, output, _, _, _) and pred = "barrier"
+      or
       summaryModel(_, _, _, _, _, _, _, output, _, _, _) and pred = "summary"
     |
       (
@@ -181,7 +187,13 @@ module ModelValidation {
   private module KindValConfig implements SharedModelVal::KindValidationConfigSig {
     predicate summaryKind(string kind) { summaryModel(_, _, _, _, _, _, _, _, kind, _, _) }
 
-    predicate sinkKind(string kind) { sinkModel(_, _, _, _, _, _, _, kind, _, _) }
+    predicate sinkKind(string kind) {
+      sinkModel(_, _, _, _, _, _, _, kind, _, _)
+      or
+      barrierModel(_, _, _, _, _, _, _, kind, _, _)
+      or
+      barrierGuardModel(_, _, _, _, _, _, _, _, kind, _, _)
+    }
 
     predicate sourceKind(string kind) { sourceModel(_, _, _, _, _, _, _, kind, _, _) }
 
@@ -199,6 +211,11 @@ module ModelValidation {
       or
       sinkModel(package, type, _, name, signature, ext, _, _, provenance, _) and pred = "sink"
       or
+      barrierModel(package, type, _, name, signature, ext, _, _, provenance, _) and pred = "barrier"
+      or
+      barrierGuardModel(package, type, _, name, signature, ext, _, _, _, provenance, _) and
+      pred = "barrier guard"
+      or
       summaryModel(package, type, _, name, signature, ext, _, _, _, provenance, _) and
       pred = "summary"
       or
@@ -224,6 +241,14 @@ module ModelValidation {
       invalidProvenance(provenance) and
       result = "Unrecognized provenance description \"" + provenance + "\" in " + pred + " model."
     )
+    or
+    exists(string acceptingvalue |
+      barrierGuardModel(_, _, _, _, _, _, _, acceptingvalue, _, _, _) and
+      invalidAcceptingValue(acceptingvalue) and
+      result =
+        "Unrecognized accepting value description \"" + acceptingvalue +
+          "\" in barrier guard model."
+    )
   }
 
   private string getInvalidPackageGroup() {
@@ -232,6 +257,11 @@ module ModelValidation {
       or
       FlowExtensions::sinkModel(package, _, _, _, _, _, _, _, _, _) and pred = "sink"
       or
+      FlowExtensions::barrierModel(package, _, _, _, _, _, _, _, _, _) and pred = "barrier"
+      or
+      FlowExtensions::barrierGuardModel(package, _, _, _, _, _, _, _, _, _, _) and
+      pred = "barrier guard"
+      or
       FlowExtensions::summaryModel(package, _, _, _, _, _, _, _, _, _, _) and
       pred = "summary"
       or
@@ -262,6 +292,10 @@ private predicate elementSpec(
   or
   sinkModel(package, type, subtypes, name, signature, ext, _, _, _, _)
   or
+  barrierModel(package, type, subtypes, name, signature, ext, _, _, _, _)
+  or
+  barrierGuardModel(package, type, subtypes, name, signature, ext, _, _, _, _, _)
+  or
   summaryModel(package, type, subtypes, name, signature, ext, _, _, _, _, _)
   or
   neutralModel(package, type, name, signature, _, _) and ext = "" and subtypes = false
@@ -397,6 +431,54 @@ private module Cached {
       isSinkNode(n, kind, model) and n.asNode() = node
     )
   }
+
+  private newtype TKindModelPair =
+    TMkPair(string kind, string model) { isBarrierGuardNode(_, _, kind, model) }
+
+  private boolean convertAcceptingValue(Public::AcceptingValue av) {
+    av.isTrue() and result = true
+    or
+    av.isFalse() and result = false
+    // Remaining cases are not supported yet, they depend on the shared Guards library.
+    // or
+    // av.isNoException() and result.getDualValue().isThrowsException()
+    // or
+    // av.isZero() and result.asIntValue() = 0
+    // or
+    // av.isNotZero() and result.getDualValue().asIntValue() = 0
+    // or
+    // av.isNull() and result.isNullValue()
+    // or
+    // av.isNotNull() and result.isNonNullValue()
+  }
+
+  private predicate barrierGuardChecks(DataFlow::Node g, Expr e, boolean gv, TKindModelPair kmp) {
+    exists(
+      SourceSinkInterpretationInput::InterpretNode n, Public::AcceptingValue acceptingvalue,
+      string kind, string model
+    |
+      isBarrierGuardNode(n, acceptingvalue, kind, model) and
+      n.asNode().asExpr() = e and
+      kmp = TMkPair(kind, model) and
+      gv = convertAcceptingValue(acceptingvalue)
+    |
+      g.asExpr().(CallExpr).getAnArgument() = e // TODO: qualifier?
+    )
+  }
+
+  /**
+   * Holds if `node` is specified as a barrier with the given kind in a MaD flow
+   * model.
+   */
+  cached
+  predicate barrierNode(DataFlow::Node node, string kind, string model) {
+    exists(SourceSinkInterpretationInput::InterpretNode n |
+      isBarrierNode(n, kind, model) and n.asNode() = node
+    )
+    or
+    DataFlow::ParameterizedBarrierGuard<TKindModelPair, barrierGuardChecks/4>::getABarrierNode(TMkPair(kind,
+        model)) = node
+  }
 }
 
 import Cached
@@ -413,6 +495,12 @@ predicate sourceNode(DataFlow::Node node, string kind) { sourceNode(node, kind,
  */
 predicate sinkNode(DataFlow::Node node, string kind) { sinkNode(node, kind, _) }
 
+/**
+ * Holds if `node` is specified as a barrier with the given kind in a MaD flow
+ * model.
+ */
+predicate barrierNode(DataFlow::Node node, string kind) { barrierNode(node, kind, _) }
+
 // adapter class for converting Mad summaries to `SummarizedCallable`s
 private class SummarizedCallableAdapter extends Public::SummarizedCallable {
   SummarizedCallableAdapter() { summaryElement(this, _, _, _, _, _) }

go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -339,29 +339,67 @@ class ContentSet instanceof TContentSet {
  */
 signature predicate guardChecksSig(Node g, Expr e, boolean branch);
 
+bindingset[this]
+private signature class ParamSig;
+
+private module WithParam<ParamSig P> {
+  /**
+   * Holds if the guard `g` validates the expression `e` upon evaluating to `branch`.
+   *
+   * The expression `e` is expected to be a syntactic part of the guard `g`.
+   * For example, the guard `g` might be a call `isSafe(x)` and the expression `e`
+   * the argument `x`.
+   */
+  signature predicate guardChecksSig(Node g, Expr e, boolean branch, P param);
+}
+
 /**
  * Provides a set of barrier nodes for a guard that validates an expression.
  *
  * This is expected to be used in `isBarrier`/`isSanitizer` definitions
  * in data flow and taint tracking.
  */
 module BarrierGuard<guardChecksSig/3 guardChecks> {
+  private predicate guardChecks(Node g, Expr e, boolean branch, Unit param) {
+    guardChecks(g, e, branch) and exists(param)
+  }
+
+  private module B = ParameterizedBarrierGuard<Unit, guardChecks/4>;
+
+  /** Gets a node that is safely guarded by the given guard check. */
+  Node getABarrierNode() { result = B::getABarrierNode(_) }
+
+  /**
+   * Gets a node that is safely guarded by the given guard check.
+   */
+  Node getABarrierNodeForGuard(Node guardCheck) {
+    result = B::getABarrierNodeForGuard(guardCheck, _)
+  }
+}
+
+/**
+ * Provides a set of barrier nodes for a guard that validates an expression.
+ *
+ * This is expected to be used in `isBarrier`/`isSanitizer` definitions
+ * in data flow and taint tracking.
+ */
+module ParameterizedBarrierGuard<ParamSig P, WithParam<P>::guardChecksSig/4 guardChecks> {
   /** Gets a node that is safely guarded by the given guard check. */
-  Node getABarrierNode() {
+  Node getABarrierNode(P param) {
     exists(ControlFlow::ConditionGuardNode guard, SsaWithFields var |
       result = pragma[only_bind_out](var).getAUse()
     |
-      guards(_, guard, _, var) and
+      guards(_, guard, _, var, param) and
       pragma[only_bind_out](guard).dominates(result.getBasicBlock())
     )
   }
 
   /**
    * Gets a node that is safely guarded by the given guard check.
    */
-  Node getABarrierNodeForGuard(Node guardCheck) {
+  Node getABarrierNodeForGuard(Node guardCheck, P param) {
     exists(ControlFlow::ConditionGuardNode guard, SsaWithFields var | result = var.getAUse() |
-      guards(guardCheck, guard, _, var) and
+      guards(guardCheck, guard, _, var, param) and
       guard.dominates(result.getBasicBlock())
     )
   }
@@ -373,22 +411,24 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
    * This predicate exists to enforce a good join order in `getAGuardedNode`.
    */
   pragma[noinline]
-  private predicate guards(Node g, ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap) {
-    guards(g, guard, nd) and nd = ap.getAUse()
+  private predicate guards(
+    Node g, ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap, P param
+  ) {
+    guards(g, guard, nd, param) and nd = ap.getAUse()
   }
 
   /**
    * Holds if `guard` marks a point in the control-flow graph where `g`
    * is known to validate `nd`.
    */
-  private predicate guards(Node g, ControlFlow::ConditionGuardNode guard, Node nd) {
+  private predicate guards(Node g, ControlFlow::ConditionGuardNode guard, Node nd, P param) {
     exists(boolean branch |
-      guardChecks(g, nd.asExpr(), branch) and
+      guardChecks(g, nd.asExpr(), branch, param) and
       guard.ensures(g, branch)
     )
     or
     exists(DataFlow::Property p, Node resNode, Node check, boolean outcome |
-      guardingCall(g, _, _, _, p, _, nd, resNode) and
+      guardingCall(g, _, _, _, p, _, nd, resNode, param) and
       p.checkOn(check, outcome, resNode) and
       guard.ensures(pragma[only_bind_into](check), outcome)
     )
@@ -405,9 +445,9 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
   pragma[noinline]
   private predicate guardingCall(
     Node g, Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p, CallNode c,
-    Node nd, Node resNode
+    Node nd, Node resNode, P param
   ) {
-    guardingFunction(g, f, inp, outp, p) and
+    guardingFunction(g, f, inp, outp, p, param) and
     c = f.getACall() and
     nd = getInputNode(inp, c) and
     localFlow(getOutputNode(outp, c), resNode)
@@ -438,15 +478,15 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
    * `false`, `nil` or a non-`nil` value.)
    */
   private predicate guardingFunction(
-    Node g, Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p
+    Node g, Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p, P param
   ) {
     exists(FuncDecl fd, Node arg, Node ret |
       fd.getFunction() = f and
       localFlow(inp.getExitNode(fd), pragma[only_bind_out](arg)) and
       (
         // Case: a function like "if someBarrierGuard(arg) { return true } else { return false }"
         exists(ControlFlow::ConditionGuardNode guard |
-          guards(g, pragma[only_bind_out](guard), arg) and
+          guards(g, pragma[only_bind_out](guard), arg, param) and
           guard.dominates(pragma[only_bind_out](ret).getBasicBlock())
         |
           onlyPossibleReturnSatisfyingProperty(fd, outp, ret, p)
@@ -456,7 +496,7 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
         // or "return !someBarrierGuard(arg) && otherCond(...)"
         exists(boolean outcome |
           ret = getUniqueOutputNode(fd, outp) and
-          guardChecks(g, arg.asExpr(), outcome) and
+          guardChecks(g, arg.asExpr(), outcome, param) and
           // This predicate's contract is (p holds of ret ==> arg is checked),
           // (and we have (this has outcome ==> arg is checked))
           // but p.checkOn(ret, outcome, this) gives us (ret has outcome ==> p holds of this),
@@ -471,7 +511,7 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
           DataFlow::Property outpProp
         |
           ret = getUniqueOutputNode(fd, outp) and
-          guardingFunction(g, f2, inp2, outp2, outpProp) and
+          guardingFunction(g, f2, inp2, outp2, outpProp, param) and
           c = f2.getACall() and
           arg = inp2.getNode(c) and
           (