Skip to content

Commit d56a9f0

Browse files
jketemajketema
authored
Merge pull request #14424 from jketema/rewrite-cgi-xss
C++: Rewrite `cpp/cgi-xss` to not use default taint tracking
2 parents fb0016e + 6167627 commit d56a9f0

File tree

3 files changed

+61
-39
lines changed

3 files changed

+61
-39
lines changed

‎cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql

+20-16
Original file line numberDiff line numberDiff line change
@@ -13,35 +13,39 @@
1313

1414
import cpp
1515
import semmle.code.cpp.commons.Environment
16-
import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
17-
import TaintedWithPath
16+
import semmle.code.cpp.ir.dataflow.TaintTracking
17+
import semmle.code.cpp.ir.IR
18+
import Flow::PathGraph
1819

1920
/** A call that prints its arguments to `stdout`. */
2021
class PrintStdoutCall extends FunctionCall {
21-
PrintStdoutCall() {
22-
this.getTarget().hasGlobalOrStdName("puts") or
23-
this.getTarget().hasGlobalOrStdName("printf")
24-
}
22+
PrintStdoutCall() { this.getTarget().hasGlobalOrStdName(["puts", "printf"]) }
2523
}
2624

2725
/** A read of the QUERY_STRING environment variable */
2826
class QueryString extends EnvironmentRead {
2927
QueryString() { this.getEnvironmentVariable() = "QUERY_STRING" }
3028
}
3129

32-
class Configuration extends TaintTrackingConfiguration {
33-
override predicate isSource(Expr source) { source instanceof QueryString }
30+
module Config implements DataFlow::ConfigSig {
31+
predicate isSource(DataFlow::Node node) { node.asIndirectExpr() instanceof QueryString }
3432

35-
override predicate isSink(Element tainted) {
36-
exists(PrintStdoutCall call | call.getAnArgument() = tainted)
33+
predicate isSink(DataFlow::Node node) {
34+
exists(PrintStdoutCall call | call.getAnArgument() = [node.asIndirectExpr(), node.asExpr()])
3735
}
3836

39-
override predicate isBarrier(Expr e) {
40-
super.isBarrier(e) or e.getUnspecifiedType() instanceof IntegralType
37+
predicate isBarrier(DataFlow::Node node) {
38+
isSink(node) and node.asExpr().getUnspecifiedType() instanceof ArithmeticType
39+
or
40+
node.asInstruction().(StoreInstruction).getResultType() instanceof ArithmeticType
4141
}
4242
}
4343

44-
from QueryString query, Element printedArg, PathNode sourceNode, PathNode sinkNode
45-
where taintedWithPath(query, printedArg, sourceNode, sinkNode)
46-
select printedArg, sourceNode, sinkNode, "Cross-site scripting vulnerability due to $@.", query,
47-
"this query data"
44+
module Flow = TaintTracking::Global<Config>;
45+
46+
from QueryString query, Flow::PathNode sourceNode, Flow::PathNode sinkNode
47+
where
48+
Flow::flowPath(sourceNode, sinkNode) and
49+
query = sourceNode.getNode().asIndirectExpr()
50+
select sinkNode.getNode(), sourceNode, sinkNode, "Cross-site scripting vulnerability due to $@.",
51+
query, "this query data"

‎cpp/ql/test/query-tests/Security/CWE/CWE-079/semmle/CgiXss/CgiXss.expected

+23-23
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,26 @@
11
edges
2-
| search.c:14:24:14:28 | query | search.c:17:8:17:12 | query |
3-
| search.c:14:24:14:28 | query | search.c:17:8:17:12 | query |
4-
| search.c:22:24:22:28 | query | search.c:23:39:23:43 | query |
5-
| search.c:22:24:22:28 | query | search.c:23:39:23:43 | query |
6-
| search.c:51:21:51:26 | call to getenv | search.c:55:17:55:25 | raw_query |
7-
| search.c:51:21:51:26 | call to getenv | search.c:55:17:55:25 | raw_query |
8-
| search.c:51:21:51:26 | call to getenv | search.c:57:17:57:25 | raw_query |
9-
| search.c:51:21:51:26 | call to getenv | search.c:57:17:57:25 | raw_query |
10-
| search.c:55:17:55:25 | raw_query | search.c:14:24:14:28 | query |
11-
| search.c:57:17:57:25 | raw_query | search.c:22:24:22:28 | query |
12-
subpaths
2+
| search.c:14:24:14:28 | query indirection | search.c:17:8:17:12 | query indirection |
3+
| search.c:22:24:22:28 | query indirection | search.c:23:39:23:43 | query indirection |
4+
| search.c:55:24:55:28 | query indirection | search.c:62:8:62:17 | query_text indirection |
5+
| search.c:67:21:67:26 | call to getenv indirection | search.c:71:17:71:25 | raw_query indirection |
6+
| search.c:67:21:67:26 | call to getenv indirection | search.c:73:17:73:25 | raw_query indirection |
7+
| search.c:67:21:67:26 | call to getenv indirection | search.c:77:17:77:25 | raw_query indirection |
8+
| search.c:71:17:71:25 | raw_query indirection | search.c:14:24:14:28 | query indirection |
9+
| search.c:73:17:73:25 | raw_query indirection | search.c:22:24:22:28 | query indirection |
10+
| search.c:77:17:77:25 | raw_query indirection | search.c:55:24:55:28 | query indirection |
1311
nodes
14-
| search.c:14:24:14:28 | query | semmle.label | query |
15-
| search.c:17:8:17:12 | query | semmle.label | query |
16-
| search.c:17:8:17:12 | query | semmle.label | query |
17-
| search.c:22:24:22:28 | query | semmle.label | query |
18-
| search.c:23:39:23:43 | query | semmle.label | query |
19-
| search.c:23:39:23:43 | query | semmle.label | query |
20-
| search.c:51:21:51:26 | call to getenv | semmle.label | call to getenv |
21-
| search.c:51:21:51:26 | call to getenv | semmle.label | call to getenv |
22-
| search.c:55:17:55:25 | raw_query | semmle.label | raw_query |
23-
| search.c:57:17:57:25 | raw_query | semmle.label | raw_query |
12+
| search.c:14:24:14:28 | query indirection | semmle.label | query indirection |
13+
| search.c:17:8:17:12 | query indirection | semmle.label | query indirection |
14+
| search.c:22:24:22:28 | query indirection | semmle.label | query indirection |
15+
| search.c:23:39:23:43 | query indirection | semmle.label | query indirection |
16+
| search.c:55:24:55:28 | query indirection | semmle.label | query indirection |
17+
| search.c:62:8:62:17 | query_text indirection | semmle.label | query_text indirection |
18+
| search.c:67:21:67:26 | call to getenv indirection | semmle.label | call to getenv indirection |
19+
| search.c:71:17:71:25 | raw_query indirection | semmle.label | raw_query indirection |
20+
| search.c:73:17:73:25 | raw_query indirection | semmle.label | raw_query indirection |
21+
| search.c:77:17:77:25 | raw_query indirection | semmle.label | raw_query indirection |
22+
subpaths
2423
#select
25-
| search.c:17:8:17:12 | query | search.c:51:21:51:26 | call to getenv | search.c:17:8:17:12 | query | Cross-site scripting vulnerability due to $@. | search.c:51:21:51:26 | call to getenv | this query data |
26-
| search.c:23:39:23:43 | query | search.c:51:21:51:26 | call to getenv | search.c:23:39:23:43 | query | Cross-site scripting vulnerability due to $@. | search.c:51:21:51:26 | call to getenv | this query data |
24+
| search.c:17:8:17:12 | query indirection | search.c:67:21:67:26 | call to getenv indirection | search.c:17:8:17:12 | query indirection | Cross-site scripting vulnerability due to $@. | search.c:67:21:67:26 | call to getenv | this query data |
25+
| search.c:23:39:23:43 | query indirection | search.c:67:21:67:26 | call to getenv indirection | search.c:23:39:23:43 | query indirection | Cross-site scripting vulnerability due to $@. | search.c:67:21:67:26 | call to getenv | this query data |
26+
| search.c:62:8:62:17 | query_text indirection | search.c:67:21:67:26 | call to getenv indirection | search.c:62:8:62:17 | query_text indirection | Cross-site scripting vulnerability due to $@. | search.c:67:21:67:26 | call to getenv | this query data |

‎cpp/ql/test/query-tests/Security/CWE/CWE-079/semmle/CgiXss/search.c

+18
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,22 @@ void good_server2(char* query) {
4747
printf("\n<p>%i</p>\n", i);
4848
}
4949

50+
typedef unsigned long size_t;
51+
size_t strlen(const char *s);
52+
char *strcpy(char *dst, const char *src);
53+
char *strcat(char *s1, const char *s2);
54+
55+
void bad_server3(char* query) {
56+
char query_text[strlen(query) + 8];
57+
strcpy(query_text, "query: ");
58+
strcat(query_text, query);
59+
60+
puts("<p>Query results for ");
61+
// BAD: Printing out an HTTP parameter with no escaping
62+
puts(query_text);
63+
puts("\n<p>\n");
64+
}
65+
5066
int main(int argc, char** argv) {
5167
char* raw_query = getenv("QUERY_STRING");
5268
if (strcmp("good1", argv[0]) == 0) {
@@ -57,5 +73,7 @@ int main(int argc, char** argv) {
5773
bad_server2(raw_query);
5874
} else if (strcmp("good2", argv[0]) == 0) {
5975
good_server2(raw_query);
76+
} else if (strcmp("bad3", argv[0]) == 0) {
77+
bad_server3(raw_query);
6078
}
6179
}

0 commit comments

Comments
 (0)