From 4c9bf2265e7048f49425cdb2025585d0c80a3a5b Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 15 Nov 2019 01:25:36 -0800 Subject: [PATCH 1/3] OpenRedirect: treat assignments to Url.Path as a barrier --- .../OpenUrlRedirectCustomizations.qll | 35 +++++++++++++++++++ .../semmle/go/security/UrlConcatenation.qll | 2 +- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 96c0567053a..8bd8eda9ac7 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -50,6 +50,41 @@ module OpenUrlRedirect { } } + /** + * Holds if `a` and `b` read the same variable, field, or method. + */ + predicate readsSameEntity(Read a, Read b) { + exists(DataFlow::Node aBase, DataFlow::Node bBase | readsSameEntity(aBase, bBase) | + exists(Field f | a.readsField(aBase, f) | b.readsField(bBase, f)) + or + exists(Method m | a.readsMethod(aBase, m) | b.readsMethod(bBase, m)) + ) + } + + /** + * 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, Read writeBase, ValueEntity v | + f.getName() = "Path" and + hasHostnameSanitizingSubstring(w.getRhs()) and + readsSameEntity(this, writeBase) + | + w.writesField(writeBase, f, _) and + w.getBasicBlock().(ReachableBasicBlock).dominates(this.asInstruction().getBasicBlock()) and + ( + not w.getBasicBlock() = this.asInstruction().getBasicBlock() + or + w.getASuccessor+() = this.asInstruction() + ) + ) + } + } + /** * 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)) From 1092fe58709f276c2893881f28dce51b30c16eb9 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 18 Nov 2019 22:45:32 -0800 Subject: [PATCH 2/3] Move SsaWithFields to the Ssa file and rework it for public use Also use it in OpenRedirect --- ql/src/semmle/go/dataflow/SSA.qll | 94 +++++++++++++++++++ .../go/dataflow/internal/DataFlowUtil.qll | 36 +------ .../OpenUrlRedirectCustomizations.qll | 16 +--- 3 files changed, 102 insertions(+), 44 deletions(-) diff --git a/ql/src/semmle/go/dataflow/SSA.qll b/ql/src/semmle/go/dataflow/SSA.qll index be04265ab90..15456ee90bf 100644 --- a/ql/src/semmle/go/dataflow/SSA.qll +++ b/ql/src/semmle/go/dataflow/SSA.qll @@ -288,3 +288,97 @@ 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() + ) +} + +abstract 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`. + */ + abstract SsaVariable getBaseVariable(); + + /** Gets the type of this SSA variable with fields. */ + abstract Type getType(); + + /** Gets a use in basic block `bb` that refers to this SSA variable with fields. */ + abstract IR::Instruction getAUseIn(ReachableBasicBlock bb); + + /** Gets a use that refers to this SSA variable with fields. */ + IR::Instruction getAUse() { result = this.getAUseIn(_) } + + /** Gets a textual representation of this element. */ + abstract string toString(); + + /** + * 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) + } +} + +private class SsaWithFieldsRoot extends SsaWithFields, TRoot { + SsaVariable self; + + SsaWithFieldsRoot() { this = TRoot(self) } + + override SsaVariable getBaseVariable() { result = self } + + override Type getType() { result = self.getType() } + + override IR::Instruction getAUseIn(ReachableBasicBlock bb) { result = self.getAUseIn(bb) } + + override string toString() { result = "(" + self.toString() + ")" } +} + +private class SsaWithFieldsStep extends SsaWithFields, TStep { + SsaWithFields base; + Field f; + + SsaWithFieldsStep() { this = TStep(base, f) } + + override SsaVariable getBaseVariable() { result = base.getBaseVariable() } + + override Type getType() { result = f.getType() } + + override IR::FieldReadInstruction getAUseIn(ReachableBasicBlock bb) { + result.getBase() = base.getAUseIn(bb) and + result.getField() = f + } + + override string toString() { result = base.toString() + "." + f.getName() } +} diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 9851766e440..cd8209f878c 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. * @@ -776,8 +749,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 - guard.dominates(result.asInstruction().getBasicBlock()) + exists(SsaWithFields var | result.asInstruction() = 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.asInstruction() = ap.getAUse() } /** diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 8bd8eda9ac7..b911b4636ed 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -50,17 +50,6 @@ module OpenUrlRedirect { } } - /** - * Holds if `a` and `b` read the same variable, field, or method. - */ - predicate readsSameEntity(Read a, Read b) { - exists(DataFlow::Node aBase, DataFlow::Node bBase | readsSameEntity(aBase, bBase) | - exists(Field f | a.readsField(aBase, f) | b.readsField(bBase, f)) - or - exists(Method m | a.readsMethod(aBase, m) | b.readsMethod(bBase, m)) - ) - } - /** * An access to a variable that is preceded by an assignment to its `Path` field. * @@ -69,10 +58,11 @@ module OpenUrlRedirect { */ class PathAssignmentBarrier extends Barrier, Read { PathAssignmentBarrier() { - exists(Write w, Field f, Read writeBase, ValueEntity v | + exists(Write w, Field f, Read writeBase, SsaWithFields var | f.getName() = "Path" and hasHostnameSanitizingSubstring(w.getRhs()) and - readsSameEntity(this, writeBase) + this.asInstruction() = var.getAUse() and + writeBase.asInstruction() = var.getAUse() | w.writesField(writeBase, f, _) and w.getBasicBlock().(ReachableBasicBlock).dominates(this.asInstruction().getBasicBlock()) and From 81ba71e47b62846aee5188db23766e2a6c74f9f8 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 19 Nov 2019 09:54:12 -0800 Subject: [PATCH 3/3] Address review comments --- .../go/controlflow/ControlFlowGraph.qll | 12 +++++ ql/src/semmle/go/dataflow/SSA.qll | 54 +++++-------------- .../go/dataflow/internal/DataFlowUtil.qll | 12 ++--- .../OpenUrlRedirectCustomizations.qll | 14 ++--- 4 files changed, 34 insertions(+), 58 deletions(-) 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 15456ee90bf..3add50eb220 100644 --- a/ql/src/semmle/go/dataflow/SSA.qll +++ b/ql/src/semmle/go/dataflow/SSA.qll @@ -316,26 +316,28 @@ private IR::Instruction accessPathAux(TSsaWithFields base, Field f) { ) } -abstract class SsaWithFields extends TSsaWithFields { +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`. */ - abstract SsaVariable getBaseVariable(); - - /** Gets the type of this SSA variable with fields. */ - abstract Type getType(); - - /** Gets a use in basic block `bb` that refers to this SSA variable with fields. */ - abstract IR::Instruction getAUseIn(ReachableBasicBlock bb); + 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. */ - IR::Instruction getAUse() { result = this.getAUseIn(_) } + DataFlow::Node getAUse() { this = accessPath(result.asInstruction()) } /** Gets a textual representation of this element. */ - abstract string toString(); + 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. @@ -350,35 +352,3 @@ abstract class SsaWithFields extends TSsaWithFields { this.getBaseVariable().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) } } - -private class SsaWithFieldsRoot extends SsaWithFields, TRoot { - SsaVariable self; - - SsaWithFieldsRoot() { this = TRoot(self) } - - override SsaVariable getBaseVariable() { result = self } - - override Type getType() { result = self.getType() } - - override IR::Instruction getAUseIn(ReachableBasicBlock bb) { result = self.getAUseIn(bb) } - - override string toString() { result = "(" + self.toString() + ")" } -} - -private class SsaWithFieldsStep extends SsaWithFields, TStep { - SsaWithFields base; - Field f; - - SsaWithFieldsStep() { this = TStep(base, f) } - - override SsaVariable getBaseVariable() { result = base.getBaseVariable() } - - override Type getType() { result = f.getType() } - - override IR::FieldReadInstruction getAUseIn(ReachableBasicBlock bb) { - result.getBase() = base.getAUseIn(bb) and - result.getField() = f - } - - override string toString() { result = base.toString() + "." + f.getName() } -} diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index cd8209f878c..395662eefc5 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -748,11 +748,11 @@ abstract class BarrierGuard extends Node { /** Gets a node guarded by this guard. */ final Node getAGuardedNode() { - exists(ControlFlow::ConditionGuardNode guard, Node nd | - exists(SsaWithFields var | result.asInstruction() = var.getAUse() | - guards(guard, nd, var) and - guard.dominates(result.asInstruction().getBasicBlock()) - ) + exists(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields var | + result = var.getAUse() + | + guards(guard, nd, var) and + guard.dominates(result.asInstruction().getBasicBlock()) ) } @@ -764,7 +764,7 @@ abstract class BarrierGuard extends Node { */ pragma[noinline] private predicate guards(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap) { - guards(guard, nd) and nd.asInstruction() = ap.getAUse() + 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 b911b4636ed..4f3d6aa87ec 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -58,19 +58,13 @@ module OpenUrlRedirect { */ class PathAssignmentBarrier extends Barrier, Read { PathAssignmentBarrier() { - exists(Write w, Field f, Read writeBase, SsaWithFields var | + exists(Write w, Field f, SsaWithFields var | f.getName() = "Path" and hasHostnameSanitizingSubstring(w.getRhs()) and - this.asInstruction() = var.getAUse() and - writeBase.asInstruction() = var.getAUse() + this = var.getAUse() | - w.writesField(writeBase, f, _) and - w.getBasicBlock().(ReachableBasicBlock).dominates(this.asInstruction().getBasicBlock()) and - ( - not w.getBasicBlock() = this.asInstruction().getBasicBlock() - or - w.getASuccessor+() = this.asInstruction() - ) + w.writesField(var.getAUse(), f, _) and + w.dominatesNode(insn) ) } }