Skip to content

Commit 5822cd7

Browse files
jbjjbj
committed
C++: Put paths in the remaining LGTM-suite queries
1 parent 3ec1f69 commit 5822cd7

File tree

14 files changed

+319
-76
lines changed

14 files changed

+319
-76
lines changed

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

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* @name CGI script vulnerable to cross-site scripting
33
* @description Writing user input directly to a web page
44
* allows for a cross-site scripting vulnerability.
5-
* @kind problem
5+
* @kind path-problem
66
* @problem.severity error
77
* @precision high
88
* @id cpp/cgi-xss
@@ -13,6 +13,7 @@
1313
import cpp
1414
import semmle.code.cpp.commons.Environment
1515
import semmle.code.cpp.security.TaintTracking
16+
import TaintedWithPath
1617

1718
/** A call that prints its arguments to `stdout`. */
1819
class PrintStdoutCall extends FunctionCall {
@@ -27,8 +28,13 @@ class QueryString extends EnvironmentRead {
2728
QueryString() { getEnvironmentVariable() = "QUERY_STRING" }
2829
}
2930

30-
from QueryString query, PrintStdoutCall call, Element printedArg
31-
where
32-
call.getAnArgument() = printedArg and
33-
tainted(query, printedArg)
34-
select printedArg, "Cross-site scripting vulnerability due to $@.", query, "this query data"
31+
class Configuration extends TaintTrackingConfiguration {
32+
override predicate isSink(Element tainted) {
33+
exists(PrintStdoutCall call | call.getAnArgument() = tainted)
34+
}
35+
}
36+
37+
from QueryString query, Element printedArg, PathNode sourceNode, PathNode sinkNode
38+
where taintedWithPath(query, printedArg, sourceNode, sinkNode)
39+
select printedArg, sourceNode, sinkNode, "Cross-site scripting vulnerability due to $@.", query,
40+
"this query data"

‎cpp/ql/src/Security/CWE/CWE-089/SqlTainted.ql‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @description Including user-supplied data in a SQL query without
44
* neutralizing special elements can make code vulnerable
55
* to SQL Injection.
6-
* @kind problem
6+
* @kind path-problem
77
* @problem.severity error
88
* @precision high
99
* @id cpp/sql-injection
@@ -15,18 +15,27 @@ import cpp
1515
import semmle.code.cpp.security.Security
1616
import semmle.code.cpp.security.FunctionWithWrappers
1717
import semmle.code.cpp.security.TaintTracking
18+
import TaintedWithPath
1819

1920
class SQLLikeFunction extends FunctionWithWrappers {
2021
SQLLikeFunction() { sqlArgument(this.getName(), _) }
2122

2223
override predicate interestingArg(int arg) { sqlArgument(this.getName(), arg) }
2324
}
2425

25-
from SQLLikeFunction runSql, Expr taintedArg, Expr taintSource, string taintCause, string callChain
26+
class Configuration extends TaintTrackingConfiguration {
27+
override predicate isSink(Element tainted) {
28+
exists(SQLLikeFunction runSql | runSql.outermostWrapperFunctionCall(tainted, _))
29+
}
30+
}
31+
32+
from
33+
SQLLikeFunction runSql, Expr taintedArg, Expr taintSource, PathNode sourceNode, PathNode sinkNode,
34+
string taintCause, string callChain
2635
where
2736
runSql.outermostWrapperFunctionCall(taintedArg, callChain) and
28-
tainted(taintSource, taintedArg) and
37+
taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and
2938
isUserInput(taintSource, taintCause)
30-
select taintedArg,
39+
select taintedArg, sourceNode, sinkNode,
3140
"This argument to a SQL query function is derived from $@ and then passed to " + callChain,
3241
taintSource, "user input (" + taintCause + ")"

‎cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @description Using externally controlled strings in a process
44
* operation can allow an attacker to execute malicious
55
* commands.
6-
* @kind problem
6+
* @kind path-problem
77
* @problem.severity warning
88
* @precision medium
99
* @id cpp/uncontrolled-process-operation
@@ -14,13 +14,24 @@
1414
import cpp
1515
import semmle.code.cpp.security.Security
1616
import semmle.code.cpp.security.TaintTracking
17+
import TaintedWithPath
1718

18-
from string processOperation, int processOperationArg, FunctionCall call, Expr arg, Element source
19+
predicate isProcessOperationExplanation(Expr arg, string processOperation) {
20+
exists(int processOperationArg, FunctionCall call |
21+
isProcessOperationArgument(processOperation, processOperationArg) and
22+
call.getTarget().getName() = processOperation and
23+
call.getArgument(processOperationArg) = arg
24+
)
25+
}
26+
27+
class Configuration extends TaintTrackingConfiguration {
28+
override predicate isSink(Element arg) { isProcessOperationExplanation(arg, _) }
29+
}
30+
31+
from string processOperation, Expr arg, Expr source, PathNode sourceNode, PathNode sinkNode
1932
where
20-
isProcessOperationArgument(processOperation, processOperationArg) and
21-
call.getTarget().getName() = processOperation and
22-
call.getArgument(processOperationArg) = arg and
23-
tainted(source, arg)
24-
select arg,
33+
isProcessOperationExplanation(arg, processOperation) and
34+
taintedWithPath(source, arg, sourceNode, sinkNode)
35+
select arg, sourceNode, sinkNode,
2536
"The value of this argument may come from $@ and is being passed to " + processOperation, source,
2637
source.toString()

‎cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql‎

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* @name Unbounded write
33
* @description Buffer write operations that do not control the length
44
* of data written may overflow.
5-
* @kind problem
5+
* @kind path-problem
66
* @problem.severity error
77
* @precision medium
88
* @id cpp/unbounded-write
@@ -16,6 +16,7 @@
1616
import semmle.code.cpp.security.BufferWrite
1717
import semmle.code.cpp.security.Security
1818
import semmle.code.cpp.security.TaintTracking
19+
import TaintedWithPath
1920

2021
/*
2122
* --- Summary of CWE-120 alerts ---
@@ -54,32 +55,48 @@ predicate isUnboundedWrite(BufferWrite bw) {
5455
* }
5556
*/
5657

58+
/**
59+
* Holds if `e` is a source buffer going into an unbounded write `bw` or a
60+
* qualifier of (a qualifier of ...) such a source.
61+
*/
62+
predicate unboundedWriteSource(Expr e, BufferWrite bw) {
63+
isUnboundedWrite(bw) and e = bw.getASource()
64+
or
65+
exists(FieldAccess fa | unboundedWriteSource(fa, bw) and e = fa.getQualifier())
66+
}
67+
5768
/*
5869
* --- user input reach ---
5970
*/
6071

61-
/**
62-
* Identifies expressions that are potentially tainted with user
63-
* input. Most of the work for this is actually done by the
64-
* TaintTracking library.
65-
*/
66-
predicate tainted2(Expr expr, Expr inputSource, string inputCause) {
67-
taintedIncludingGlobalVars(inputSource, expr, _) and
68-
inputCause = inputSource.toString()
69-
or
70-
exists(Expr e | tainted2(e, inputSource, inputCause) |
71-
// field accesses of a tainted struct are tainted
72-
e = expr.(FieldAccess).getQualifier()
73-
)
72+
class Configuration extends TaintTrackingConfiguration {
73+
override predicate isSink(Element tainted) { unboundedWriteSource(tainted, _) }
74+
75+
override predicate taintThroughGlobals() { any() }
7476
}
7577

7678
/*
7779
* --- put it together ---
7880
*/
7981

80-
from BufferWrite bw, Expr inputSource, string inputCause
82+
/*
83+
* An unbounded write is, for example `strcpy(..., tainted)`. We're looking
84+
* for a tainted source buffer of an unbounded write, where this source buffer
85+
* is a sink in the taint-tracking analysis.
86+
*
87+
* In the case of `gets` and `scanf`, where the source buffer is implicit, the
88+
* `BufferWrite` library reports the source buffer to be the same as the
89+
* destination buffer. Since those destination-buffer arguments are also
90+
* modeled in the taint-tracking library as being _sources_ of taint, they are
91+
* in practice reported as being tainted because the `security.TaintTracking`
92+
* library does not distinguish between taint going into an argument and out of
93+
* an argument. Thus, we get the desired alerts.
94+
*/
95+
96+
from BufferWrite bw, Expr inputSource, Expr tainted, PathNode sourceNode, PathNode sinkNode
8197
where
82-
isUnboundedWrite(bw) and
83-
tainted2(bw.getASource(), inputSource, inputCause)
84-
select bw, "This '" + bw.getBWDesc() + "' with input from $@ may overflow the destination.",
85-
inputSource, inputCause
98+
taintedWithPath(inputSource, tainted, sourceNode, sinkNode) and
99+
unboundedWriteSource(tainted, bw)
100+
select bw, sourceNode, sinkNode,
101+
"This '" + bw.getBWDesc() + "' with input from $@ may overflow the destination.", inputSource,
102+
inputSource.toString()

‎cpp/ql/src/Security/CWE/CWE-290/AuthenticationBypass.ql‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @description Authentication by checking that the peer's address
44
* matches a known IP or web address is unsafe as it is
55
* vulnerable to spoofing attacks.
6-
* @kind problem
6+
* @kind path-problem
77
* @problem.severity warning
88
* @precision medium
99
* @id cpp/user-controlled-bypass
@@ -12,6 +12,7 @@
1212
*/
1313

1414
import semmle.code.cpp.security.TaintTracking
15+
import TaintedWithPath
1516

1617
predicate hardCodedAddressOrIP(StringLiteral txt) {
1718
exists(string s | s = txt.getValueText() |
@@ -102,16 +103,21 @@ predicate useOfHardCodedAddressOrIP(Expr use) {
102103
* untrusted input then it might be vulnerable to a spoofing
103104
* attack.
104105
*/
105-
predicate hardCodedAddressInCondition(Expr source, Expr condition) {
106-
// One of the sub-expressions of the condition is tainted.
107-
exists(Expr taintedExpr | taintedExpr.getParent+() = condition | tainted(source, taintedExpr)) and
106+
predicate hardCodedAddressInCondition(Expr subexpression, Expr condition) {
107+
subexpression = condition.getAChild+() and
108108
// One of the sub-expressions of the condition is a hard-coded
109109
// IP or web-address.
110-
exists(Expr use | use.getParent+() = condition | useOfHardCodedAddressOrIP(use)) and
110+
exists(Expr use | use = condition.getAChild+() | useOfHardCodedAddressOrIP(use)) and
111111
condition = any(IfStmt ifStmt).getCondition()
112112
}
113113

114-
from Expr source, Expr condition
115-
where hardCodedAddressInCondition(source, condition)
116-
select condition, "Untrusted input $@ might be vulnerable to a spoofing attack.", source,
117-
source.toString()
114+
class Configuration extends TaintTrackingConfiguration {
115+
override predicate isSink(Element sink) { hardCodedAddressInCondition(sink, _) }
116+
}
117+
118+
from Expr subexpression, Expr source, Expr condition, PathNode sourceNode, PathNode sinkNode
119+
where
120+
hardCodedAddressInCondition(subexpression, condition) and
121+
taintedWithPath(source, subexpression, sourceNode, sinkNode)
122+
select condition, sourceNode, sinkNode,
123+
"Untrusted input $@ might be vulnerable to a spoofing attack.", source, source.toString()

‎cpp/ql/src/Security/CWE/CWE-311/CleartextBufferWrite.ql‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* @name Cleartext storage of sensitive information in buffer
33
* @description Storing sensitive information in cleartext can expose it
44
* to an attacker.
5-
* @kind problem
5+
* @kind path-problem
66
* @problem.severity warning
77
* @precision medium
88
* @id cpp/cleartext-storage-buffer
@@ -14,12 +14,20 @@ import cpp
1414
import semmle.code.cpp.security.BufferWrite
1515
import semmle.code.cpp.security.TaintTracking
1616
import semmle.code.cpp.security.SensitiveExprs
17+
import TaintedWithPath
1718

18-
from BufferWrite w, Expr taintedArg, Expr taintSource, string taintCause, SensitiveExpr dest
19+
class Configuration extends TaintTrackingConfiguration {
20+
override predicate isSink(Element tainted) { exists(BufferWrite w | w.getASource() = tainted) }
21+
}
22+
23+
from
24+
BufferWrite w, Expr taintedArg, Expr taintSource, PathNode sourceNode, PathNode sinkNode,
25+
string taintCause, SensitiveExpr dest
1926
where
20-
tainted(taintSource, taintedArg) and
27+
taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and
2128
isUserInput(taintSource, taintCause) and
2229
w.getASource() = taintedArg and
2330
dest = w.getDest()
24-
select w, "This write into buffer '" + dest.toString() + "' may contain unencrypted data from $@",
31+
select w, sourceNode, sinkNode,
32+
"This write into buffer '" + dest.toString() + "' may contain unencrypted data from $@",
2533
taintSource, "user input (" + taintCause + ")"

‎cpp/ql/src/Security/CWE/CWE-807/TaintedCondition.ql‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @description Using untrusted inputs in a statement that makes a
44
* security decision makes code vulnerable to
55
* attack.
6-
* @kind problem
6+
* @kind path-problem
77
* @problem.severity warning
88
* @precision medium
99
* @id cpp/tainted-permissions-check
@@ -12,22 +12,29 @@
1212
*/
1313

1414
import semmle.code.cpp.security.TaintTracking
15+
import TaintedWithPath
1516

16-
/**
17-
* Holds if there is an 'if' statement whose condition `condition`
18-
* is influenced by tainted data `source`, and the body contains
19-
* `raise` which escalates privilege.
20-
*/
21-
predicate cwe807violation(Expr source, Expr condition, Expr raise) {
22-
tainted(source, condition) and
17+
predicate sensitiveCondition(Expr condition, Expr raise) {
2318
raisesPrivilege(raise) and
2419
exists(IfStmt ifstmt |
2520
ifstmt.getCondition() = condition and
2621
raise.getEnclosingStmt().getParentStmt*() = ifstmt
2722
)
2823
}
2924

30-
from Expr source, Expr condition, Expr raise
31-
where cwe807violation(source, condition, raise)
32-
select condition, "Reliance on untrusted input $@ to raise privilege at $@", source,
33-
source.toString(), raise, raise.toString()
25+
class Configuration extends TaintTrackingConfiguration {
26+
override predicate isSink(Element tainted) { sensitiveCondition(tainted, _) }
27+
}
28+
29+
/*
30+
* Produce an alert if there is an 'if' statement whose condition `condition`
31+
* is influenced by tainted data `source`, and the body contains
32+
* `raise` which escalates privilege.
33+
*/
34+
35+
from Expr source, Expr condition, Expr raise, PathNode sourceNode, PathNode sinkNode
36+
where
37+
taintedWithPath(source, condition, sourceNode, sinkNode) and
38+
sensitiveCondition(condition, raise)
39+
select condition, sourceNode, sinkNode, "Reliance on untrusted input $@ to raise privilege at $@",
40+
source, source.toString(), raise, raise.toString()

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

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,30 @@
1-
| search.c:17:8:17:12 | query | Cross-site scripting vulnerability due to $@. | search.c:41:21:41:26 | call to getenv | this query data |
2-
| search.c:23:39:23:43 | query | Cross-site scripting vulnerability due to $@. | search.c:41:21:41:26 | call to getenv | this query data |
1+
edges
2+
| search.c:14:24:14:28 | query | search.c:17:8:17:12 | (const char *)... |
3+
| search.c:14:24:14:28 | query | search.c:17:8:17:12 | query |
4+
| search.c:14:24:14:28 | query | search.c:17:8:17:12 | query |
5+
| search.c:22:24:22:28 | query | search.c:23:39:23:43 | query |
6+
| search.c:22:24:22:28 | query | search.c:23:39:23:43 | query |
7+
| search.c:41:21:41:26 | call to getenv | search.c:45:17:45:25 | raw_query |
8+
| search.c:41:21:41:26 | call to getenv | search.c:45:17:45:25 | raw_query |
9+
| search.c:41:21:41:26 | call to getenv | search.c:47:17:47:25 | raw_query |
10+
| search.c:41:21:41:26 | call to getenv | search.c:47:17:47:25 | raw_query |
11+
| search.c:45:17:45:25 | raw_query | search.c:14:24:14:28 | query |
12+
| search.c:47:17:47:25 | raw_query | search.c:22:24:22:28 | query |
13+
nodes
14+
| search.c:14:24:14:28 | query | semmle.label | query |
15+
| search.c:17:8:17:12 | (const char *)... | semmle.label | (const char *)... |
16+
| search.c:17:8:17:12 | (const char *)... | semmle.label | (const char *)... |
17+
| search.c:17:8:17:12 | query | semmle.label | query |
18+
| search.c:17:8:17:12 | query | semmle.label | query |
19+
| search.c:17:8:17:12 | query | semmle.label | query |
20+
| search.c:22:24:22:28 | query | semmle.label | query |
21+
| search.c:23:39:23:43 | query | semmle.label | query |
22+
| search.c:23:39:23:43 | query | semmle.label | query |
23+
| search.c:23:39:23:43 | query | semmle.label | query |
24+
| search.c:41:21:41:26 | call to getenv | semmle.label | call to getenv |
25+
| search.c:41:21:41:26 | call to getenv | semmle.label | call to getenv |
26+
| search.c:45:17:45:25 | raw_query | semmle.label | raw_query |
27+
| search.c:47:17:47:25 | raw_query | semmle.label | raw_query |
28+
#select
29+
| search.c:17:8:17:12 | query | search.c:41:21:41:26 | call to getenv | search.c:17:8:17:12 | query | Cross-site scripting vulnerability due to $@. | search.c:41:21:41:26 | call to getenv | this query data |
30+
| search.c:23:39:23:43 | query | search.c:41:21:41:26 | call to getenv | search.c:23:39:23:43 | query | Cross-site scripting vulnerability due to $@. | search.c:41:21:41:26 | call to getenv | this query data |

‎cpp/ql/test/query-tests/Security/CWE/CWE-114/semmle/UncontrolledProcessOperation/UncontrolledProcessOperation.expected‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,25 @@
1-
| test.cpp:26:10:26:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:42:18:42:23 | call to getenv | call to getenv |
2-
| test.cpp:31:10:31:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:43:18:43:23 | call to getenv | call to getenv |
1+
edges
2+
| test.cpp:24:30:24:36 | command | test.cpp:26:10:26:16 | command |
3+
| test.cpp:24:30:24:36 | command | test.cpp:26:10:26:16 | command |
4+
| test.cpp:29:30:29:36 | command | test.cpp:31:10:31:16 | command |
5+
| test.cpp:29:30:29:36 | command | test.cpp:31:10:31:16 | command |
6+
| test.cpp:42:18:42:23 | call to getenv | test.cpp:24:30:24:36 | command |
7+
| test.cpp:42:18:42:34 | (const char *)... | test.cpp:24:30:24:36 | command |
8+
| test.cpp:43:18:43:23 | call to getenv | test.cpp:29:30:29:36 | command |
9+
| test.cpp:43:18:43:34 | (const char *)... | test.cpp:29:30:29:36 | command |
10+
nodes
11+
| test.cpp:24:30:24:36 | command | semmle.label | command |
12+
| test.cpp:26:10:26:16 | command | semmle.label | command |
13+
| test.cpp:26:10:26:16 | command | semmle.label | command |
14+
| test.cpp:26:10:26:16 | command | semmle.label | command |
15+
| test.cpp:29:30:29:36 | command | semmle.label | command |
16+
| test.cpp:31:10:31:16 | command | semmle.label | command |
17+
| test.cpp:31:10:31:16 | command | semmle.label | command |
18+
| test.cpp:31:10:31:16 | command | semmle.label | command |
19+
| test.cpp:42:18:42:23 | call to getenv | semmle.label | call to getenv |
20+
| test.cpp:42:18:42:34 | (const char *)... | semmle.label | (const char *)... |
21+
| test.cpp:43:18:43:23 | call to getenv | semmle.label | call to getenv |
22+
| test.cpp:43:18:43:34 | (const char *)... | semmle.label | (const char *)... |
23+
#select
24+
| test.cpp:26:10:26:16 | command | test.cpp:42:18:42:23 | call to getenv | test.cpp:26:10:26:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:42:18:42:23 | call to getenv | call to getenv |
25+
| test.cpp:31:10:31:16 | command | test.cpp:43:18:43:23 | call to getenv | test.cpp:31:10:31:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:43:18:43:23 | call to getenv | call to getenv |

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
edges
2+
nodes
3+
#select

0 commit comments

Comments
 (0)