Skip to content

Commit 2c50014

Browse files
authored
Merge pull request github#11435 from jketema/rewrite-tainted-path
C++: Rewrite `cpp/path-injection` to not use `DefaultTaintTracking`
2 parents 8e4190d + 995efef commit 2c50014

File tree

5 files changed

+139
-34
lines changed

5 files changed

+139
-34
lines changed

cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql

Lines changed: 82 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@
1717
import cpp
1818
import semmle.code.cpp.security.FunctionWithWrappers
1919
import semmle.code.cpp.security.Security
20-
import semmle.code.cpp.security.TaintTracking
21-
import TaintedWithPath
20+
import semmle.code.cpp.ir.IR
21+
import semmle.code.cpp.ir.dataflow.TaintTracking
22+
import DataFlow::PathGraph
2223

2324
/**
2425
* A function for opening a file.
@@ -46,18 +47,91 @@ class FileFunction extends FunctionWithWrappers {
4647
override predicate interestingArg(int arg) { arg = 0 }
4748
}
4849

49-
class TaintedPathConfiguration extends TaintTrackingConfiguration {
50-
override predicate isSink(Element tainted) {
51-
exists(FileFunction fileFunction | fileFunction.outermostWrapperFunctionCall(tainted, _))
50+
Expr asSourceExpr(DataFlow::Node node) {
51+
result = node.asConvertedExpr()
52+
or
53+
result = node.asDefiningArgument()
54+
}
55+
56+
Expr asSinkExpr(DataFlow::Node node) {
57+
result =
58+
node.asOperand()
59+
.(SideEffectOperand)
60+
.getUse()
61+
.(ReadSideEffectInstruction)
62+
.getArgumentDef()
63+
.getUnconvertedResultExpression()
64+
}
65+
66+
/**
67+
* Holds for a variable that has any kind of upper-bound check anywhere in the program.
68+
* This is biased towards being inclusive and being a coarse overapproximation because
69+
* there are a lot of valid ways of doing an upper bounds checks if we don't consider
70+
* where it occurs, for example:
71+
* ```cpp
72+
* if (x < 10) { sink(x); }
73+
*
74+
* if (10 > y) { sink(y); }
75+
*
76+
* if (z > 10) { z = 10; }
77+
* sink(z);
78+
* ```
79+
*/
80+
predicate hasUpperBoundsCheck(Variable var) {
81+
exists(RelationalOperation oper, VariableAccess access |
82+
oper.getAnOperand() = access and
83+
access.getTarget() = var and
84+
// Comparing to 0 is not an upper bound check
85+
not oper.getAnOperand().getValue() = "0"
86+
)
87+
}
88+
89+
class TaintedPathConfiguration extends TaintTracking::Configuration {
90+
TaintedPathConfiguration() { this = "TaintedPathConfiguration" }
91+
92+
override predicate isSource(DataFlow::Node node) { isUserInput(asSourceExpr(node), _) }
93+
94+
override predicate isSink(DataFlow::Node node) {
95+
exists(FileFunction fileFunction |
96+
fileFunction.outermostWrapperFunctionCall(asSinkExpr(node), _)
97+
)
98+
}
99+
100+
override predicate isSanitizerIn(DataFlow::Node node) { this.isSource(node) }
101+
102+
override predicate isSanitizer(DataFlow::Node node) {
103+
node.asExpr().(Call).getTarget().getUnspecifiedType() instanceof ArithmeticType
104+
or
105+
exists(LoadInstruction load, Variable checkedVar |
106+
load = node.asInstruction() and
107+
checkedVar = load.getSourceAddress().(VariableAddressInstruction).getAstVariable() and
108+
hasUpperBoundsCheck(checkedVar)
109+
)
110+
}
111+
112+
predicate hasFilteredFlowPath(DataFlow::PathNode source, DataFlow::PathNode sink) {
113+
this.hasFlowPath(source, sink) and
114+
// The use of `isUserInput` in `isSink` in combination with `asSourceExpr` causes
115+
// duplicate results. Filter these duplicates. The proper solution is to switch to
116+
// using `LocalFlowSource` and `RemoteFlowSource`, but this currently only supports
117+
// a subset of the cases supported by `isUserInput`.
118+
not exists(DataFlow::PathNode source2 |
119+
this.hasFlowPath(source2, sink) and
120+
asSourceExpr(source.getNode()) = asSourceExpr(source2.getNode())
121+
|
122+
not exists(source.getNode().asConvertedExpr()) and exists(source2.getNode().asConvertedExpr())
123+
)
52124
}
53125
}
54126

55127
from
56-
FileFunction fileFunction, Expr taintedArg, Expr taintSource, PathNode sourceNode,
57-
PathNode sinkNode, string taintCause, string callChain
128+
FileFunction fileFunction, Expr taintedArg, Expr taintSource, TaintedPathConfiguration cfg,
129+
DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, string taintCause, string callChain
58130
where
131+
taintedArg = asSinkExpr(sinkNode.getNode()) and
59132
fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and
60-
taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and
133+
cfg.hasFilteredFlowPath(sourceNode, sinkNode) and
134+
taintSource = asSourceExpr(sourceNode.getNode()) and
61135
isUserInput(taintSource, taintCause)
62136
select taintedArg, sourceNode, sinkNode,
63137
"This argument to a file access function is derived from $@ and then passed to " + callChain + ".",
Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,8 @@
11
edges
2-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | (const char *)... |
3-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
4-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
5-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection |
6-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | (const char *)... |
7-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
8-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
92
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection |
10-
subpaths
113
nodes
12-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | semmle.label | ... + ... |
134
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | semmle.label | fgets output argument |
14-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | (const char *)... | semmle.label | (const char *)... |
15-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | semmle.label | data |
16-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | semmle.label | data |
175
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection | semmle.label | data indirection |
6+
subpaths
187
#select
19-
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | This argument to a file access function is derived from $@ and then passed to fopen(filename). | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | user input (fgets) |
8+
| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | user input (fgets) |
Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,24 @@
11
edges
2-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | (const char *)... |
3-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | (const char *)... |
4-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
5-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
6-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
7-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
82
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection |
9-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection |
10-
subpaths
3+
| test.c:31:22:31:25 | argv | test.c:32:11:32:18 | fileName indirection |
4+
| test.c:37:17:37:24 | fileName | test.c:38:11:38:18 | fileName indirection |
5+
| test.c:37:17:37:24 | scanf output argument | test.c:38:11:38:18 | fileName indirection |
6+
| test.c:43:17:43:24 | fileName | test.c:44:11:44:18 | fileName indirection |
7+
| test.c:43:17:43:24 | scanf output argument | test.c:44:11:44:18 | fileName indirection |
118
nodes
129
| test.c:9:23:9:26 | argv | semmle.label | argv |
13-
| test.c:9:23:9:26 | argv | semmle.label | argv |
14-
| test.c:17:11:17:18 | (const char *)... | semmle.label | (const char *)... |
15-
| test.c:17:11:17:18 | fileName | semmle.label | fileName |
16-
| test.c:17:11:17:18 | fileName | semmle.label | fileName |
1710
| test.c:17:11:17:18 | fileName indirection | semmle.label | fileName indirection |
11+
| test.c:31:22:31:25 | argv | semmle.label | argv |
12+
| test.c:32:11:32:18 | fileName indirection | semmle.label | fileName indirection |
13+
| test.c:37:17:37:24 | fileName | semmle.label | fileName |
14+
| test.c:37:17:37:24 | scanf output argument | semmle.label | scanf output argument |
15+
| test.c:38:11:38:18 | fileName indirection | semmle.label | fileName indirection |
16+
| test.c:43:17:43:24 | fileName | semmle.label | fileName |
17+
| test.c:43:17:43:24 | scanf output argument | semmle.label | scanf output argument |
18+
| test.c:44:11:44:18 | fileName indirection | semmle.label | fileName indirection |
19+
subpaths
1820
#select
19-
| test.c:17:11:17:18 | fileName | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:9:23:9:26 | argv | user input (argv) |
21+
| test.c:17:11:17:18 | fileName | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:9:23:9:26 | argv | user input (argv) |
22+
| test.c:32:11:32:18 | fileName | test.c:31:22:31:25 | argv | test.c:32:11:32:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:31:22:31:25 | argv | user input (argv) |
23+
| test.c:38:11:38:18 | fileName | test.c:37:17:37:24 | fileName | test.c:38:11:38:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:37:17:37:24 | fileName | user input (scanf) |
24+
| test.c:44:11:44:18 | fileName | test.c:43:17:43:24 | fileName | test.c:44:11:44:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:43:17:43:24 | fileName | user input (scanf) |

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,7 @@ FILE *fopen(const char *filename, const char *mode);
1111
int sprintf(char *s, const char *format, ...);
1212
size_t strlen(const char *s);
1313
char *strncat(char *s1, const char *s2, size_t n);
14+
int scanf(const char *format, ...);
15+
void *malloc(size_t size);
16+
double strtod(const char *ptr, char **endptr);
17+
char *getenv(const char *name);

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/test.c

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,5 +26,38 @@ int main(int argc, char** argv) {
2626
strncat(fileName+len, fixed, FILENAME_MAX-len-1);
2727
fopen(fileName, "wb+");
2828
}
29+
30+
{
31+
char *fileName = argv[1];
32+
fopen(fileName, "wb+"); // BAD
33+
}
34+
35+
{
36+
char fileName[20];
37+
scanf("%s", fileName);
38+
fopen(fileName, "wb+"); // BAD
39+
}
40+
41+
{
42+
char *fileName = (char*)malloc(20 * sizeof(char));
43+
scanf("%s", fileName);
44+
fopen(fileName, "wb+"); // BAD
45+
}
46+
47+
{
48+
char *aNumber = getenv("A_NUMBER");
49+
double number = strtod(aNumber, 0);
50+
char fileName[20];
51+
sprintf(fileName, "/foo/%f", number);
52+
fopen(fileName, "wb+"); // GOOD
53+
}
54+
55+
{
56+
void read(const char *fileName);
57+
read(argv[1]); // BAD [NOT DETECTED]
58+
}
2959
}
3060

61+
void read(char *fileName) {
62+
fopen(fileName, "wb+");
63+
}

0 commit comments

Comments
 (0)