From 7834626e26ffae64b0d758383008ef58b285d183 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Thu, 23 Nov 2023 11:32:06 +0100 Subject: [PATCH] C++: Rewrite `cpp/tainted-permissions-check` to not use `DefaultTaintTracking` --- .../Security/CWE/CWE-807/TaintedCondition.ql | 73 ++++++++++++++++--- .../TaintedCondition.expected | 12 ++- 2 files changed, 68 insertions(+), 17 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-807/TaintedCondition.ql b/cpp/ql/src/Security/CWE/CWE-807/TaintedCondition.ql index c5d7f2cbb61..3e4ceef974d 100644 --- a/cpp/ql/src/Security/CWE/CWE-807/TaintedCondition.ql +++ b/cpp/ql/src/Security/CWE/CWE-807/TaintedCondition.ql @@ -12,30 +12,83 @@ * external/cwe/cwe-807 */ -import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl -import TaintedWithPath +import cpp +import semmle.code.cpp.security.Security +import semmle.code.cpp.security.FlowSources +import semmle.code.cpp.ir.dataflow.TaintTracking +import semmle.code.cpp.ir.IR +import Flow::PathGraph + +Expr getExprWithoutNot(Expr expr) { + result = expr and not expr instanceof NotExpr + or + result = getExprWithoutNot(expr.(NotExpr).getOperand()) and expr instanceof NotExpr +} predicate sensitiveCondition(Expr condition, Expr raise) { raisesPrivilege(raise) and exists(IfStmt ifstmt | - ifstmt.getCondition() = condition and + getExprWithoutNot(ifstmt.getCondition()) = condition and raise.getEnclosingStmt().getParentStmt*() = ifstmt ) } -class Configuration extends TaintTrackingConfiguration { - override predicate isSink(Element tainted) { sensitiveCondition(tainted, _) } +private predicate constantInstruction(Instruction instr) { + instr instanceof ConstantInstruction + or + instr instanceof StringConstantInstruction + or + constantInstruction(instr.(UnaryInstruction).getUnary()) } +predicate isSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() } + +module Config implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { isSource(node, _) } + + predicate isSink(DataFlow::Node node) { + sensitiveCondition([node.asExpr(), node.asIndirectExpr()], _) + } + + predicate isBarrier(DataFlow::Node node) { + // Block flow into binary instructions if both operands are non-constant + exists(BinaryInstruction iTo | + iTo = node.asInstruction() and + not constantInstruction(iTo.getLeft()) and + not constantInstruction(iTo.getRight()) and + // propagate taint from either the pointer or the offset, regardless of constant-ness + not iTo instanceof PointerArithmeticInstruction + ) + or + // Block flow through calls to pure functions if two or more operands are non-constant + exists(Instruction iFrom1, Instruction iFrom2, CallInstruction iTo | + iTo = node.asInstruction() and + isPureFunction(iTo.getStaticCallTarget().getName()) and + iFrom1 = iTo.getAnArgument() and + iFrom2 = iTo.getAnArgument() and + not constantInstruction(iFrom1) and + not constantInstruction(iFrom2) and + iFrom1 != iFrom2 + ) + } +} + +module Flow = TaintTracking::Global; + /* * Produce an alert if there is an 'if' statement whose condition `condition` * is influenced by tainted data `source`, and the body contains * `raise` which escalates privilege. */ -from Expr source, Expr condition, Expr raise, PathNode sourceNode, PathNode sinkNode +from + Expr raise, string sourceType, DataFlow::Node source, DataFlow::Node sink, + Flow::PathNode sourceNode, Flow::PathNode sinkNode where - taintedWithPath(source, condition, sourceNode, sinkNode) and - sensitiveCondition(condition, raise) -select condition, sourceNode, sinkNode, "Reliance on untrusted input $@ to raise privilege at $@.", - source, source.toString(), raise, raise.toString() + source = sourceNode.getNode() and + sink = sinkNode.getNode() and + isSource(source, sourceType) and + sensitiveCondition([sink.asExpr(), sink.asIndirectExpr()], raise) and + Flow::flowPath(sourceNode, sinkNode) +select sink, sourceNode, sinkNode, "Reliance on $@ to raise privilege at $@.", source, sourceType, + raise, raise.toString() diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/TaintedCondition.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/TaintedCondition.expected index c2bd2653994..f5f0feb9040 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/TaintedCondition.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/TaintedCondition.expected @@ -1,13 +1,11 @@ edges -| test.cpp:20:29:20:34 | call to getenv | test.cpp:24:10:24:35 | ! ... | -| test.cpp:20:29:20:34 | call to getenv | test.cpp:24:11:24:16 | call to strcmp | -| test.cpp:20:29:20:47 | call to getenv | test.cpp:24:10:24:35 | ! ... | | test.cpp:20:29:20:47 | call to getenv | test.cpp:24:11:24:16 | call to strcmp | -subpaths +| test.cpp:20:29:20:47 | call to getenv indirection | test.cpp:24:11:24:16 | call to strcmp | nodes -| test.cpp:20:29:20:34 | call to getenv | semmle.label | call to getenv | | test.cpp:20:29:20:47 | call to getenv | semmle.label | call to getenv | -| test.cpp:24:10:24:35 | ! ... | semmle.label | ! ... | +| test.cpp:20:29:20:47 | call to getenv indirection | semmle.label | call to getenv indirection | | test.cpp:24:11:24:16 | call to strcmp | semmle.label | call to strcmp | +subpaths #select -| test.cpp:24:10:24:35 | ! ... | test.cpp:20:29:20:34 | call to getenv | test.cpp:24:10:24:35 | ! ... | Reliance on untrusted input $@ to raise privilege at $@. | test.cpp:20:29:20:34 | call to getenv | call to getenv | test.cpp:25:9:25:27 | ... = ... | ... = ... | +| test.cpp:24:11:24:16 | call to strcmp | test.cpp:20:29:20:47 | call to getenv | test.cpp:24:11:24:16 | call to strcmp | Reliance on $@ to raise privilege at $@. | test.cpp:20:29:20:47 | call to getenv | an environment variable | test.cpp:25:9:25:27 | ... = ... | ... = ... | +| test.cpp:24:11:24:16 | call to strcmp | test.cpp:20:29:20:47 | call to getenv indirection | test.cpp:24:11:24:16 | call to strcmp | Reliance on $@ to raise privilege at $@. | test.cpp:20:29:20:47 | call to getenv indirection | an environment variable | test.cpp:25:9:25:27 | ... = ... | ... = ... |