Skip to content

Commit 45cf634

Browse files
jbjjbj
authored
Merge pull request github#6184 from github/rdmarsh2/improve-exec-tainted
C++: Refactor ExecTainted.ql to only report results after string concatenation
2 parents e9b4e57 + d47c473 commit 45cf634

18 files changed

Lines changed: 793 additions & 103 deletions

File tree

‎cpp/change-notes/2021-09-27-command-line-injection.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The "Uncontrolled data used in OS command" (`cpp/command-line-injection`) query has been enhanced to reduce false positive results and its `@precision` increased to `high`

‎cpp/ql/lib/semmle/code/cpp/commons/Printf.qll‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,21 @@ class FormattingFunctionCall extends Expr {
253253
// format arguments must be known
254254
exists(getTarget().(FormattingFunction).getFirstFormatArgumentIndex())
255255
}
256+
257+
/**
258+
* Gets the argument, if any, to which the output is written. If `isStream` is
259+
* `true`, the output argument is a stream (that is, this call behaves like
260+
* `fprintf`). If `isStream` is `false`, the output argument is a buffer (that
261+
* is, this call behaves like `sprintf`)
262+
*/
263+
Expr getOutputArgument(boolean isStream) {
264+
result =
265+
this.(Call)
266+
.getArgument(this.(Call)
267+
.getTarget()
268+
.(FormattingFunction)
269+
.getOutputParameterIndex(isStream))
270+
}
256271
}
257272

258273
/**

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ private import semmle.code.cpp.ir.dataflow.DataFlow
44
private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
55
private import semmle.code.cpp.ir.dataflow.DataFlow3
66
private import semmle.code.cpp.ir.IR
7-
private import semmle.code.cpp.ir.dataflow.internal.DataFlowDispatch as Dispatch
7+
private import semmle.code.cpp.ir.dataflow.ResolveCall
88
private import semmle.code.cpp.controlflow.IRGuards
99
private import semmle.code.cpp.models.interfaces.Taint
1010
private import semmle.code.cpp.models.interfaces.DataFlow
@@ -355,20 +355,6 @@ predicate taintedIncludingGlobalVars(Expr source, Element tainted, string global
355355
*/
356356
GlobalOrNamespaceVariable globalVarFromId(string id) { id = result.getQualifiedName() }
357357

358-
/**
359-
* Resolve potential target function(s) for `call`.
360-
*
361-
* If `call` is a call through a function pointer (`ExprCall`) or
362-
* targets a virtual method, simple data flow analysis is performed
363-
* in order to identify target(s).
364-
*/
365-
Function resolveCall(Call call) {
366-
exists(CallInstruction callInstruction |
367-
callInstruction.getAST() = call and
368-
result = Dispatch::viableCallable(callInstruction)
369-
)
370-
}
371-
372358
/**
373359
* Provides definitions for augmenting source/sink pairs with data-flow paths
374360
* between them. From a `@kind path-problem` query, import this module in the

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/ResolveCall.qll‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* Provides a predicate for non-contextual virtual dispatch and function
3+
* pointer resolution.
4+
*/
5+
6+
import cpp
7+
private import semmle.code.cpp.ir.ValueNumbering
8+
private import internal.DataFlowDispatch
9+
private import semmle.code.cpp.ir.IR
10+
11+
/**
12+
* Resolve potential target function(s) for `call`.
13+
*
14+
* If `call` is a call through a function pointer (`ExprCall`) or its target is
15+
* a virtual member function, simple data flow analysis is performed in order
16+
* to identify the possible target(s).
17+
*/
18+
Function resolveCall(Call call) {
19+
exists(CallInstruction callInstruction |
20+
callInstruction.getAST() = call and
21+
result = viableCallable(callInstruction)
22+
)
23+
}

‎cpp/ql/lib/semmle/code/cpp/models/Models.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,3 +36,4 @@ private import implementations.Select
3636
private import implementations.MySql
3737
private import implementations.SqLite3
3838
private import implementations.PostgreSql
39+
private import implementations.System

‎cpp/ql/lib/semmle/code/cpp/models/implementations/System.qll‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import cpp
2+
import semmle.code.cpp.models.interfaces.SideEffect
3+
import semmle.code.cpp.models.interfaces.Alias
4+
import semmle.code.cpp.models.interfaces.CommandExecution
5+
6+
/**
7+
* A function for running a command using a command interpreter.
8+
*/
9+
private class SystemFunction extends CommandExecutionFunction, ArrayFunction, AliasFunction,
10+
SideEffectFunction {
11+
SystemFunction() {
12+
hasGlobalOrStdName("system") or // system(command)
13+
hasGlobalName("popen") or // popen(command, mode)
14+
// Windows variants
15+
hasGlobalName("_popen") or // _popen(command, mode)
16+
hasGlobalName("_wpopen") or // _wpopen(command, mode)
17+
hasGlobalName("_wsystem") // _wsystem(command)
18+
}
19+
20+
override predicate hasCommandArgument(FunctionInput input) { input.isParameterDeref(0) }
21+
22+
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 0 or bufParam = 1 }
23+
24+
override predicate hasArrayInput(int bufParam) { bufParam = 0 or bufParam = 1 }
25+
26+
override predicate parameterNeverEscapes(int index) { index = 0 or index = 1 }
27+
28+
override predicate parameterEscapesOnlyViaReturn(int index) { none() }
29+
30+
override predicate parameterIsAlwaysReturned(int index) { none() }
31+
32+
override predicate hasOnlySpecificReadSideEffects() { any() }
33+
34+
override predicate hasOnlySpecificWriteSideEffects() {
35+
hasGlobalOrStdName("system") or
36+
hasGlobalName("_wsystem")
37+
}
38+
39+
override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
40+
(i = 0 or i = 1) and
41+
buffer = true
42+
}
43+
}

‎cpp/ql/lib/semmle/code/cpp/models/interfaces/CommandExecution.qll‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* Provides classes for modeling functions that execute new programs by
3+
* interpreting string data as shell commands. To use this QL library, create
4+
* a QL class extending `CommandExecutionFunction` with a characteristic
5+
* predicate that selects the function or set of functions you are modeling.
6+
* Within that class, override the `hasCommandArgument` predicate to indicate
7+
* which parameters are interpreted as shell commands.
8+
*/
9+
10+
import cpp
11+
import FunctionInputsAndOutputs
12+
import semmle.code.cpp.models.Models
13+
14+
/**
15+
* A function, such as `exec` or `popen` that starts a new process by
16+
* interpreting a string as a shell command.
17+
*/
18+
abstract class CommandExecutionFunction extends Function {
19+
/**
20+
* Holds if `input` is interpreted as a shell command.
21+
*/
22+
abstract predicate hasCommandArgument(FunctionInput input);
23+
}

‎cpp/ql/lib/semmle/code/cpp/security/CommandExecution.qll‎

Lines changed: 10 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -4,42 +4,20 @@ import cpp
44
import semmle.code.cpp.security.FunctionWithWrappers
55
import semmle.code.cpp.models.interfaces.SideEffect
66
import semmle.code.cpp.models.interfaces.Alias
7+
import semmle.code.cpp.models.interfaces.CommandExecution
78

89
/**
910
* A function for running a command using a command interpreter.
1011
*/
11-
class SystemFunction extends FunctionWithWrappers, ArrayFunction, AliasFunction, SideEffectFunction {
12-
SystemFunction() {
13-
hasGlobalOrStdName("system") or // system(command)
14-
hasGlobalName("popen") or // popen(command, mode)
15-
// Windows variants
16-
hasGlobalName("_popen") or // _popen(command, mode)
17-
hasGlobalName("_wpopen") or // _wpopen(command, mode)
18-
hasGlobalName("_wsystem") // _wsystem(command)
19-
}
20-
21-
override predicate interestingArg(int arg) { arg = 0 }
22-
23-
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 0 or bufParam = 1 }
24-
25-
override predicate hasArrayInput(int bufParam) { bufParam = 0 or bufParam = 1 }
26-
27-
override predicate parameterNeverEscapes(int index) { index = 0 or index = 1 }
28-
29-
override predicate parameterEscapesOnlyViaReturn(int index) { none() }
30-
31-
override predicate parameterIsAlwaysReturned(int index) { none() }
32-
33-
override predicate hasOnlySpecificReadSideEffects() { any() }
34-
35-
override predicate hasOnlySpecificWriteSideEffects() {
36-
hasGlobalOrStdName("system") or
37-
hasGlobalName("_wsystem")
38-
}
39-
40-
override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
41-
(i = 0 or i = 1) and
42-
buffer = true
12+
class SystemFunction extends FunctionWithWrappers instanceof CommandExecutionFunction {
13+
override predicate interestingArg(int arg) {
14+
exists(FunctionInput input |
15+
this.(CommandExecutionFunction).hasCommandArgument(input) and
16+
(
17+
input.isParameterDerefOrQualifierObject(arg) or
18+
input.isParameterOrQualifierAddress(arg)
19+
)
20+
)
4321
}
4422
}
4523

‎cpp/ql/lib/semmle/code/cpp/security/FunctionWithWrappers.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717

1818
import cpp
1919
import PrintfLike
20-
private import TaintTracking
20+
private import semmle.code.cpp.ir.dataflow.ResolveCall
2121

2222
bindingset[index]
2323
private string toCause(Function func, int index) {

‎cpp/ql/src/Security/CWE/CWE-078/ExecTainted.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ int main(int argc, char** argv) {
99
system(command1);
1010
}
1111

12-
{
12+
{
1313
// GOOD: the user string is encoded by a library routine.
1414
char userNameQuoted[1000] = {0};
1515
encodeShellString(userNameQuoted, 1000, userName);

0 commit comments

Comments
 (0)