Skip to content

Commit e0d42d1

Browse files
ex0dus-0xex0dus-0x
committed
Fix up tests, and enhance query with better isBarrier
1 parent 4d28876 commit e0d42d1

8 files changed

Lines changed: 89 additions & 149 deletions

File tree

‎cpp/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql‎

Lines changed: 64 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -15,110 +15,92 @@ import cpp
1515
private import semmle.code.cpp.ir.dataflow.TaintTracking
1616
private import semmle.code.cpp.security.FlowSources
1717
private import semmle.code.cpp.security.CommandExecution
18+
private import semmle.code.cpp.controlflow.IRGuards
1819

1920
/**
20-
* Generic case: invoke protocol handling through OS's protocol handling utilities. This aligns with CVE-2022-43550.
21+
* Holds when `call` invokes a system function (system, popen, exec*) with a command string
22+
* that contains a URL protocol handler, `sink` is the argument carrying the URL, and
23+
* `handlerType` describes which handler is invoked.
2124
*/
22-
class ShellProtocolHandler extends SystemFunction {
23-
ShellProtocolHandler() {
24-
// Check if any calls to this function contain protocol handler invocations
25-
exists(FunctionCall call |
26-
call.getTarget() = this and
27-
exists(Expr arg |
28-
arg = call.getArgument(0).getAChild*() and
29-
exists(StringLiteral sl | sl = arg or sl.getParent*() = arg |
30-
sl.getValue()
31-
.regexpMatch("(?i).*(rundll32.*url\\.dll.*FileProtocolHandler|xdg-open|\\bopen\\b).*")
32-
)
33-
)
25+
predicate isShellProtocolHandlerSink(FunctionCall call, Expr sink, string handlerType) {
26+
call.getTarget() instanceof SystemFunction and
27+
exists(StringLiteral sl |
28+
sl.getParent*() = call.getArgument(0) and
29+
(
30+
sl.getValue().regexpMatch("(?i).*rundll32.*url\\.dll.*FileProtocolHandler.*") and
31+
handlerType = "rundll32 url.dll,FileProtocolHandler"
32+
or
33+
sl.getValue().regexpMatch("(?i).*xdg-open.*") and
34+
handlerType = "xdg-open"
35+
or
36+
sl.getValue().regexpMatch("(?i).*\\bopen\\b.*") and
37+
handlerType = "open"
3438
)
35-
}
36-
37-
string getHandlerType() {
38-
exists(FunctionCall call, StringLiteral sl |
39-
call.getTarget() = this and
40-
sl.getParent*() = call.getArgument(0) and
41-
(
42-
sl.getValue().regexpMatch("(?i).*rundll32.*url\\.dll.*FileProtocolHandler.*") and
43-
result = "rundll32 url.dll,FileProtocolHandler"
44-
or
45-
sl.getValue().regexpMatch("(?i).*xdg-open.*") and
46-
result = "xdg-open"
47-
or
48-
sl.getValue().regexpMatch("(?i).*\\bopen\\b.*") and
49-
result = "open"
50-
)
51-
)
52-
}
39+
) and
40+
sink = call.getArgument(0)
5341
}
5442

5543
/**
56-
* Qt's QDesktopServices::openUrl method
44+
* Holds when `call` invokes QDesktopServices::openUrl and `sink` is the URL argument.
5745
*/
58-
class QtProtocolHandler extends FunctionCall {
59-
QtProtocolHandler() { this.getTarget().hasQualifiedName("QDesktopServices", "openUrl") }
46+
predicate isQtProtocolHandlerSink(FunctionCall call, Expr sink) {
47+
call.getTarget().hasQualifiedName("", "QDesktopServices", "openUrl") and
48+
sink = call.getArgument(0)
49+
}
6050

61-
Expr getUrlArgument() { result = this.getArgument(0) }
51+
/**
52+
* Gets the sink expression for a given DataFlow::Node matching either Qt or shell handler sinks.
53+
*/
54+
Expr getSinkExpr(DataFlow::Node sink) {
55+
result = sink.asExpr() or
56+
result = sink.asIndirectExpr()
6257
}
6358

6459
/**
65-
* A sanitizer node that represents URL scheme validation
60+
* Holds when `g` is a guard condition that validates the URL scheme of expression `e`.
61+
* Flow is considered safe on the `branch` edge.
6662
*/
67-
class UrlSchemeValidationSanitizer extends DataFlow::Node {
68-
UrlSchemeValidationSanitizer() {
69-
exists(FunctionCall fc |
70-
fc = this.asExpr() and
71-
(
72-
// String comparison on the untrusted URL
73-
fc.getTarget().getName() =
74-
[
75-
"strcmp", "strncmp", "strcasecmp", "strncasecmp", "strstr", "strcasestr", "_stricmp",
76-
"_strnicmp"
77-
]
78-
or
79-
// Qt QUrl::scheme() comparison - QUrl::scheme() returns QString
80-
// Pattern: url.scheme() == "http" or url.scheme() == "https"
81-
exists(FunctionCall schemeCall |
82-
schemeCall.getTarget().hasQualifiedName("QUrl", "scheme") and
83-
(
84-
// Direct comparison
85-
fc.getTarget().hasName(["operator==", "operator!="]) and
86-
fc.getAnArgument() = schemeCall
87-
or
88-
// QString comparison methods
89-
fc = schemeCall and
90-
exists(FunctionCall qstringCmp |
91-
qstringCmp.getQualifier() = schemeCall and
92-
qstringCmp.getTarget().hasQualifiedName("QString", ["compare", "operator=="])
93-
)
94-
)
95-
)
96-
or
97-
// Qt QString startsWith check for direct URL strings
98-
fc.getTarget().hasQualifiedName("QString", "startsWith")
99-
)
63+
predicate urlSchemeGuardChecks(IRGuardCondition g, Expr e, boolean branch) {
64+
branch = [true, false] and
65+
exists(FunctionCall fc | g.getUnconvertedResultExpression().getAChild*() = fc |
66+
// C string comparison on the URL (strcmp, strncmp, etc.)
67+
fc.getTarget().getName() =
68+
[
69+
"strcmp", "strncmp", "strcasecmp", "strncasecmp", "strstr", "strcasestr", "_stricmp",
70+
"_strnicmp"
71+
] and
72+
e = fc.getAnArgument()
73+
or
74+
// Qt QString::startsWith check
75+
fc.getTarget().hasQualifiedName("", "QString", "startsWith") and
76+
e = fc.getQualifier()
77+
or
78+
// Qt QUrl::scheme() comparison
79+
exists(FunctionCall schemeCall |
80+
schemeCall.getTarget().hasQualifiedName("", "QUrl", "scheme") and
81+
fc.getAnArgument() = schemeCall and
82+
e = schemeCall.getQualifier()
10083
)
101-
}
84+
)
10285
}
10386

10487
/**
105-
* Configuration for tracking untrusted data to protocol handler invocations
88+
* Configuration for tracking untrusted data to protocol handler invocations.
10689
*/
10790
module PotentiallyUnguardedProtocolHandlerConfig implements DataFlow::ConfigSig {
10891
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
10992

11093
predicate isSink(DataFlow::Node sink) {
111-
// QDesktopServices::openUrl()
112-
exists(QtProtocolHandler call | sink.asExpr() = call.getUrlArgument())
94+
isQtProtocolHandlerSink(_, getSinkExpr(sink))
11395
or
114-
// Shell protocol handlers (rundll32, xdg-open, open) via system()/popen()/exec*()
115-
exists(FunctionCall call |
116-
call.getTarget() instanceof ShellProtocolHandler and
117-
sink.asExpr() = call.getArgument(0)
118-
)
96+
isShellProtocolHandlerSink(_, getSinkExpr(sink), _)
11997
}
12098

121-
predicate isBarrier(DataFlow::Node node) { node instanceof UrlSchemeValidationSanitizer }
99+
predicate isBarrier(DataFlow::Node node) {
100+
node = DataFlow::BarrierGuard<urlSchemeGuardChecks/3>::getABarrierNode()
101+
or
102+
node = DataFlow::BarrierGuard<urlSchemeGuardChecks/3>::getAnIndirectBarrierNode()
103+
}
122104
}
123105

124106
module PotentiallyUnguardedProtocolHandlerFlow =
@@ -132,17 +114,10 @@ from
132114
where
133115
PotentiallyUnguardedProtocolHandlerFlow::flowPath(source, sink) and
134116
(
135-
exists(QtProtocolHandler qtCall |
136-
call = qtCall and
137-
sink.getNode().asExpr() = qtCall.getUrlArgument() and
138-
callType = "QDesktopServices::openUrl()"
139-
)
117+
isQtProtocolHandlerSink(call, getSinkExpr(sink.getNode())) and
118+
callType = "QDesktopServices::openUrl()"
140119
or
141-
exists(ShellProtocolHandler shellFunc |
142-
call.getTarget() = shellFunc and
143-
sink.getNode().asExpr() = call.getArgument(0) and
144-
callType = shellFunc.getHandlerType()
145-
)
120+
isShellProtocolHandlerSink(call, getSinkExpr(sink.getNode()), callType)
146121
)
147122
select call, source, sink,
148123
callType + " is called with untrusted input from $@ without proper URL scheme validation.",

‎cpp/test/include/libc/stdint.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
typedef unsigned char uint8_t;
77
typedef unsigned short uint16_t;
88
typedef unsigned int uint32_t;
9+
typedef unsigned long size_t;
10+
typedef long ssize_t;
911

1012
#endif
1113
#else // --- else USE_HEADERS

‎cpp/test/include/libc/stdlib.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
extern "C" {
88
#endif
99

10+
extern int system(const char *);
11+
1012
extern void *reallocf(void *, unsigned long);
1113

1214
int rand(void) {

‎cpp/test/include/libc/string_stubs.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ extern int wprintf(const wchar_t * format, ...);
2828
extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2);
2929
extern void perror(const char *s);
3030
extern int puts(const char *s);
31+
extern int strcmp(const char *, const char *);
32+
extern int strncmp(const char *, const char *, size_t);
3133

3234
extern void openlog(const char*, int, int);
3335
extern void syslog(int, const char*, ...);

‎cpp/test/include/libc/unistd.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
extern "C" {
88
#endif
99

10+
ssize_t read(int, void *, size_t);
11+
1012
void _exit(int);
1113

1214
#ifdef __cplusplus

‎cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,12 @@
11
edges
2+
| PotentiallyUnguardedProtocolHandler.cpp:27:11:27:13 | read output argument | PotentiallyUnguardedProtocolHandler.cpp:28:29:28:31 | *buf | provenance | |
3+
| PotentiallyUnguardedProtocolHandler.cpp:39:11:39:13 | read output argument | PotentiallyUnguardedProtocolHandler.cpp:42:31:42:33 | *buf | provenance | |
24
nodes
5+
| PotentiallyUnguardedProtocolHandler.cpp:27:11:27:13 | read output argument | semmle.label | read output argument |
6+
| PotentiallyUnguardedProtocolHandler.cpp:28:29:28:31 | *buf | semmle.label | *buf |
7+
| PotentiallyUnguardedProtocolHandler.cpp:39:11:39:13 | read output argument | semmle.label | read output argument |
8+
| PotentiallyUnguardedProtocolHandler.cpp:42:31:42:33 | *buf | semmle.label | *buf |
39
subpaths
410
#select
11+
| PotentiallyUnguardedProtocolHandler.cpp:28:3:28:27 | call to openUrl | PotentiallyUnguardedProtocolHandler.cpp:27:11:27:13 | read output argument | PotentiallyUnguardedProtocolHandler.cpp:28:29:28:31 | *buf | QDesktopServices::openUrl() is called with untrusted input from $@ without proper URL scheme validation. | PotentiallyUnguardedProtocolHandler.cpp:27:11:27:13 | read output argument | this source |
12+
| PotentiallyUnguardedProtocolHandler.cpp:42:5:42:29 | call to openUrl | PotentiallyUnguardedProtocolHandler.cpp:39:11:39:13 | read output argument | PotentiallyUnguardedProtocolHandler.cpp:42:31:42:33 | *buf | QDesktopServices::openUrl() is called with untrusted input from $@ without proper URL scheme validation. | PotentiallyUnguardedProtocolHandler.cpp:39:11:39:13 | read output argument | this source |

‎cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/test.cpp‎

Lines changed: 0 additions & 60 deletions
This file was deleted.

‎java/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qhelp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,15 @@
1414
they come from untrusted sources.
1515
</p>
1616
</recommendation>
17+
<example>
18+
<sample src="PotentiallyUnguardedProtocolHandler.java" />
19+
<p>
20+
In the bad example, an untrusted URL is passed directly to <code>Desktop.browse()</code>
21+
without any scheme validation.
22+
In the good example, the URL scheme is checked with <code>getScheme()</code> before
23+
invoking the handler.
24+
</p>
25+
</example>
1726
<references>
1827
<li> Positive Security: <a
1928
href="https://positive.security/blog/url-open-rce">Allow

0 commit comments

Comments
 (0)