|
12 | 12 | */ |
13 | 13 |
|
14 | 14 | import cpp |
15 | | -import codingstandards.c.cert |
16 | 15 | import semmle.code.cpp.security.FunctionWithWrappers |
17 | 16 | import semmle.code.cpp.security.Security |
18 | | -import semmle.code.cpp.security.TaintTracking |
19 | | -import TaintedWithPath |
| 17 | +import semmle.code.cpp.ir.IR |
| 18 | +import semmle.code.cpp.ir.dataflow.TaintTracking |
| 19 | +import DataFlow::PathGraph |
20 | 20 |
|
21 | 21 | // Query TaintedPath.ql from the CodeQL standard library |
22 | 22 | /** |
@@ -45,20 +45,92 @@ class FileFunction extends FunctionWithWrappers { |
45 | 45 | override predicate interestingArg(int arg) { arg = 0 } |
46 | 46 | } |
47 | 47 |
|
48 | | -class TaintedPathConfiguration extends TaintTrackingConfiguration { |
49 | | - override predicate isSink(Element tainted) { |
50 | | - exists(FileFunction fileFunction | fileFunction.outermostWrapperFunctionCall(tainted, _)) |
| 48 | +Expr asSourceExpr(DataFlow::Node node) { |
| 49 | + result = node.asConvertedExpr() |
| 50 | + or |
| 51 | + result = node.asDefiningArgument() |
| 52 | +} |
| 53 | + |
| 54 | +Expr asSinkExpr(DataFlow::Node node) { |
| 55 | + result = |
| 56 | + node.asOperand() |
| 57 | + .(SideEffectOperand) |
| 58 | + .getUse() |
| 59 | + .(ReadSideEffectInstruction) |
| 60 | + .getArgumentDef() |
| 61 | + .getUnconvertedResultExpression() |
| 62 | +} |
| 63 | + |
| 64 | +/** |
| 65 | + * Holds for a variable that has any kind of upper-bound check anywhere in the program. |
| 66 | + * This is biased towards being inclusive and being a coarse overapproximation because |
| 67 | + * there are a lot of valid ways of doing an upper bounds checks if we don't consider |
| 68 | + * where it occurs, for example: |
| 69 | + * ```cpp |
| 70 | + * if (x < 10) { sink(x); } |
| 71 | + * |
| 72 | + * if (10 > y) { sink(y); } |
| 73 | + * |
| 74 | + * if (z > 10) { z = 10; } |
| 75 | + * sink(z); |
| 76 | + * ``` |
| 77 | + */ |
| 78 | +predicate hasUpperBoundsCheck(Variable var) { |
| 79 | + exists(RelationalOperation oper, VariableAccess access | |
| 80 | + oper.getAnOperand() = access and |
| 81 | + access.getTarget() = var and |
| 82 | + // Comparing to 0 is not an upper bound check |
| 83 | + not oper.getAnOperand().getValue() = "0" |
| 84 | + ) |
| 85 | +} |
| 86 | + |
| 87 | +class TaintedPathConfiguration extends TaintTracking::Configuration { |
| 88 | + TaintedPathConfiguration() { this = "TaintedPathConfiguration" } |
| 89 | + |
| 90 | + override predicate isSource(DataFlow::Node node) { isUserInput(asSourceExpr(node), _) } |
| 91 | + |
| 92 | + override predicate isSink(DataFlow::Node node) { |
| 93 | + exists(FileFunction fileFunction | |
| 94 | + fileFunction.outermostWrapperFunctionCall(asSinkExpr(node), _) |
| 95 | + ) |
| 96 | + } |
| 97 | + |
| 98 | + override predicate isSanitizerIn(DataFlow::Node node) { this.isSource(node) } |
| 99 | + |
| 100 | + override predicate isSanitizer(DataFlow::Node node) { |
| 101 | + node.asExpr().(Call).getTarget().getUnspecifiedType() instanceof ArithmeticType |
| 102 | + or |
| 103 | + exists(LoadInstruction load, Variable checkedVar | |
| 104 | + load = node.asInstruction() and |
| 105 | + checkedVar = load.getSourceAddress().(VariableAddressInstruction).getAstVariable() and |
| 106 | + hasUpperBoundsCheck(checkedVar) |
| 107 | + ) |
| 108 | + } |
| 109 | + |
| 110 | + predicate hasFilteredFlowPath(DataFlow::PathNode source, DataFlow::PathNode sink) { |
| 111 | + this.hasFlowPath(source, sink) and |
| 112 | + // The use of `isUserInput` in `isSink` in combination with `asSourceExpr` causes |
| 113 | + // duplicate results. Filter these duplicates. The proper solution is to switch to |
| 114 | + // using `LocalFlowSource` and `RemoteFlowSource`, but this currently only supports |
| 115 | + // a subset of the cases supported by `isUserInput`. |
| 116 | + not exists(DataFlow::PathNode source2 | |
| 117 | + this.hasFlowPath(source2, sink) and |
| 118 | + asSourceExpr(source.getNode()) = asSourceExpr(source2.getNode()) |
| 119 | + | |
| 120 | + not exists(source.getNode().asConvertedExpr()) and exists(source2.getNode().asConvertedExpr()) |
| 121 | + ) |
51 | 122 | } |
52 | 123 | } |
53 | 124 |
|
54 | 125 | from |
55 | | - FileFunction fileFunction, Expr taintedArg, Expr taintSource, PathNode sourceNode, |
56 | | - PathNode sinkNode, string taintCause, string callChain |
| 126 | + FileFunction fileFunction, Expr taintedArg, Expr taintSource, TaintedPathConfiguration cfg, |
| 127 | + DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, string taintCause, string callChain |
57 | 128 | where |
58 | | - not isExcluded(taintedArg, IO3Package::doNotPerformFileOperationsOnDevicesQuery()) and |
| 129 | + taintedArg = asSinkExpr(sinkNode.getNode()) and |
59 | 130 | fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and |
60 | | - taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and |
| 131 | + cfg.hasFilteredFlowPath(sourceNode, sinkNode) and |
| 132 | + taintSource = asSourceExpr(sourceNode.getNode()) and |
61 | 133 | isUserInput(taintSource, taintCause) |
62 | 134 | select taintedArg, sourceNode, sinkNode, |
63 | | - "This argument to a file access function is derived from $@ and then passed to " + callChain, |
| 135 | + "This argument to a file access function is derived from $@ and then passed to " + callChain + ".", |
64 | 136 | taintSource, "user input (" + taintCause + ")" |
0 commit comments