Skip to content

Commit 354a12c

Browse files
MathiasVPMathiasVP
committed
C++: Fix queries. Since there's no longer indirect -> direct flow in
taint-tracking we need to make sure the affected sink definitions also handle indirect flow.
1 parent 1db24dd commit 354a12c

17 files changed

+443
-377
lines changed

‎cpp/ql/src/Critical/OverflowDestination.ql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class OverflowDestinationConfig extends TaintTracking::Configuration {
7171

7272
override predicate isSource(DataFlow::Node source) { source instanceof FlowSource }
7373

74-
override predicate isSink(DataFlow::Node sink) { sourceSized(_, sink.asConvertedExpr()) }
74+
override predicate isSink(DataFlow::Node sink) { sourceSized(_, sink.asIndirectConvertedExpr()) }
7575

7676
override predicate isSanitizer(DataFlow::Node node) {
7777
exists(Variable checkedVar |
@@ -91,6 +91,6 @@ from
9191
DataFlow::PathNode sink
9292
where
9393
conf.hasFlowPath(source, sink) and
94-
sourceSized(fc, sink.getNode().asConvertedExpr())
94+
sourceSized(fc, sink.getNode().asIndirectConvertedExpr())
9595
select fc, source, sink,
9696
"To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size."

‎cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,12 @@ predicate underscoreMacro(Expr e) {
5757
*/
5858
predicate cannotContainString(Type t, boolean isIndirect) {
5959
isIndirect = false and
60-
(
61-
t.getUnspecifiedType() instanceof BuiltInType or
62-
t.getUnspecifiedType() instanceof IntegralOrEnumType
60+
exists(Type unspecified |
61+
unspecified = t.getUnspecifiedType() and
62+
not unspecified instanceof UnknownType
63+
|
64+
unspecified instanceof BuiltInType or
65+
unspecified instanceof IntegralOrEnumType
6366
)
6467
}
6568

@@ -124,6 +127,11 @@ predicate isSanitizerNode(DataFlow::Node node) {
124127
cannotContainString(node.getType(), false)
125128
}
126129

130+
predicate isSinkImpl(DataFlow::Node sink, Expr formatString) {
131+
[sink.asExpr(), sink.asIndirectExpr()] = formatString and
132+
exists(FormattingFunctionCall fc | formatString = fc.getArgument(fc.getFormatParameterIndex()))
133+
}
134+
127135
class NonConstFlow extends TaintTracking::Configuration {
128136
NonConstFlow() { this = "NonConstFlow" }
129137

@@ -135,9 +143,7 @@ class NonConstFlow extends TaintTracking::Configuration {
135143
)
136144
}
137145

138-
override predicate isSink(DataFlow::Node sink) {
139-
exists(FormattingFunctionCall fc | sink.asExpr() = fc.getArgument(fc.getFormatParameterIndex()))
140-
}
146+
override predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _) }
141147

142148
override predicate isSanitizer(DataFlow::Node node) { isSanitizerNode(node) }
143149
}
@@ -147,7 +153,7 @@ where
147153
call.getArgument(call.getFormatParameterIndex()) = formatString and
148154
exists(NonConstFlow cf, DataFlow::Node sink |
149155
cf.hasFlowTo(sink) and
150-
sink.asExpr() = formatString
156+
isSinkImpl(sink, formatString)
151157
)
152158
select formatString,
153159
"The format string argument to " + call.getTarget().getName() +

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,11 @@ class ToBufferConfiguration extends TaintTracking::Configuration {
4848
node.asExpr().getUnspecifiedType() instanceof IntegralType
4949
}
5050

51-
override predicate isSink(DataFlow::Node sink) {
52-
exists(SensitiveBufferWrite w | w.getASource() = sink.asExpr())
53-
}
51+
override predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _) }
52+
}
53+
54+
predicate isSinkImpl(DataFlow::Node sink, SensitiveBufferWrite w) {
55+
w.getASource() = sink.asIndirectExpr()
5456
}
5557

5658
from
@@ -59,7 +61,7 @@ from
5961
where
6062
config.hasFlowPath(sourceNode, sinkNode) and
6163
sourceNode.getNode() = source and
62-
w.getASource() = sinkNode.getNode().asExpr()
64+
isSinkImpl(sinkNode.getNode(), w)
6365
select w, sourceNode, sinkNode,
6466
"This write into buffer '" + w.getDest().toString() + "' may contain unencrypted data from $@.",
6567
source, "user input (" + source.getSourceType() + ")"

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

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,32 @@ import DataFlow::PathGraph
2626
class FromSensitiveConfiguration extends TaintTracking::Configuration {
2727
FromSensitiveConfiguration() { this = "FromSensitiveConfiguration" }
2828

29-
override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SensitiveExpr }
29+
override predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) }
3030

31-
override predicate isSink(DataFlow::Node sink) { any(FileWrite w).getASource() = sink.asExpr() }
31+
override predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _, _) }
3232

3333
override predicate isSanitizer(DataFlow::Node node) {
3434
node.asExpr().getUnspecifiedType() instanceof IntegralType
3535
}
3636
}
3737

38+
predicate isSinkImpl(DataFlow::Node sink, FileWrite w, Expr dest) {
39+
exists(Expr e |
40+
e = [sink.asExpr(), sink.asIndirectExpr()] and
41+
w.getASource() = e and
42+
dest = w.getDest() and
43+
// ignore things written with other conversion characters
44+
not exists(string convChar | convChar = w.getSourceConvChar(e) | not convChar = ["s", "S"]) and
45+
// exclude calls with standard streams
46+
not dest.(VariableAccess).getTarget().getName() = ["stdin", "stdout", "stderr"]
47+
)
48+
}
49+
50+
predicate isSourceImpl(DataFlow::Node source, SensitiveExpr sensitive) {
51+
not isFileName(globalValueNumber(sensitive)) and // file names are not passwords
52+
source.asExpr() = sensitive
53+
}
54+
3855
/**
3956
* An operation on a filename.
4057
*/
@@ -61,17 +78,12 @@ predicate isFileName(GVN gvn) {
6178
}
6279

6380
from
64-
FromSensitiveConfiguration config, SensitiveExpr source, DataFlow::PathNode sourceNode, Expr mid,
81+
FromSensitiveConfiguration config, SensitiveExpr source, DataFlow::PathNode sourceNode,
6582
DataFlow::PathNode midNode, FileWrite w, Expr dest
6683
where
6784
config.hasFlowPath(sourceNode, midNode) and
68-
sourceNode.getNode().asExpr() = source and
69-
midNode.getNode().asExpr() = mid and
70-
mid = w.getASource() and
71-
dest = w.getDest() and
72-
not dest.(VariableAccess).getTarget().getName() = ["stdin", "stdout", "stderr"] and // exclude calls with standard streams
73-
not isFileName(globalValueNumber(source)) and // file names are not passwords
74-
not exists(string convChar | convChar = w.getSourceConvChar(mid) | not convChar = ["s", "S"]) // ignore things written with other conversion characters
85+
isSourceImpl(sourceNode.getNode(), source) and
86+
isSinkImpl(midNode.getNode(), w, dest)
7587
select w, sourceNode, midNode,
7688
"This write into file '" + dest.toString() + "' may contain unencrypted data from $@.", source,
7789
"this source."

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ class Encrypted extends Expr {
208208
* operation `nsr`.
209209
*/
210210
predicate isSinkSendRecv(DataFlow::Node sink, NetworkSendRecv nsr) {
211-
sink.asConvertedExpr() = nsr.getDataExpr().getFullyConverted()
211+
[sink.asIndirectConvertedExpr(), sink.asConvertedExpr()] = nsr.getDataExpr().getFullyConverted()
212212
}
213213

214214
/**

‎cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,11 @@ class HttpStringToUrlOpenConfig extends TaintTracking::Configuration {
5959

6060
override predicate isSource(DataFlow::Node src) {
6161
// Sources are strings containing an HTTP URL not in a private domain.
62-
src.asExpr() instanceof HttpStringLiteral and
62+
src.asIndirectExpr() instanceof HttpStringLiteral and
6363
// block taint starting at `strstr`, which is likely testing an existing URL, rather than constructing an HTTP URL.
6464
not exists(FunctionCall fc |
6565
fc.getTarget().getName() = ["strstr", "strcasestr"] and
66-
fc.getArgument(1) = globalValueNumber(src.asExpr()).getAnExpr()
66+
fc.getArgument(1) = globalValueNumber(src.asIndirectExpr()).getAnExpr()
6767
)
6868
}
6969

@@ -77,16 +77,16 @@ class HttpStringToUrlOpenConfig extends TaintTracking::Configuration {
7777
"system", "gethostbyname", "gethostbyname2", "gethostbyname_r", "getaddrinfo",
7878
"X509_load_http", "X509_CRL_load_http"
7979
]) and
80-
sink.asExpr() = fc.getArgument(0)
80+
sink.asIndirectExpr() = fc.getArgument(0)
8181
or
8282
fc.getTarget().hasGlobalOrStdName(["send", "URLDownloadToFile", "URLDownloadToCacheFile"]) and
83-
sink.asExpr() = fc.getArgument(1)
83+
sink.asIndirectExpr() = fc.getArgument(1)
8484
or
8585
fc.getTarget().hasGlobalOrStdName(["curl_easy_setopt", "getnameinfo"]) and
86-
sink.asExpr() = fc.getArgument(2)
86+
sink.asIndirectExpr() = fc.getArgument(2)
8787
or
8888
fc.getTarget().hasGlobalOrStdName(["ShellExecute", "ShellExecuteA", "ShellExecuteW"]) and
89-
sink.asExpr() = fc.getArgument(3)
89+
sink.asIndirectExpr() = fc.getArgument(3)
9090
)
9191
}
9292
}
@@ -96,5 +96,5 @@ from
9696
HttpStringLiteral str
9797
where
9898
config.hasFlowPath(source, sink) and
99-
str = source.getNode().asExpr()
99+
str = source.getNode().asIndirectExpr()
100100
select str, source, sink, "This URL may be constructed with the HTTP protocol."

‎cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ class ExposedSystemDataConfiguration extends TaintTracking::Configuration {
2727
exists(FunctionCall fc, FunctionInput input, int arg |
2828
fc.getTarget().(RemoteFlowSinkFunction).hasRemoteFlowSink(input, _) and
2929
input.isParameterDeref(arg) and
30-
fc.getArgument(arg).getAChild*() = sink.asExpr()
30+
fc.getArgument(arg).getAChild*() = sink.asIndirectExpr()
3131
)
3232
}
3333
}
@@ -39,7 +39,7 @@ where
3939
DataFlow::Node alt // remove duplicate results on conversions
4040
|
4141
config.hasFlow(source.getNode(), alt) and
42-
alt.asConvertedExpr() = sink.getNode().asExpr() and
42+
alt.asConvertedExpr() = sink.getNode().asIndirectExpr() and
4343
alt != sink.getNode()
4444
)
4545
select sink, source, sink, "This operation exposes system data from $@.", source,

‎cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class PotentiallyExposedSystemDataConfiguration extends TaintTracking::Configura
3939
}
4040

4141
override predicate isSink(DataFlow::Node sink) {
42-
exists(OutputWrite ow | ow.getASource().getAChild*() = sink.asExpr())
42+
exists(OutputWrite ow | ow.getASource().getAChild*() = sink.asIndirectExpr())
4343
}
4444
}
4545

‎cpp/ql/src/Security/CWE/CWE-497/SystemData.qll

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ class EnvData extends SystemData {
3434
.regexpMatch(".*(user|host|admin|root|home|path|http|ssl|snmp|sock|port|proxy|pass|token|crypt|key).*")
3535
}
3636

37-
override DataFlow::Node getAnExpr() { result.asConvertedExpr() = this }
37+
override DataFlow::Node getAnExpr() { result.asIndirectConvertedExpr() = this }
3838

3939
override predicate isSensitive() {
4040
this.(EnvironmentRead)
@@ -50,7 +50,7 @@ class EnvData extends SystemData {
5050
class SqlClientInfo extends SystemData {
5151
SqlClientInfo() { this.(FunctionCall).getTarget().hasName("mysql_get_client_info") }
5252

53-
override DataFlow::Node getAnExpr() { result.asConvertedExpr() = this }
53+
override DataFlow::Node getAnExpr() { result.asIndirectConvertedExpr() = this }
5454

5555
override predicate isSensitive() { any() }
5656
}
@@ -72,7 +72,7 @@ private predicate sqlConnectInfo(FunctionCall source, Expr use) {
7272
class SqlConnectInfo extends SystemData {
7373
SqlConnectInfo() { sqlConnectInfo(this, _) }
7474

75-
override DataFlow::Node getAnExpr() { sqlConnectInfo(this, result.asConvertedExpr()) }
75+
override DataFlow::Node getAnExpr() { sqlConnectInfo(this, result.asExpr()) }
7676

7777
override predicate isSensitive() { any() }
7878
}
@@ -114,7 +114,7 @@ private predicate posixPWInfo(FunctionCall source, DataFlow::Node use) {
114114
source
115115
.getTarget()
116116
.hasName(["getpwnam", "getpwuid", "getpwent", "getgrnam", "getgrgid", "getgrent"]) and
117-
use.asConvertedExpr() = source
117+
use.asIndirectExpr() = source
118118
or
119119
// int getpwnam_r(const char *name, struct passwd *pwd,
120120
// char *buf, size_t buflen, struct passwd **result);
@@ -126,7 +126,7 @@ private predicate posixPWInfo(FunctionCall source, DataFlow::Node use) {
126126
// char *buf, size_t buflen, struct group **result);
127127
source.getTarget().hasName(["getpwnam_r", "getpwuid_r", "getgrgid_r", "getgrnam_r"]) and
128128
(
129-
use.asConvertedExpr() = source.getArgument([1, 2]) or
129+
use.asExpr() = source.getArgument([1, 2]) or
130130
use.asDefiningArgument() = source.getArgument(4)
131131
)
132132
or
@@ -136,7 +136,7 @@ private predicate posixPWInfo(FunctionCall source, DataFlow::Node use) {
136136
// size_t buflen, struct group **gbufp);
137137
source.getTarget().hasName(["getpwent_r", "getgrent_r"]) and
138138
(
139-
use.asConvertedExpr() = source.getArgument([0, 1]) or
139+
use.asExpr() = source.getArgument([0, 1]) or
140140
use.asDefiningArgument() = source.getArgument(3)
141141
)
142142
}
@@ -155,7 +155,7 @@ class PosixPWInfo extends SystemData {
155155
private predicate windowsSystemInfo(FunctionCall source, DataFlow::Node use) {
156156
// DWORD WINAPI GetVersion(void);
157157
source.getTarget().hasGlobalName("GetVersion") and
158-
use.asConvertedExpr() = source
158+
use.asExpr() = source
159159
or
160160
// BOOL WINAPI GetVersionEx(_Inout_ LPOSVERSIONINFO lpVersionInfo);
161161
// void WINAPI GetSystemInfo(_Out_ LPSYSTEM_INFO lpSystemInfo);
@@ -236,7 +236,7 @@ class WindowsFolderPath extends SystemData {
236236
override DataFlow::Node getAnExpr() { windowsFolderPath(this, result.asDefiningArgument()) }
237237
}
238238

239-
private predicate logonUser(FunctionCall source, VariableAccess use) {
239+
private predicate logonUser(FunctionCall source, Expr use) {
240240
source.getTarget().hasGlobalName(["LogonUser", "LogonUserW", "LogonUserA"]) and
241241
use = source.getAnArgument()
242242
}
@@ -247,7 +247,7 @@ private predicate logonUser(FunctionCall source, VariableAccess use) {
247247
class LogonUser extends SystemData {
248248
LogonUser() { logonUser(this, _) }
249249

250-
override DataFlow::Node getAnExpr() { logonUser(this, result.asConvertedExpr()) }
250+
override DataFlow::Node getAnExpr() { logonUser(this, result.asIndirectExpr()) }
251251

252252
override predicate isSensitive() { any() }
253253
}

0 commit comments

Comments
 (0)