diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll b/ql/src/semmle/go/controlflow/ControlFlowGraph.qll index 65f58795263..6d5a3f618ea 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll +++ b/ql/src/semmle/go/controlflow/ControlFlowGraph.qll @@ -57,6 +57,18 @@ module ControlFlow { /** Gets the basic block to which this node belongs. */ BasicBlock getBasicBlock() { result.getANode() = this } + /** Holds if this node dominates `dominee` in the control-flow graph. */ + pragma[inline] + predicate dominatesNode(ControlFlow::Node dominee) { + exists(ReachableBasicBlock thisbb, ReachableBasicBlock dbb, int i, int j | + this = thisbb.getNode(i) and dominee = dbb.getNode(j) + | + thisbb.strictlyDominates(dbb) + or + thisbb = dbb and i <= j + ) + } + /** Gets the innermost function or file to which this node belongs. */ Root getRoot() { none() } diff --git a/ql/src/semmle/go/dataflow/SSA.qll b/ql/src/semmle/go/dataflow/SSA.qll index be04265ab90..3add50eb220 100644 --- a/ql/src/semmle/go/dataflow/SSA.qll +++ b/ql/src/semmle/go/dataflow/SSA.qll @@ -288,3 +288,67 @@ class SsaPhiNode extends SsaPseudoDefinition, TPhi { getBasicBlock().hasLocationInfo(filepath, startline, startcolumn, _, _) } } + +/** + * An SSA variable, possibly with a chain of field reads on it. + */ +private newtype TSsaWithFields = + TRoot(SsaVariable v) or + TStep(SsaWithFields base, Field f) { exists(accessPathAux(base, f)) } + +/** + * Gets a representation of `nd` as an ssa-with-fields value if there is one. + */ +private TSsaWithFields accessPath(IR::Instruction insn) { + exists(SsaVariable v | insn = v.getAUse() | result = TRoot(v)) + or + exists(SsaWithFields base, Field f | insn = accessPathAux(base, f) | result = TStep(base, f)) +} + +/** + * Gets a data-flow node that reads a field `f` from a node that is represented + * by ssa-with-fields value `base`. + */ +private IR::Instruction accessPathAux(TSsaWithFields base, Field f) { + exists(IR::FieldReadInstruction fr | fr = result | + base = accessPath(fr.getBase()) and + f = fr.getField() + ) +} + +class SsaWithFields extends TSsaWithFields { + /** + * Gets the SSA variable corresponding to the base of this SSA variable with fields. + * + * For example, the SSA variable corresponding to `a` for the SSA variable with fields + * corresponding to `a.b`. + */ + SsaVariable getBaseVariable() { + this = TRoot(result) + or + exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base.getBaseVariable()) + } + + /** Gets a use that refers to this SSA variable with fields. */ + DataFlow::Node getAUse() { this = accessPath(result.asInstruction()) } + + /** Gets a textual representation of this element. */ + string toString() { + exists(SsaVariable var | this = TRoot(var) | result = "(" + var + ")") + or + exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base + "." + f.getName()) + } + + /** + * Holds if this element is at the specified location. + * The location spans column `startcolumn` of line `startline` to + * column `endcolumn` of line `endline` in file `filepath`. + * For more information, see + * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html). + */ + predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + this.getBaseVariable().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } +} diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 9851766e440..395662eefc5 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -733,33 +733,6 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { */ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) } -/** - * An SSA variable, possibly with a chain of field reads on it. - */ -private newtype SsaWithFields = - Root(SsaVariable v) or - Step(SsaWithFields base, Field f) { exists(accessPathAux(base, f)) } - -/** - * Gets a representation of `nd` as an ssa-with-fields value if there is one. - */ -private SsaWithFields accessPath(Node nd) { - exists(SsaVariable v | nd.asInstruction() = v.getAUse() | result = Root(v)) - or - exists(SsaWithFields base, Field f | nd = accessPathAux(base, f) | result = Step(base, f)) -} - -/** - * Gets a data-flow node that reads a field `f` from a node that is represented - * by ssa-with-fields value `base`. - */ -private Node accessPathAux(SsaWithFields base, Field f) { - exists(IR::FieldReadInstruction fr | fr = result.asInstruction() | - base = accessPath(instructionNode(fr.getBase())) and - f = fr.getField() - ) -} - /** * A guard that validates some expression. * @@ -775,8 +748,10 @@ abstract class BarrierGuard extends Node { /** Gets a node guarded by this guard. */ final Node getAGuardedNode() { - exists(ControlFlow::ConditionGuardNode guard, Node nd | - guards(guard, nd, accessPath(result)) and + exists(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields var | + result = var.getAUse() + | + guards(guard, nd, var) and guard.dominates(result.asInstruction().getBasicBlock()) ) } @@ -789,8 +764,7 @@ abstract class BarrierGuard extends Node { */ pragma[noinline] private predicate guards(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap) { - guards(guard, nd) and - ap = accessPath(nd) + guards(guard, nd) and nd = ap.getAUse() } /** diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 96c0567053a..4f3d6aa87ec 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -50,6 +50,25 @@ module OpenUrlRedirect { } } + /** + * An access to a variable that is preceded by an assignment to its `Path` field. + * + * This is overapproximate; this will currently remove flow through all `Url.Path` assignments + * which contain a substring that could sanitize data. + */ + class PathAssignmentBarrier extends Barrier, Read { + PathAssignmentBarrier() { + exists(Write w, Field f, SsaWithFields var | + f.getName() = "Path" and + hasHostnameSanitizingSubstring(w.getRhs()) and + this = var.getAUse() + | + w.writesField(var.getAUse(), f, _) and + w.dominatesNode(insn) + ) + } + } + /** * A call to a function called `isLocalUrl` or similar, which is * considered a barrier for purposes of URL redirection. diff --git a/ql/src/semmle/go/security/UrlConcatenation.qll b/ql/src/semmle/go/security/UrlConcatenation.qll index 3147b077c93..898db550c62 100644 --- a/ql/src/semmle/go/security/UrlConcatenation.qll +++ b/ql/src/semmle/go/security/UrlConcatenation.qll @@ -55,7 +55,7 @@ predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) { * In the latter two cases, the additional check is necessary to avoid a `/` that could be interpreted as * the `//` separating the (optional) scheme from the hostname. */ -private predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) { +predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) { nd.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*") or hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd))