From c2e7fa09fa79cc2b8158ddda3028c11d536c6229 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Thu, 16 Mar 2023 11:39:37 +0100 Subject: [PATCH 1/2] Update FIO32-C with the latest version of the query from CodeQL The update is required due to changes in the dataflow library in CodeQL version 2.12.5. --- .../DoNotPerformFileOperationsOnDevices.ql | 63 +++++-------------- ...NotPerformFileOperationsOnDevices.expected | 4 +- 2 files changed, 17 insertions(+), 50 deletions(-) diff --git a/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql b/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql index 88cc11ef80..89e1f9e133 100644 --- a/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql +++ b/c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql @@ -14,10 +14,10 @@ import cpp import codingstandards.c.cert import semmle.code.cpp.security.FunctionWithWrappers -import semmle.code.cpp.security.Security +import semmle.code.cpp.security.FlowSources import semmle.code.cpp.ir.IR import semmle.code.cpp.ir.dataflow.TaintTracking -import DataFlow::PathGraph +import TaintedPath::PathGraph // Query TaintedPath.ql from the CodeQL standard library /** @@ -46,22 +46,6 @@ class FileFunction extends FunctionWithWrappers { override predicate interestingArg(int arg) { arg = 0 } } -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 @@ -85,20 +69,16 @@ predicate hasUpperBoundsCheck(Variable var) { ) } -class TaintedPathConfiguration extends TaintTracking::Configuration { - TaintedPathConfiguration() { this = "TaintedPathConfiguration" } - - override predicate isSource(DataFlow::Node node) { isUserInput(asSourceExpr(node), _) } +module TaintedPathConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node instanceof FlowSource } - override predicate isSink(DataFlow::Node node) { + predicate isSink(DataFlow::Node node) { exists(FileFunction fileFunction | - fileFunction.outermostWrapperFunctionCall(asSinkExpr(node), _) + fileFunction.outermostWrapperFunctionCall(node.asIndirectArgument(), _) ) } - override predicate isSanitizerIn(DataFlow::Node node) { this.isSource(node) } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().(Call).getTarget().getUnspecifiedType() instanceof ArithmeticType or exists(LoadInstruction load, Variable checkedVar | @@ -107,32 +87,19 @@ class TaintedPathConfiguration extends TaintTracking::Configuration { 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()) - ) - } } +module TaintedPath = TaintTracking::Make; + from - FileFunction fileFunction, Expr taintedArg, Expr taintSource, TaintedPathConfiguration cfg, - DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, string taintCause, string callChain + FileFunction fileFunction, Expr taintedArg, FlowSource taintSource, + TaintedPath::PathNode sourceNode, TaintedPath::PathNode sinkNode, string callChain where not isExcluded(taintedArg, IO3Package::doNotPerformFileOperationsOnDevicesQuery()) and - taintedArg = asSinkExpr(sinkNode.getNode()) and + taintedArg = sinkNode.getNode().asIndirectArgument() and fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and - cfg.hasFilteredFlowPath(sourceNode, sinkNode) and - taintSource = asSourceExpr(sourceNode.getNode()) and - isUserInput(taintSource, taintCause) + TaintedPath::hasFlowPath(sourceNode, sinkNode) and + taintSource = sourceNode.getNode() select taintedArg, sourceNode, sinkNode, "This argument to a file access function is derived from $@ and then passed to " + callChain + ".", - taintSource, "user input (" + taintCause + ")" + taintSource, "user input (" + taintSource.getSourceType() + ")" diff --git a/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected b/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected index 824149f294..06bf56cf8a 100644 --- a/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected +++ b/c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected @@ -8,5 +8,5 @@ nodes | 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 | scanf output argument | 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 | scanf output argument | 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) | +| 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 | 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 | scanf output argument | user input (value read by scanf) | +| 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 | This argument to a file access function is derived from $@ and then passed to CreateFile(lpFileName). | test.c:45:15:45:23 | scanf output argument | user input (value read by scanf) | From d1c0a5d55e1399c40c729a91be354e8c74ccad3a Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Thu, 16 Mar 2023 11:47:23 +0100 Subject: [PATCH 2/2] Add change note --- change_notes/2022-03-16-update-for-dataflow-changes.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change_notes/2022-03-16-update-for-dataflow-changes.md diff --git a/change_notes/2022-03-16-update-for-dataflow-changes.md b/change_notes/2022-03-16-update-for-dataflow-changes.md new file mode 100644 index 0000000000..af0aaed7ca --- /dev/null +++ b/change_notes/2022-03-16-update-for-dataflow-changes.md @@ -0,0 +1,2 @@ + - `FIO32-C` - `DoNotPerformFileOperationsOnDevices.ql`: + - The query was updated to work with the latest version of the dataflow library.