diff --git a/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql b/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql index 2d16b2ffea..88cc11ef80 100644 --- a/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql +++ b/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql @@ -15,8 +15,9 @@ import cpp import codingstandards.c.cert import semmle.code.cpp.security.FunctionWithWrappers import semmle.code.cpp.security.Security -import semmle.code.cpp.security.TaintTracking -import TaintedWithPath +import semmle.code.cpp.ir.IR +import semmle.code.cpp.ir.dataflow.TaintTracking +import DataFlow::PathGraph // Query TaintedPath.ql from the CodeQL standard library /** @@ -45,20 +46,93 @@ class FileFunction extends FunctionWithWrappers { override predicate interestingArg(int arg) { arg = 0 } } -class TaintedPathConfiguration extends TaintTrackingConfiguration { - override predicate isSink(Element tainted) { - exists(FileFunction fileFunction | fileFunction.outermostWrapperFunctionCall(tainted, _)) +Expr asSourceExpr(DataFlow::Node node) { + result = node.asConvertedExpr() + or + result = node.asDefiningArgument() +} + +Expr asSinkExpr(DataFlow::Node node) { + result = + node.asOperand() + .(SideEffectOperand) + .getUse() + .(ReadSideEffectInstruction) + .getArgumentDef() + .getUnconvertedResultExpression() +} + +/** + * Holds for a variable that has any kind of upper-bound check anywhere in the program. + * This is biased towards being inclusive and being a coarse overapproximation because + * there are a lot of valid ways of doing an upper bounds checks if we don't consider + * where it occurs, for example: + * ```cpp + * if (x < 10) { sink(x); } + * + * if (10 > y) { sink(y); } + * + * if (z > 10) { z = 10; } + * sink(z); + * ``` + */ +predicate hasUpperBoundsCheck(Variable var) { + exists(RelationalOperation oper, VariableAccess access | + oper.getAnOperand() = access and + access.getTarget() = var and + // Comparing to 0 is not an upper bound check + not oper.getAnOperand().getValue() = "0" + ) +} + +class TaintedPathConfiguration extends TaintTracking::Configuration { + TaintedPathConfiguration() { this = "TaintedPathConfiguration" } + + override predicate isSource(DataFlow::Node node) { isUserInput(asSourceExpr(node), _) } + + override predicate isSink(DataFlow::Node node) { + exists(FileFunction fileFunction | + fileFunction.outermostWrapperFunctionCall(asSinkExpr(node), _) + ) + } + + override predicate isSanitizerIn(DataFlow::Node node) { this.isSource(node) } + + override predicate isSanitizer(DataFlow::Node node) { + node.asExpr().(Call).getTarget().getUnspecifiedType() instanceof ArithmeticType + or + exists(LoadInstruction load, Variable checkedVar | + load = node.asInstruction() and + checkedVar = load.getSourceAddress().(VariableAddressInstruction).getAstVariable() and + hasUpperBoundsCheck(checkedVar) + ) + } + + predicate hasFilteredFlowPath(DataFlow::PathNode source, DataFlow::PathNode sink) { + this.hasFlowPath(source, sink) and + // The use of `isUserInput` in `isSink` in combination with `asSourceExpr` causes + // duplicate results. Filter these duplicates. The proper solution is to switch to + // using `LocalFlowSource` and `RemoteFlowSource`, but this currently only supports + // a subset of the cases supported by `isUserInput`. + not exists(DataFlow::PathNode source2 | + this.hasFlowPath(source2, sink) and + asSourceExpr(source.getNode()) = asSourceExpr(source2.getNode()) + | + not exists(source.getNode().asConvertedExpr()) and exists(source2.getNode().asConvertedExpr()) + ) } } from - FileFunction fileFunction, Expr taintedArg, Expr taintSource, PathNode sourceNode, - PathNode sinkNode, string taintCause, string callChain + FileFunction fileFunction, Expr taintedArg, Expr taintSource, TaintedPathConfiguration cfg, + DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, string taintCause, string callChain where not isExcluded(taintedArg, IO3Package::doNotPerformFileOperationsOnDevicesQuery()) and + taintedArg = asSinkExpr(sinkNode.getNode()) and fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and - taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and + cfg.hasFilteredFlowPath(sourceNode, sinkNode) and + taintSource = asSourceExpr(sourceNode.getNode()) and isUserInput(taintSource, taintCause) select taintedArg, sourceNode, sinkNode, - "This argument to a file access function is derived from $@ and then passed to " + callChain, + "This argument to a file access function is derived from $@ and then passed to " + callChain + ".", taintSource, "user input (" + taintCause + ")" diff --git a/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected b/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected index b4852b09e7..b4f07d6ca8 100644 --- a/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected +++ b/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected @@ -1,36 +1,16 @@ edges -| test.c:20:15:20:23 | array to pointer conversion | test.c:21:8:21:16 | (const char *)... | -| test.c:20:15:20:23 | array to pointer conversion | test.c:21:8:21:16 | file_name | -| test.c:20:15:20:23 | array to pointer conversion | test.c:21:8:21:16 | file_name indirection | -| test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | (const char *)... | -| test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name | | test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name indirection | -| test.c:20:15:20:23 | scanf output argument | test.c:21:8:21:16 | (const char *)... | -| test.c:20:15:20:23 | scanf output argument | test.c:21:8:21:16 | file_name | | test.c:20:15:20:23 | scanf output argument | test.c:21:8:21:16 | file_name indirection | -| test.c:45:15:45:23 | array to pointer conversion | test.c:46:29:46:37 | (LPCTSTR)... | -| test.c:45:15:45:23 | array to pointer conversion | test.c:46:29:46:37 | file_name | -| test.c:45:15:45:23 | array to pointer conversion | test.c:46:29:46:37 | file_name indirection | -| test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | (LPCTSTR)... | -| test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name | | test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name indirection | -| test.c:45:15:45:23 | scanf output argument | test.c:46:29:46:37 | (LPCTSTR)... | -| test.c:45:15:45:23 | scanf output argument | test.c:46:29:46:37 | file_name | | test.c:45:15:45:23 | scanf output argument | test.c:46:29:46:37 | file_name indirection | -subpaths nodes -| test.c:20:15:20:23 | array to pointer conversion | semmle.label | array to pointer conversion | | test.c:20:15:20:23 | file_name | semmle.label | file_name | | test.c:20:15:20:23 | scanf output argument | semmle.label | scanf output argument | -| test.c:21:8:21:16 | (const char *)... | semmle.label | (const char *)... | -| test.c:21:8:21:16 | file_name | semmle.label | file_name | | test.c:21:8:21:16 | file_name indirection | semmle.label | file_name indirection | -| test.c:45:15:45:23 | array to pointer conversion | semmle.label | array to pointer conversion | | test.c:45:15:45:23 | file_name | semmle.label | file_name | | test.c:45:15:45:23 | scanf output argument | semmle.label | scanf output argument | -| test.c:46:29:46:37 | (LPCTSTR)... | semmle.label | (LPCTSTR)... | -| test.c:46:29:46:37 | file_name | semmle.label | file_name | | test.c:46:29:46:37 | file_name indirection | semmle.label | file_name indirection | +subpaths #select -| test.c:21:8:21:16 | file_name | test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name | This argument to a file access function is derived from $@ and then passed to func(file_name), which calls fopen((unnamed parameter 0)) | test.c:20:15:20:23 | file_name | user input (scanf) | -| test.c:46:29:46:37 | file_name | test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name | This argument to a file access function is derived from $@ and then passed to CreateFile(lpFileName) | test.c:45:15:45:23 | file_name | user input (scanf) | +| test.c:21:8:21:16 | file_name | test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name indirection | This argument to a file access function is derived from $@ and then passed to func(file_name), which calls fopen((unnamed parameter 0)). | test.c:20:15:20:23 | file_name | user input (scanf) | +| test.c:46:29:46:37 | file_name | test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name indirection | This argument to a file access function is derived from $@ and then passed to CreateFile(lpFileName). | test.c:45:15:45:23 | file_name | user input (scanf) | diff --git a/change_notes/2022-12-06-remove-use-of-default-taint-tracking.md b/change_notes/2022-12-06-remove-use-of-default-taint-tracking.md new file mode 100644 index 0000000000..2f0c6706fc --- /dev/null +++ b/change_notes/2022-12-06-remove-use-of-default-taint-tracking.md @@ -0,0 +1,2 @@ + - `FIO32-C` - `DoNotPerformFileOperationsOnDevices.ql`: + - The query was rewritten to no longer depend of the `DefaultTaintTracking` library, which will be deprecated.