From f279fa17afd8950b71ca9e81ce6b8a55ebf109bd Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 2 Feb 2021 10:36:17 +0000 Subject: [PATCH 01/13] (clean-up) Move comment --- ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index f99365d1706..b06d73ce694 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -1184,8 +1184,8 @@ abstract class BarrierGuard extends Node { fd.getFunction() = f and localFlow(inp.getExitNode(fd), arg) and ret = outp.getEntryNode(fd) and - // Case: a function like "if someBarrierGuard(arg) { return true } else { return false }" ( + // Case: a function like "if someBarrierGuard(arg) { return true } else { return false }" exists(ControlFlow::ConditionGuardNode guard | guards(guard, arg) and guard.dominates(ret.getBasicBlock()) From dd079d4e5119df0a85c64d9644246cbca1d65bb8 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 2 Feb 2021 11:04:16 +0000 Subject: [PATCH 02/13] (clean-up) Make use of `this` explicit --- ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index b06d73ce694..f9dc9195759 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -1187,7 +1187,7 @@ abstract class BarrierGuard extends Node { ( // Case: a function like "if someBarrierGuard(arg) { return true } else { return false }" exists(ControlFlow::ConditionGuardNode guard | - guards(guard, arg) and + this.guards(guard, arg) and guard.dominates(ret.getBasicBlock()) | exists(boolean b | @@ -1221,7 +1221,7 @@ abstract class BarrierGuard extends Node { DataFlow::Property outpProp | not exists(DataFlow::Node otherRet | otherRet = outp.getEntryNode(fd) | otherRet != ret) and - guardingFunction(f2, inp2, outp2, outpProp) and + this.guardingFunction(f2, inp2, outp2, outpProp) and c = f2.getACall() and arg = inp2.getNode(c) and ( From c4eaf791e6b95f0605e042f7442cc71ea444177f Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 2 Feb 2021 11:07:44 +0000 Subject: [PATCH 03/13] Add predicate for cast test passing edge in switch statement --- .../go/controlflow/ControlFlowGraph.qll | 11 +++++++++ .../go/controlflow/ControlFlowGraphImpl.qll | 23 +++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll b/ql/src/semmle/go/controlflow/ControlFlowGraph.qll index 63225ca78b0..71bd7f1c74f 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll +++ b/ql/src/semmle/go/controlflow/ControlFlowGraph.qll @@ -274,6 +274,17 @@ module ControlFlow { * cannot return normally, but never fails to hold of a function that can return normally. */ predicate mayReturnNormally(FuncDecl f) { CFG::mayReturnNormally(f.getBody()) } + + /** + * Holds if `pred` is the node for the case `testExpr` in an expression + * switch statement which is switching on `switchExpr`, and `succ` is the + * node to be executed next if the case test succeeds. + */ + predicate isSwitchCaseTestPassingEdge( + ControlFlow::Node pred, ControlFlow::Node succ, Expr switchExpr, Expr testExpr + ) { + CFG::isSwitchCaseTestPassingEdge(pred, succ, switchExpr, testExpr) + } } class Write = ControlFlow::WriteNode; diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll index 17ed1d7204b..31b27be2ac6 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll +++ b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll @@ -1046,11 +1046,16 @@ module CFG { pred = getExprEnd(i, false) and succ = getExprStart(i + 1) or - pred = getExprEnd(i, true) and - succ = getBodyStart() + isPassingEdge(i, pred, succ, _) ) } + predicate isPassingEdge(int i, ControlFlow::Node pred, ControlFlow::Node succ, Expr testExpr) { + pred = getExprEnd(i, true) and + succ = getBodyStart() and + testExpr = getExpr(i) + } + override ControlFlowTree getChildTree(int i) { result = getStmt(i) } } @@ -1959,6 +1964,20 @@ module CFG { exists(ControlFlow::Node last, Completion cmpl | lastNode(root, last, cmpl) and cmpl != Panic()) } + /** + * Holds if `pred` is the node for the case `testExpr` in an expression + * switch statement which is switching on `switchExpr`, and `succ` is the + * node to be executed next if the case test succeeds. + */ + cached + predicate isSwitchCaseTestPassingEdge( + ControlFlow::Node pred, ControlFlow::Node succ, Expr switchExpr, Expr testExpr + ) { + exists(ExpressionSwitchStmt ess | ess.getExpr() = switchExpr | + ess.getACase().(CaseClauseTree).isPassingEdge(_, pred, succ, testExpr) + ) + } + /** Gets a successor of `nd`, that is, a node that is executed after `nd`. */ cached ControlFlow::Node succ(ControlFlow::Node nd) { any(ControlFlowTree tree).succ(nd, result) } From 4c30ed90547be1c3451166dd67c9d674f3f98383 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 2 Feb 2021 11:18:31 +0000 Subject: [PATCH 04/13] Add predicate to get return statement from return instruction --- ql/src/semmle/go/controlflow/IR.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index 1b1e7df9425..319d2193d00 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -905,6 +905,8 @@ module IR { ReturnInstruction() { this = MkReturnNode(ret) } + ReturnStmt getReturnStmt() { result = ret } + /** Holds if this statement returns multiple results. */ predicate returnsMultipleResults() { exists(MkExtractNode(ret, _)) or ret.getNumExpr() > 1 } From 08c59f0f48684916ddd20def9c53f58572a8ad53 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 2 Feb 2021 12:11:42 +0000 Subject: [PATCH 05/13] Add a default sanitizer guard for list of constants comparison Currently it only deals with the case of a switch statement in a function. --- .../go/dataflow/internal/DataFlowUtil.qll | 123 +++++++++++++ .../test.expected | 0 .../ListOfConstantsSanitizerGuards/test.go | 170 ++++++++++++++++++ .../ListOfConstantsSanitizerGuards/test.ql | 29 +++ 4 files changed, 322 insertions(+) create mode 100644 ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.expected create mode 100644 ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go create mode 100644 ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.ql diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index f9dc9195759..3bb31ed5c85 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -1320,3 +1320,126 @@ private predicate onlyPossibleReturnOfNil(FuncDecl fd, FunctionOutput res, DataF isCertainlyNotNil(otherRet) ) } + +/** + * Gets a predecessor of `succ` without following edges corresponding to + * passing constant case tests in the switch block which is switching on + * `protectedExpr`. + */ +private ControlFlow::Node getANonTestPassingPredecessor(ControlFlow::Node succ, Expr protectedExpr) { + result = succ.getAPredecessor() and + not exists(Expr testExpr | + ControlFlow::isSwitchCaseTestPassingEdge(result, succ, protectedExpr, testExpr) and + testExpr.isConst() + ) +} + +private ControlFlow::Node getANonTestPassingReachingNodeRecursive( + ControlFlow::Node n, Expr protectedExpr +) { + result = n or + result = + getANonTestPassingReachingNodeRecursive(getANonTestPassingPredecessor(n, protectedExpr), + protectedExpr) +} + +/** + * Gets a node by following predecessors from `ret` without following edges + * corresponding to passing constant test cases in switch blocks. + */ +private ControlFlow::Node getANonTestPassingReachingNodeBase( + IR::ReturnInstruction ret, Expr protectedExpr +) { + protectedExpr.getEnclosingFunction() = ret.getReturnStmt().getEnclosingFunction() and + result = getANonTestPassingReachingNodeRecursive(ret, protectedExpr) +} + +/** + * Holds if every way to get from the entry node of the function to `ret` + * involves passing a constant test case in the switch statement switching on + * `protectedExpr`. + */ +private predicate mustPassConstantCaseTestToReach(IR::ReturnInstruction ret, Expr protectedExpr) { + not exists(ControlFlow::Node entry | entry = ret.getRoot().getEntryNode() | + entry = getANonTestPassingReachingNodeBase(ret, protectedExpr) + ) +} + +/** + * Holds if whenever `outp` of function `f` satisfies `p`, the input `inp` of + * `f` was compared to a constant in a case clause of a switch statement. + * + * We check this by looking for guards on `inp` that dominate a `return` statement that + * is the only `return` in `f` that can return `true`. This means that if `f` returns `true`, + * the guard must have been satisfied. (Similar reasoning is applied for statements returning + * `false`, `nil` or a non-`nil` value.) + */ +predicate isListOfConstantsComparisonUsingFunctionSwitch( + Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p +) { + outp.isResult(_) and + exists(FuncDecl fd, ExpressionSwitchStmt ess, Node exprNode | + fd.getFunction() = f and + exprNode.asExpr() = ess.getExpr() and + localFlow(inp.getExitNode(fd), exprNode) + | + exists(boolean b | + p.isBoolean(b) and + forex(DataFlow::Node ret, IR::ReturnInstruction ri | + ret = outp.getEntryNode(f.getFuncDecl()) and + ri.getReturnStmt().getAnExpr() = ret.asExpr() and + possiblyReturnsBool(fd, outp, ret, b) + | + mustPassConstantCaseTestToReach(ri, ess.getExpr()) + ) + ) + or + p.isNonNil() and + forex(DataFlow::Node ret, IR::ReturnInstruction ri | + ret = outp.getEntryNode(f.getFuncDecl()) and + ri.getReturnStmt().getAnExpr() = ret.asExpr() and + possiblyReturnsNonNil(fd, outp, ret) + | + mustPassConstantCaseTestToReach(ri, ess.getExpr()) + ) + or + p.isNil() and + forex(DataFlow::Node ret, IR::ReturnInstruction ri | + ret = outp.getEntryNode(f.getFuncDecl()) and + ri.getReturnStmt().getAnExpr() = ret.asExpr() and + ret.asExpr() = Builtin::nil().getAReference() + | + mustPassConstantCaseTestToReach(ri, ess.getExpr()) + ) + ) +} + +/** + * A comparison against a list of constants, acting as a sanitizer guard for + * `guardedExpr` by restricting it to a known value. + * + * Currently this only looks for functions containing a switch statement, but + * it could equally look for a check for membership of a constant map or + * constant array, which does not need to be in its own function. + */ +class ListOfConstantsComparisonSanitizerGuard extends TaintTracking::DefaultTaintSanitizerGuard { + DataFlow::Node guardedExpr; + boolean outcome; + + ListOfConstantsComparisonSanitizerGuard() { + exists( + Function f, FunctionInput inp, FunctionOutput outp, DataFlow::CallNode call, + DataFlow::Property p, DataFlow::Node res + | + isListOfConstantsComparisonUsingFunctionSwitch(f, inp, outp, p) and + call = f.getACall() and + guardedExpr = inp.getNode(call) and + p.checkOn(this, outcome, res) and + DataFlow::localFlow(outp.getNode(call), res) + ) + } + + override predicate checks(Expr e, boolean branch) { + e = guardedExpr.asExpr() and branch = outcome + } +} diff --git a/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.expected b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go new file mode 100644 index 00000000000..67592a94adc --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go @@ -0,0 +1,170 @@ +package main + +import ( + "crypto/rand" + "fmt" +) + +const constantGlobalVariable string = "constant global variable" + +// Utilities: a source, a sink and an error struct: + +func source() string { + return "tainted" +} + +func sink(s string) {} + +func getRandomString() string { + buff := make([]byte, 10) + rand.Read(buff) + return fmt.Sprintf("%x", buff) +} + +func getConstantString() string { + return "constant return value" +} + +type errorString struct { + s string +} + +func (e *errorString) Error() string { + return e.s +} + +// Candidate functions which compare the input against a list of constants: + +func switchStatementReturningTrueOnlyWhenConstant(s string) bool { + switch s { + case constantGlobalVariable, "string literal": + return true + case getRandomString(): + return false + case "another string literal": + return false + default: + return false + } +} + +func switchStatementReturningFalseOnlyWhenConstant(r string, s string) bool { + switch s { + case "string literal": + return false + case constantGlobalVariable: + return false + case "another string literal": + return true + } + return true +} + +func switchStatementReturningNonNilOnlyWhenConstant(s string) (string, error) { + switch s { + case constantGlobalVariable, "string literal": + return "error", &errorString{"a"} + case getRandomString(): + return "no error", nil + case "another string literal": + return "no error", nil + default: + return "no error", nil + } +} + +func switchStatementReturningNilOnlyWhenConstant(s string) *string { + t := s + switch t { + case "string literal": + return nil + case constantGlobalVariable, "another string literal": + str := "matches random string" + return &str + } + str := "no matches" + return &str +} + +func switchStatementWithoutUsefulInfo(s string) bool { + switch s { + case constantGlobalVariable, "string literal": + return false + case getRandomString(): + return true + default: + return false + } +} + +func switchStatementOverRandomString(s string) bool { + switch getRandomString() { + case "string literal": + return true + default: + return false + } +} + +// Tests + +func main() { + + // Switch statements in functions + + { + s := source() + if switchStatementReturningTrueOnlyWhenConstant(s) { + sink(s) + } else { + sink(s) // $dataflow=s + } + } + + { + s := source() + if switchStatementReturningFalseOnlyWhenConstant("", s) { + sink(s) // $dataflow=s + } else { + sink(s) + } + } + + { + s := source() + _, err := switchStatementReturningNonNilOnlyWhenConstant(s) + if err != nil { + sink(s) + } else { + sink(s) // $dataflow=s + } + } + + { + s := source() + if switchStatementReturningNilOnlyWhenConstant(s) == nil { + sink(s) + } else { + sink(s) // $dataflow=s + } + } + + { + s := source() + if switchStatementWithoutUsefulInfo(s) { + sink(s) // $dataflow=s + } else { + sink(s) // $dataflow=s + } + } + + { + s := source() + if switchStatementOverRandomString(s) { + sink(s) // $dataflow=s + } else { + sink(s) // $dataflow=s + } + } + +} diff --git a/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.ql b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.ql new file mode 100644 index 00000000000..2b6bddff7f9 --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.ql @@ -0,0 +1,29 @@ +import go +import TestUtilities.InlineExpectationsTest + +class TestConfig extends TaintTracking::Configuration { + TestConfig() { this = "test config" } + + override predicate isSource(DataFlow::Node source) { + source.(DataFlow::CallNode).getTarget().getName() = "source" + } + + override predicate isSink(DataFlow::Node sink) { + sink = any(DataFlow::CallNode c | c.getTarget().getName() = "sink").getAnArgument() + } +} + +class DataFlowTest extends InlineExpectationsTest { + DataFlowTest() { this = "DataFlowTest" } + + override string getARelevantTag() { result = "dataflow" } + + override predicate hasActualResult(string file, int line, string element, string tag, string value) { + tag = "dataflow" and + exists(DataFlow::Node sink | any(TestConfig c).hasFlow(_, sink) | + element = sink.toString() and + value = sink.toString() and + sink.hasLocationInfo(file, line, _, _, _) + ) + } +} From 5ec25de1fcf805e46afd855103b297bc7543d5aa Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 2 Feb 2021 16:27:44 +0000 Subject: [PATCH 06/13] Add change note --- change-notes/2021-02-02-constant-comparison-sanitizer-guard.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2021-02-02-constant-comparison-sanitizer-guard.md diff --git a/change-notes/2021-02-02-constant-comparison-sanitizer-guard.md b/change-notes/2021-02-02-constant-comparison-sanitizer-guard.md new file mode 100644 index 00000000000..0442a7f9ccd --- /dev/null +++ b/change-notes/2021-02-02-constant-comparison-sanitizer-guard.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* A function which compares a value with a list of constants now acts as a sanitizer guard. This should lead to fewer false positive results. From 760d89b0d3ade1150175188789e69765b96d1737 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Wed, 3 Feb 2021 14:31:34 +0000 Subject: [PATCH 07/13] Apply suggestions from code review Co-authored-by: Chris Smowton --- ql/src/semmle/go/controlflow/IR.qll | 1 + .../semmle/go/dataflow/internal/DataFlowUtil.qll | 15 ++++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index 319d2193d00..95270055d18 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -905,6 +905,7 @@ module IR { ReturnInstruction() { this = MkReturnNode(ret) } + /** Gets the corresponding `ReturnStmt`. */ ReturnStmt getReturnStmt() { result = ret } /** Holds if this statement returns multiple results. */ diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 3bb31ed5c85..bdb4ab046b5 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -1367,14 +1367,15 @@ private predicate mustPassConstantCaseTestToReach(IR::ReturnInstruction ret, Exp /** * Holds if whenever `outp` of function `f` satisfies `p`, the input `inp` of - * `f` was compared to a constant in a case clause of a switch statement. + * `f` matched a constant in a case clause of a switch statement. * - * We check this by looking for guards on `inp` that dominate a `return` statement that - * is the only `return` in `f` that can return `true`. This means that if `f` returns `true`, - * the guard must have been satisfied. (Similar reasoning is applied for statements returning - * `false`, `nil` or a non-`nil` value.) + * We check this by looking for guards on `inp` that collectively dominate a + * `return` statement that is the only `return` in `f` that can return `true`. + * This means that if `f` returns `true`, one of the guards must have been + * satisfied. (Similar reasoning is applied for statements returning `false`, + * `nil` or a non-`nil` value.) */ -predicate isListOfConstantsComparisonUsingFunctionSwitch( +predicate functionEnsuresInputIsConstant( Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p ) { outp.isResult(_) and @@ -1431,7 +1432,7 @@ class ListOfConstantsComparisonSanitizerGuard extends TaintTracking::DefaultTain Function f, FunctionInput inp, FunctionOutput outp, DataFlow::CallNode call, DataFlow::Property p, DataFlow::Node res | - isListOfConstantsComparisonUsingFunctionSwitch(f, inp, outp, p) and + functionEnsuresInputIsConstant(f, inp, outp, p) and call = f.getACall() and guardedExpr = inp.getNode(call) and p.checkOn(this, outcome, res) and From a7545cd11b74c754b8988580ecd213e9346ece4b Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 3 Feb 2021 14:38:53 +0000 Subject: [PATCH 08/13] Add test with multiple switch statements --- .../ListOfConstantsSanitizerGuards/test.go | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go index 67592a94adc..0f77f4e46df 100644 --- a/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go +++ b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go @@ -86,6 +86,25 @@ func switchStatementReturningNilOnlyWhenConstant(s string) *string { return &str } +func multipleSwitchStatementReturningTrueOnlyWhenConstant(s string, t string) bool { + switch s { + case constantGlobalVariable, "string literal": + return true + case getRandomString(): + return false + } + switch s { + case "another string literal": + return true + } + switch t { + case "another string literal": + return false + default: + return false + } +} + func switchStatementWithoutUsefulInfo(s string) bool { switch s { case constantGlobalVariable, "string literal": @@ -149,6 +168,15 @@ func main() { } } + { + s := source() + if multipleSwitchStatementReturningTrueOnlyWhenConstant(s, getRandomString()) { + sink(s) + } else { + sink(s) // $dataflow=s + } + } + { s := source() if switchStatementWithoutUsefulInfo(s) { From 36fafadda54fb6ee3c61b7bc144383e8f4569788 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 3 Feb 2021 15:26:07 +0000 Subject: [PATCH 09/13] Add `fallthrough` statements to switch statement tests --- .../go/dataflow/ListOfConstantsSanitizerGuards/test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go index 0f77f4e46df..ceab5488627 100644 --- a/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go +++ b/ql/test/library-tests/semmle/go/dataflow/ListOfConstantsSanitizerGuards/test.go @@ -67,7 +67,7 @@ func switchStatementReturningNonNilOnlyWhenConstant(s string) (string, error) { case getRandomString(): return "no error", nil case "another string literal": - return "no error", nil + fallthrough default: return "no error", nil } @@ -77,8 +77,10 @@ func switchStatementReturningNilOnlyWhenConstant(s string) *string { t := s switch t { case "string literal": + fallthrough + case constantGlobalVariable: return nil - case constantGlobalVariable, "another string literal": + case "another string literal": str := "matches random string" return &str } From d75cc404830b9fc43d534cb06c97fa23acc36fc8 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 4 Feb 2021 12:03:59 +0000 Subject: [PATCH 10/13] Make test with multiple switch statements pass Made various changes to make it work when there are multiple switch statements. Also addressed performance problems. --- .../go/dataflow/internal/DataFlowUtil.qll | 93 ++++++++++++------- 1 file changed, 57 insertions(+), 36 deletions(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index bdb4ab046b5..544abc75479 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -1322,46 +1322,69 @@ private predicate onlyPossibleReturnOfNil(FuncDecl fd, FunctionOutput res, DataF } /** - * Gets a predecessor of `succ` without following edges corresponding to - * passing constant case tests in the switch block which is switching on - * `protectedExpr`. + * Holds if data flows from `node` to `switchExprNode`, which is the expression + * of a switch statement. */ -private ControlFlow::Node getANonTestPassingPredecessor(ControlFlow::Node succ, Expr protectedExpr) { +private predicate flowsToSwitchExpression(Node node, Node switchExprNode) { + switchExprNode.asExpr() = any(ExpressionSwitchStmt ess).getExpr() and + localFlow(node, switchExprNode) +} + +/** + * Holds if `inputNode` is the exit node of a parameter to `fd` and data flows + * from `inputNode` to the expression of a switch statement. + */ +private predicate isPossibleInputNode(Node inputNode, FuncDef fd) { + inputNode = any(FunctionInput inp | inp.isParameter(_)).getExitNode(fd) and + flowsToSwitchExpression(inputNode, _) +} + +/** + * Gets a predecessor of `succ` without following edges corresponding to + * passing a constant case test in a switch statement which is switching on + * an expression which data flows to from `inputNode`. + */ +private ControlFlow::Node getANonTestPassingPredecessor(ControlFlow::Node succ, Node inputNode) { + isPossibleInputNode(inputNode, succ.getRoot().(FuncDef)) and result = succ.getAPredecessor() and - not exists(Expr testExpr | - ControlFlow::isSwitchCaseTestPassingEdge(result, succ, protectedExpr, testExpr) and + not exists(Expr testExpr, Node switchExprNode | + flowsToSwitchExpression(inputNode, switchExprNode) and + ControlFlow::isSwitchCaseTestPassingEdge(result, succ, switchExprNode.asExpr(), testExpr) and testExpr.isConst() ) } private ControlFlow::Node getANonTestPassingReachingNodeRecursive( - ControlFlow::Node n, Expr protectedExpr + ControlFlow::Node n, Node inputNode ) { - result = n or - result = - getANonTestPassingReachingNodeRecursive(getANonTestPassingPredecessor(n, protectedExpr), - protectedExpr) + isPossibleInputNode(inputNode, n.getRoot().(FuncDef)) and + ( + result = n or + result = + getANonTestPassingReachingNodeRecursive(getANonTestPassingPredecessor(n, inputNode), inputNode) + ) } /** * Gets a node by following predecessors from `ret` without following edges - * corresponding to passing constant test cases in switch blocks. + * corresponding to passing a constant case test in a switch statement which is + * switching on an expression which data flows to from `inputNode`. */ private ControlFlow::Node getANonTestPassingReachingNodeBase( - IR::ReturnInstruction ret, Expr protectedExpr + IR::ReturnInstruction ret, Node inputNode ) { - protectedExpr.getEnclosingFunction() = ret.getReturnStmt().getEnclosingFunction() and - result = getANonTestPassingReachingNodeRecursive(ret, protectedExpr) + result = getANonTestPassingReachingNodeRecursive(ret, inputNode) } /** * Holds if every way to get from the entry node of the function to `ret` - * involves passing a constant test case in the switch statement switching on - * `protectedExpr`. + * involves passing a constant test case in a switch statement which is + * switching on an expression which data flows to from `inputNode`. */ -private predicate mustPassConstantCaseTestToReach(IR::ReturnInstruction ret, Expr protectedExpr) { +private predicate mustPassConstantCaseTestToReach(IR::ReturnInstruction ret, Node inputNode) { + isPossibleInputNode(inputNode, ret.getRoot().(FuncDef)) and not exists(ControlFlow::Node entry | entry = ret.getRoot().getEntryNode() | - entry = getANonTestPassingReachingNodeBase(ret, protectedExpr) + entry = getANonTestPassingReachingNodeBase(ret, inputNode) ) } @@ -1369,48 +1392,46 @@ private predicate mustPassConstantCaseTestToReach(IR::ReturnInstruction ret, Exp * Holds if whenever `outp` of function `f` satisfies `p`, the input `inp` of * `f` matched a constant in a case clause of a switch statement. * - * We check this by looking for guards on `inp` that collectively dominate a - * `return` statement that is the only `return` in `f` that can return `true`. - * This means that if `f` returns `true`, one of the guards must have been - * satisfied. (Similar reasoning is applied for statements returning `false`, - * `nil` or a non-`nil` value.) + * We check this by looking for guards on `inp` that collectively dominate all + * the `return` statements in `f` that can return `true`. This means that if + * `f` returns `true`, one of the guards must have been satisfied. (Similar + * reasoning is applied for statements returning `false`, `nil` or a non-`nil` + * value.) */ predicate functionEnsuresInputIsConstant( Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p ) { - outp.isResult(_) and - exists(FuncDecl fd, ExpressionSwitchStmt ess, Node exprNode | - fd.getFunction() = f and - exprNode.asExpr() = ess.getExpr() and - localFlow(inp.getExitNode(fd), exprNode) - | + exists(FuncDecl fd | fd.getFunction() = f | exists(boolean b | p.isBoolean(b) and forex(DataFlow::Node ret, IR::ReturnInstruction ri | - ret = outp.getEntryNode(f.getFuncDecl()) and + ret = outp.getEntryNode(fd) and ri.getReturnStmt().getAnExpr() = ret.asExpr() and possiblyReturnsBool(fd, outp, ret, b) | - mustPassConstantCaseTestToReach(ri, ess.getExpr()) + mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) ) ) or p.isNonNil() and forex(DataFlow::Node ret, IR::ReturnInstruction ri | - ret = outp.getEntryNode(f.getFuncDecl()) and + ret = outp.getEntryNode(fd) and ri.getReturnStmt().getAnExpr() = ret.asExpr() and possiblyReturnsNonNil(fd, outp, ret) | - mustPassConstantCaseTestToReach(ri, ess.getExpr()) + mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) ) or p.isNil() and forex(DataFlow::Node ret, IR::ReturnInstruction ri | - ret = outp.getEntryNode(f.getFuncDecl()) and + ret = outp.getEntryNode(fd) and ri.getReturnStmt().getAnExpr() = ret.asExpr() and ret.asExpr() = Builtin::nil().getAReference() | - mustPassConstantCaseTestToReach(ri, ess.getExpr()) + exists(Node exprNode | + localFlow(inp.getExitNode(fd), exprNode) and + mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) + ) ) ) } From ef658b292a8396c89d066aa4b99a731a64c6766b Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 9 Feb 2021 19:42:23 +0000 Subject: [PATCH 11/13] Fix join order for ListOfConstantsComparisonSanitizerGuard --- .../go/dataflow/internal/DataFlowUtil.qll | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 544abc75479..28344cf0458 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -1436,6 +1436,22 @@ predicate functionEnsuresInputIsConstant( ) } +/** + * Holds if whenever `outputNode` satisfies `p`, `inputNode` matched a constant + * in a case clause of a switch statement. + */ +pragma[noinline] +predicate inputIsConstantIfOutputHasProperty( + DataFlow::Node inputNode, DataFlow::Node outputNode, DataFlow::Property p +) { + exists(Function f, FunctionInput inp, FunctionOutput outp, DataFlow::CallNode call | + functionEnsuresInputIsConstant(f, inp, outp, p) and + call = f.getACall() and + inputNode = inp.getNode(call) and + DataFlow::localFlow(outp.getNode(call), outputNode) + ) +} + /** * A comparison against a list of constants, acting as a sanitizer guard for * `guardedExpr` by restricting it to a known value. @@ -1449,15 +1465,9 @@ class ListOfConstantsComparisonSanitizerGuard extends TaintTracking::DefaultTain boolean outcome; ListOfConstantsComparisonSanitizerGuard() { - exists( - Function f, FunctionInput inp, FunctionOutput outp, DataFlow::CallNode call, - DataFlow::Property p, DataFlow::Node res - | - functionEnsuresInputIsConstant(f, inp, outp, p) and - call = f.getACall() and - guardedExpr = inp.getNode(call) and - p.checkOn(this, outcome, res) and - DataFlow::localFlow(outp.getNode(call), res) + exists(DataFlow::Node outputNode, DataFlow::Property p | + inputIsConstantIfOutputHasProperty(guardedExpr, outputNode, p) and + p.checkOn(this, outcome, outputNode) ) } From b84aef6b833e3d73a9763e9368bf2d27d161669e Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 11 Feb 2021 10:29:30 +0000 Subject: [PATCH 12/13] Prevent getACalleeSource() from sharing magic with other users of getASuccessor* This avoids recursion through the magic side-condition as each discovery of a ListOfConstantsComparisonSanitizerGuard expands the set of things whose getASuccessor* is wanted, which in turn enlarges the set of transitive successors and causes getACalleeSource() to be pointlessly recomputed (pointlessly because all exprNode(getCalleeExpr())s were already computed) --- .../semmle/go/dataflow/internal/DataFlowUtil.qll | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 28344cf0458..3abd39c974e 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -331,6 +331,20 @@ class FuncLitNode extends FunctionNode::Range, ExprNode { override ResultNode getAResult() { result.getRoot() = getExpr() } } +/** + * Gets a possible target of call `cn`.class + * + * This is written explicitly like this instead of using `getCalleeNode().getAPredecessor*()` + * or `result.getASuccessor*() = cn.getCalleeNode()` because the explicit form inhibits the + * optimizer from combining this with other uses of `getASuccessor*()`, which can lead to + * recursion through a magic side-condition if those other users call `getACallee()` and thus + * pointless recomputation of `getACallee()` each recursive iteration. + */ +private DataFlow::Node getACalleeSource(DataFlow::CallNode cn) { + result = cn.getCalleeNode() or + result.getASuccessor() = getACalleeSource(cn) +} + /** A data flow node that represents a call. */ class CallNode extends ExprNode { override CallExpr expr; @@ -338,7 +352,7 @@ class CallNode extends ExprNode { /** Gets the declared target of this call */ Function getTarget() { result = expr.getTarget() } - private DataFlow::Node getACalleeSource() { result.getASuccessor*() = getCalleeNode() } + private DataFlow::Node getACalleeSource() { result = getACalleeSource(this) } /** * Gets the definition of a possible target of this call. From 68c54d43e6464ad512430cbc5b23fc3aaf8cad6d Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 15 Feb 2021 09:26:59 +0000 Subject: [PATCH 13/13] Move code to TaintTrackingUtil.qll --- .../go/dataflow/internal/DataFlowUtil.qll | 159 +----------------- .../dataflow/internal/TaintTrackingUtil.qll | 159 ++++++++++++++++++ 2 files changed, 161 insertions(+), 157 deletions(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 3abd39c974e..9efbbd4e1e3 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -1256,7 +1256,7 @@ abstract class BarrierGuard extends Node { * Holds if `ret` is a data-flow node whose value contributes to the output `res` of `fd`, * and that node may have Boolean value `b`. */ -private predicate possiblyReturnsBool(FuncDecl fd, FunctionOutput res, Node ret, Boolean b) { +predicate possiblyReturnsBool(FuncDecl fd, FunctionOutput res, Node ret, Boolean b) { ret = res.getEntryNode(fd) and ret.getType().getUnderlyingType() instanceof BoolType and not ret.getBoolValue() != b @@ -1278,7 +1278,7 @@ private predicate onlyPossibleReturnOfBool(FuncDecl fd, FunctionOutput res, Node * Holds if `ret` is a data-flow node whose value contributes to the output `res` of `fd`, * and that node may evaluate to a value other than `nil`. */ -private predicate possiblyReturnsNonNil(FuncDecl fd, FunctionOutput res, Node ret) { +predicate possiblyReturnsNonNil(FuncDecl fd, FunctionOutput res, Node ret) { ret = res.getEntryNode(fd) and not ret.asExpr() = Builtin::nil().getAReference() } @@ -1334,158 +1334,3 @@ private predicate onlyPossibleReturnOfNil(FuncDecl fd, FunctionOutput res, DataF isCertainlyNotNil(otherRet) ) } - -/** - * Holds if data flows from `node` to `switchExprNode`, which is the expression - * of a switch statement. - */ -private predicate flowsToSwitchExpression(Node node, Node switchExprNode) { - switchExprNode.asExpr() = any(ExpressionSwitchStmt ess).getExpr() and - localFlow(node, switchExprNode) -} - -/** - * Holds if `inputNode` is the exit node of a parameter to `fd` and data flows - * from `inputNode` to the expression of a switch statement. - */ -private predicate isPossibleInputNode(Node inputNode, FuncDef fd) { - inputNode = any(FunctionInput inp | inp.isParameter(_)).getExitNode(fd) and - flowsToSwitchExpression(inputNode, _) -} - -/** - * Gets a predecessor of `succ` without following edges corresponding to - * passing a constant case test in a switch statement which is switching on - * an expression which data flows to from `inputNode`. - */ -private ControlFlow::Node getANonTestPassingPredecessor(ControlFlow::Node succ, Node inputNode) { - isPossibleInputNode(inputNode, succ.getRoot().(FuncDef)) and - result = succ.getAPredecessor() and - not exists(Expr testExpr, Node switchExprNode | - flowsToSwitchExpression(inputNode, switchExprNode) and - ControlFlow::isSwitchCaseTestPassingEdge(result, succ, switchExprNode.asExpr(), testExpr) and - testExpr.isConst() - ) -} - -private ControlFlow::Node getANonTestPassingReachingNodeRecursive( - ControlFlow::Node n, Node inputNode -) { - isPossibleInputNode(inputNode, n.getRoot().(FuncDef)) and - ( - result = n or - result = - getANonTestPassingReachingNodeRecursive(getANonTestPassingPredecessor(n, inputNode), inputNode) - ) -} - -/** - * Gets a node by following predecessors from `ret` without following edges - * corresponding to passing a constant case test in a switch statement which is - * switching on an expression which data flows to from `inputNode`. - */ -private ControlFlow::Node getANonTestPassingReachingNodeBase( - IR::ReturnInstruction ret, Node inputNode -) { - result = getANonTestPassingReachingNodeRecursive(ret, inputNode) -} - -/** - * Holds if every way to get from the entry node of the function to `ret` - * involves passing a constant test case in a switch statement which is - * switching on an expression which data flows to from `inputNode`. - */ -private predicate mustPassConstantCaseTestToReach(IR::ReturnInstruction ret, Node inputNode) { - isPossibleInputNode(inputNode, ret.getRoot().(FuncDef)) and - not exists(ControlFlow::Node entry | entry = ret.getRoot().getEntryNode() | - entry = getANonTestPassingReachingNodeBase(ret, inputNode) - ) -} - -/** - * Holds if whenever `outp` of function `f` satisfies `p`, the input `inp` of - * `f` matched a constant in a case clause of a switch statement. - * - * We check this by looking for guards on `inp` that collectively dominate all - * the `return` statements in `f` that can return `true`. This means that if - * `f` returns `true`, one of the guards must have been satisfied. (Similar - * reasoning is applied for statements returning `false`, `nil` or a non-`nil` - * value.) - */ -predicate functionEnsuresInputIsConstant( - Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p -) { - exists(FuncDecl fd | fd.getFunction() = f | - exists(boolean b | - p.isBoolean(b) and - forex(DataFlow::Node ret, IR::ReturnInstruction ri | - ret = outp.getEntryNode(fd) and - ri.getReturnStmt().getAnExpr() = ret.asExpr() and - possiblyReturnsBool(fd, outp, ret, b) - | - mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) - ) - ) - or - p.isNonNil() and - forex(DataFlow::Node ret, IR::ReturnInstruction ri | - ret = outp.getEntryNode(fd) and - ri.getReturnStmt().getAnExpr() = ret.asExpr() and - possiblyReturnsNonNil(fd, outp, ret) - | - mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) - ) - or - p.isNil() and - forex(DataFlow::Node ret, IR::ReturnInstruction ri | - ret = outp.getEntryNode(fd) and - ri.getReturnStmt().getAnExpr() = ret.asExpr() and - ret.asExpr() = Builtin::nil().getAReference() - | - exists(Node exprNode | - localFlow(inp.getExitNode(fd), exprNode) and - mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) - ) - ) - ) -} - -/** - * Holds if whenever `outputNode` satisfies `p`, `inputNode` matched a constant - * in a case clause of a switch statement. - */ -pragma[noinline] -predicate inputIsConstantIfOutputHasProperty( - DataFlow::Node inputNode, DataFlow::Node outputNode, DataFlow::Property p -) { - exists(Function f, FunctionInput inp, FunctionOutput outp, DataFlow::CallNode call | - functionEnsuresInputIsConstant(f, inp, outp, p) and - call = f.getACall() and - inputNode = inp.getNode(call) and - DataFlow::localFlow(outp.getNode(call), outputNode) - ) -} - -/** - * A comparison against a list of constants, acting as a sanitizer guard for - * `guardedExpr` by restricting it to a known value. - * - * Currently this only looks for functions containing a switch statement, but - * it could equally look for a check for membership of a constant map or - * constant array, which does not need to be in its own function. - */ -class ListOfConstantsComparisonSanitizerGuard extends TaintTracking::DefaultTaintSanitizerGuard { - DataFlow::Node guardedExpr; - boolean outcome; - - ListOfConstantsComparisonSanitizerGuard() { - exists(DataFlow::Node outputNode, DataFlow::Property p | - inputIsConstantIfOutputHasProperty(guardedExpr, outputNode, p) and - p.checkOn(this, outcome, outputNode) - ) - } - - override predicate checks(Expr e, boolean branch) { - e = guardedExpr.asExpr() and branch = outcome - } -} diff --git a/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll b/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll index 4627bbf3159..d060ee25bea 100644 --- a/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll @@ -223,3 +223,162 @@ class EqualityTestGuard extends DefaultTaintSanitizerGuard, DataFlow::EqualityTe outcome = this.getPolarity() } } + +/** + * Holds if data flows from `node` to `switchExprNode`, which is the expression + * of a switch statement. + */ +private predicate flowsToSwitchExpression(DataFlow::Node node, DataFlow::Node switchExprNode) { + switchExprNode.asExpr() = any(ExpressionSwitchStmt ess).getExpr() and + DataFlow::localFlow(node, switchExprNode) +} + +/** + * Holds if `inputNode` is the exit node of a parameter to `fd` and data flows + * from `inputNode` to the expression of a switch statement. + */ +private predicate isPossibleInputNode(DataFlow::Node inputNode, FuncDef fd) { + inputNode = any(FunctionInput inp | inp.isParameter(_)).getExitNode(fd) and + flowsToSwitchExpression(inputNode, _) +} + +/** + * Gets a predecessor of `succ` without following edges corresponding to + * passing a constant case test in a switch statement which is switching on + * an expression which data flows to from `inputNode`. + */ +private ControlFlow::Node getANonTestPassingPredecessor( + ControlFlow::Node succ, DataFlow::Node inputNode +) { + isPossibleInputNode(inputNode, succ.getRoot().(FuncDef)) and + result = succ.getAPredecessor() and + not exists(Expr testExpr, DataFlow::Node switchExprNode | + flowsToSwitchExpression(inputNode, switchExprNode) and + ControlFlow::isSwitchCaseTestPassingEdge(result, succ, switchExprNode.asExpr(), testExpr) and + testExpr.isConst() + ) +} + +private ControlFlow::Node getANonTestPassingReachingNodeRecursive( + ControlFlow::Node n, DataFlow::Node inputNode +) { + isPossibleInputNode(inputNode, n.getRoot().(FuncDef)) and + ( + result = n or + result = + getANonTestPassingReachingNodeRecursive(getANonTestPassingPredecessor(n, inputNode), inputNode) + ) +} + +/** + * Gets a node by following predecessors from `ret` without following edges + * corresponding to passing a constant case test in a switch statement which is + * switching on an expression which data flows to from `inputNode`. + */ +private ControlFlow::Node getANonTestPassingReachingNodeBase( + IR::ReturnInstruction ret, DataFlow::Node inputNode +) { + result = getANonTestPassingReachingNodeRecursive(ret, inputNode) +} + +/** + * Holds if every way to get from the entry node of the function to `ret` + * involves passing a constant test case in a switch statement which is + * switching on an expression which data flows to from `inputNode`. + */ +private predicate mustPassConstantCaseTestToReach( + IR::ReturnInstruction ret, DataFlow::Node inputNode +) { + isPossibleInputNode(inputNode, ret.getRoot().(FuncDef)) and + not exists(ControlFlow::Node entry | entry = ret.getRoot().getEntryNode() | + entry = getANonTestPassingReachingNodeBase(ret, inputNode) + ) +} + +/** + * Holds if whenever `outp` of function `f` satisfies `p`, the input `inp` of + * `f` matched a constant in a case clause of a switch statement. + * + * We check this by looking for guards on `inp` that collectively dominate all + * the `return` statements in `f` that can return `true`. This means that if + * `f` returns `true`, one of the guards must have been satisfied. (Similar + * reasoning is applied for statements returning `false`, `nil` or a non-`nil` + * value.) + */ +predicate functionEnsuresInputIsConstant( + Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p +) { + exists(FuncDecl fd | fd.getFunction() = f | + exists(boolean b | + p.isBoolean(b) and + forex(DataFlow::Node ret, IR::ReturnInstruction ri | + ret = outp.getEntryNode(fd) and + ri.getReturnStmt().getAnExpr() = ret.asExpr() and + DataFlow::possiblyReturnsBool(fd, outp, ret, b) + | + mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) + ) + ) + or + p.isNonNil() and + forex(DataFlow::Node ret, IR::ReturnInstruction ri | + ret = outp.getEntryNode(fd) and + ri.getReturnStmt().getAnExpr() = ret.asExpr() and + DataFlow::possiblyReturnsNonNil(fd, outp, ret) + | + mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) + ) + or + p.isNil() and + forex(DataFlow::Node ret, IR::ReturnInstruction ri | + ret = outp.getEntryNode(fd) and + ri.getReturnStmt().getAnExpr() = ret.asExpr() and + ret.asExpr() = Builtin::nil().getAReference() + | + exists(DataFlow::Node exprNode | + DataFlow::localFlow(inp.getExitNode(fd), exprNode) and + mustPassConstantCaseTestToReach(ri, inp.getExitNode(fd)) + ) + ) + ) +} + +/** + * Holds if whenever `outputNode` satisfies `p`, `inputNode` matched a constant + * in a case clause of a switch statement. + */ +pragma[noinline] +predicate inputIsConstantIfOutputHasProperty( + DataFlow::Node inputNode, DataFlow::Node outputNode, DataFlow::Property p +) { + exists(Function f, FunctionInput inp, FunctionOutput outp, DataFlow::CallNode call | + functionEnsuresInputIsConstant(f, inp, outp, p) and + call = f.getACall() and + inputNode = inp.getNode(call) and + DataFlow::localFlow(outp.getNode(call), outputNode) + ) +} + +/** + * A comparison against a list of constants, acting as a sanitizer guard for + * `guardedExpr` by restricting it to a known value. + * + * Currently this only looks for functions containing a switch statement, but + * it could equally look for a check for membership of a constant map or + * constant array, which does not need to be in its own function. + */ +class ListOfConstantsComparisonSanitizerGuard extends TaintTracking::DefaultTaintSanitizerGuard { + DataFlow::Node guardedExpr; + boolean outcome; + + ListOfConstantsComparisonSanitizerGuard() { + exists(DataFlow::Node outputNode, DataFlow::Property p | + inputIsConstantIfOutputHasProperty(guardedExpr, outputNode, p) and + p.checkOn(this, outcome, outputNode) + ) + } + + override predicate checks(Expr e, boolean branch) { + e = guardedExpr.asExpr() and branch = outcome + } +}