From 8dd0bdbdb0847863115615ea3c02bcba3c1c07d7 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Feb 2023 15:05:18 +0000 Subject: [PATCH 1/2] C++: Rename 'fst' and 'snd' to 'incoming' and 'outgoing'. --- .../src/Security/CWE/CWE-078/ExecTainted.ql | 45 ++++++++++--------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql b/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql index 51cc6a0660c..01dc143a042 100644 --- a/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql +++ b/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql @@ -25,15 +25,15 @@ import semmle.code.cpp.models.implementations.Strcat import DataFlow::PathGraph /** - * Holds if `fst` is a string that is used in a format or concatenation function resulting in `snd`, - * and is *not* placed at the start of the resulting string. This indicates that the author did not - * expect `fst` to control what program is run if the resulting string is eventually interpreted as - * a command line, for example as an argument to `system`. + * Holds if `incoming` is a string that is used in a format or concatenation function resulting + * in `outgoing`, and is *not* placed at the start of the resulting string. This indicates that + * the author did not expect `incoming` to control what program is run if the resulting string + * is eventually interpreted as a command line, for example as an argument to `system`. */ -predicate interestingConcatenation(DataFlow::Node fst, DataFlow::Node snd) { +predicate interestingConcatenation(DataFlow::Node incoming, DataFlow::Node outgoing) { exists(FormattingFunctionCall call, int index, FormatLiteral literal | - fst.asIndirectArgument() = call.getConversionArgument(index) and - snd.asDefiningArgument() = call.getOutputArgument(false) and + incoming.asIndirectArgument() = call.getConversionArgument(index) and + outgoing.asDefiningArgument() = call.getOutputArgument(false) and literal = call.getFormat() and not literal.getConvSpecOffset(index) = 0 and literal.getConversionChar(index) = ["s", "S"] @@ -42,16 +42,16 @@ predicate interestingConcatenation(DataFlow::Node fst, DataFlow::Node snd) { // strcat and friends exists(StrcatFunction strcatFunc, Call call | call.getTarget() = strcatFunc and - fst.asIndirectArgument() = call.getArgument(strcatFunc.getParamSrc()) and - snd.asDefiningArgument() = call.getArgument(strcatFunc.getParamDest()) + incoming.asIndirectArgument() = call.getArgument(strcatFunc.getParamSrc()) and + outgoing.asDefiningArgument() = call.getArgument(strcatFunc.getParamDest()) ) or exists(Call call, Operator op | call.getTarget() = op and op.hasQualifiedName("std", "operator+") and op.getType().(UserType).hasQualifiedName("std", "basic_string") and - fst.asIndirectArgument() = call.getArgument(1) and // left operand - call = snd.asInstruction().getUnconvertedResultExpression() + incoming.asIndirectArgument() = call.getArgument(1) and // left operand + call = outgoing.asInstruction().getUnconvertedResultExpression() ) } @@ -60,22 +60,23 @@ class ConcatState extends DataFlow::FlowState { } class ExecState extends DataFlow::FlowState { - DataFlow::Node fst; - DataFlow::Node snd; + DataFlow::Node incoming; + DataFlow::Node outgoing; ExecState() { this = - "ExecState (" + fst.getLocation() + " | " + fst + ", " + snd.getLocation() + " | " + snd + ")" and - interestingConcatenation(pragma[only_bind_into](fst), pragma[only_bind_into](snd)) + "ExecState (" + incoming.getLocation() + " | " + incoming + ", " + outgoing.getLocation() + + " | " + outgoing + ")" and + interestingConcatenation(pragma[only_bind_into](incoming), pragma[only_bind_into](outgoing)) } - DataFlow::Node getFstNode() { result = fst } + DataFlow::Node getIncomingNode() { result = incoming } - DataFlow::Node getSndNode() { result = snd } + DataFlow::Node getOutgoingNode() { result = outgoing } /** Holds if this is a possible `ExecState` for `sink`. */ predicate isFeasibleForSink(DataFlow::Node sink) { - any(ExecStateConfiguration conf).hasFlow(snd, sink) + any(ExecStateConfiguration conf).hasFlow(outgoing, sink) } } @@ -93,7 +94,7 @@ class ExecStateConfiguration extends TaintTracking2::Configuration { ExecStateConfiguration() { this = "ExecStateConfiguration" } override predicate isSource(DataFlow::Node source) { - exists(ExecState state | state.getSndNode() = source) + any(ExecState state).getOutgoingNode() = source } override predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _, _) } @@ -121,8 +122,8 @@ class ExecTaintConfiguration extends TaintTracking::Configuration { DataFlow::FlowState state2 ) { state1 instanceof ConcatState and - state2.(ExecState).getFstNode() = node1 and - state2.(ExecState).getSndNode() = node2 + state2.(ExecState).getIncomingNode() = node1 and + state2.(ExecState).getOutgoingNode() = node2 } override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) { @@ -146,7 +147,7 @@ where conf.hasFlowPath(sourceNode, sinkNode) and taintCause = sourceNode.getNode().(FlowSource).getSourceType() and isSinkImpl(sinkNode.getNode(), command, callChain) and - concatResult = sinkNode.getState().(ExecState).getSndNode() + concatResult = sinkNode.getState().(ExecState).getOutgoingNode() select command, sourceNode, sinkNode, "This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to " + callChain + ".", sourceNode, "user input (" + taintCause + ")", concatResult, From 075a83c9871ce9a3cb8cc67e2d813d08bebf37c9 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Feb 2023 15:05:29 +0000 Subject: [PATCH 2/2] Stage stats before on 'ExecTainted.ql' before: ``` 1 10 1 Fwd 609968 1398 -1 94 769936 ExecTaintConfiguration 2 15 1 Rev 239464 774 -1 52 320663 ExecTaintConfiguration 3 20 2 Fwd 205794 511 650 39 18576546 ExecTaintConfiguration 4 25 2 Rev 161966 351 428 39 13639502 ExecTaintConfiguration 5 30 3 Fwd 31889 322 791 39 5982574 ExecTaintConfiguration 6 35 3 Rev 30068 303 661 39 4181421 ExecTaintConfiguration 7 40 4 Fwd 24031 232 1432 39 14725618 ExecTaintConfiguration 8 45 4 Rev 21506 219 907 39 5962780 ExecTaintConfiguration 9 50 5 Fwd 20149 204 1527 38 8350094 ExecTaintConfiguration 10 55 5 Rev 20102 204 1472 38 7515307 ExecTaintConfiguration 11 60 6 Fwd 19950 200 904 33 9673369 ExecTaintConfiguration 12 65 6 Rev 18431 200 901 33 7030957 ExecTaintConfiguration ``` Stage stats after: ``` 1 10 1 Fwd 368610 699 -1 65 445199 ExecTaintConfiguration 2 15 1 Rev 112848 336 -1 23 150522 ExecTaintConfiguration 3 20 2 Fwd 91528 219 270 22 4120713 ExecTaintConfiguration 4 25 2 Rev 66017 141 159 22 2657398 ExecTaintConfiguration 5 30 3 Fwd 12161 119 208 22 792468 ExecTaintConfiguration 6 35 3 Rev 11640 111 167 22 569193 ExecTaintConfiguration 7 40 4 Fwd 11423 109 331 22 1203871 ExecTaintConfiguration 8 45 4 Rev 10851 107 323 22 904017 ExecTaintConfiguration 9 50 5 Fwd 10694 107 763 22 2428404 ExecTaintConfiguration 10 55 5 Rev 10332 104 735 22 2355698 ExecTaintConfiguration 11 60 6 Fwd 10302 104 729 22 5772762 ExecTaintConfiguration 12 65 6 Rev 9482 102 725 22 4020951 ExecTaintConfiguration ``` --- cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql b/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql index 01dc143a042..53d534a50ad 100644 --- a/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql +++ b/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql @@ -85,6 +85,12 @@ predicate isSinkImpl(DataFlow::Node sink, Expr command, string callChain) { shellCommand(command, callChain) } +predicate isSanitizerImpl(DataFlow::Node node) { + node.asExpr().getUnspecifiedType() instanceof IntegralType + or + node.asExpr().getUnspecifiedType() instanceof FloatingPointType +} + /** * A `TaintTracking` configuration that's used to find the relevant `ExecState`s for a * given sink. This avoids a cartesian product between all sinks and all `ExecState`s in @@ -99,6 +105,8 @@ class ExecStateConfiguration extends TaintTracking2::Configuration { override predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _, _) } + override predicate isSanitizer(DataFlow::Node node) { isSanitizerImpl(node) } + override predicate isSanitizerOut(DataFlow::Node node) { isSink(node, _) // Prevent duplicates along a call chain, since `shellCommand` will include wrappers } @@ -126,14 +134,7 @@ class ExecTaintConfiguration extends TaintTracking::Configuration { state2.(ExecState).getOutgoingNode() = node2 } - override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) { - ( - node.asInstruction().getResultType() instanceof IntegralType - or - node.asInstruction().getResultType() instanceof FloatingPointType - ) and - state instanceof ConcatState - } + override predicate isSanitizer(DataFlow::Node node) { isSanitizerImpl(node) } override predicate isSanitizerOut(DataFlow::Node node) { isSink(node, _) // Prevent duplicates along a call chain, since `shellCommand` will include wrappers