diff --git a/README.md b/README.md index 8488b85..402f6ba 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,8 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif - |[Invalid string size passed to string manipulation function](./cpp/src/docs/security/CStrnFinder/CStrnFinder.md)|Finds calls to functions that take as input a string and its size as separate arguments (e.g., `strncmp`, `strncat`, ...) and the size argument is wrong|error|low| |[Iterator invalidation](./cpp/src/docs/security/IteratorInvalidation/IteratorInvalidation.md)|Modifying a container while iterating over it can invalidate iterators, leading to undefined behavior.|warning|medium| |[Missing null terminator](./cpp/src/docs/security/NoNullTerminator/NoNullTerminator.md)|This query finds incorrectly initialized strings that are passed to functions expecting null-byte-terminated strings|error|high| +|[Potentially unguarded protocol handler invocation](./cpp/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md)|Detects calls to URL protocol handlers with untrusted input that may not be properly validated for dangerous protocols|warning|medium| +|[Unsafe implicit integer conversion](./cpp/src/docs/security/UnsafeImplicitConversions/UnsafeImplicitConversions.md)|Finds implicit integer casts that may overflow or be truncated, with false positive reduction via Value Range Analysis|warning|low| ### Go @@ -77,7 +79,8 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif - | Name | Description | Severity | Precision | | --- | ----------- | :----: | :--------: | -|[Recursive functions](./java-kotlin/src/docs/security/Recursion/Recursion.md)|Detects possibly unbounded recursive calls|warning|low| +|[Potentially unguarded protocol handler invocation](./java/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md)|Detects calls to URL protocol handlers with untrusted input that may not be properly validated for dangerous protocols|warning|medium| +|[Recursive functions](./java-kotlin/src/docs/security/Recursion/Recursion.md)|Detects recursive calls|warning|low| ## Query suites diff --git a/cpp/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md b/cpp/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md new file mode 100644 index 0000000..5e56eef --- /dev/null +++ b/cpp/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md @@ -0,0 +1,87 @@ +# Potentially unguarded protocol handler invocation + +This query detects calls to URL protocol handlers with untrusted input that may not be properly validated for dangerous protocols. This vulnerability is related to CWE-939 (Improper Authorization in Handler for Custom URL Scheme) and aligns with CVE-2022-43550. + +When applications invoke protocol handlers (like `rundll32 url.dll,FileProtocolHandler` on Windows, `xdg-open` on Linux, `open` on macOS, or Qt's `QDesktopServices::openUrl()`), untrusted URLs can potentially trigger dangerous protocols such as `file://`, `smb://`, or other custom handlers that may lead to unauthorized file access, command execution, or other security issues. + +## Detected patterns + +The query identifies several common protocol handler invocation patterns: + +- **Windows**: `rundll32 url.dll,FileProtocolHandler` via system calls +- **Linux**: `xdg-open` via system calls +- **macOS**: `open` command via system calls +- **Qt applications**: `QDesktopServices::openUrl()` + +## Recommendation + +Always validate URL schemes before passing them to protocol handlers. Only allow safe protocols like `http://` and `https://`. Reject or sanitize URLs containing potentially dangerous protocols. + +## Example + +The following vulnerable code passes untrusted input directly to a protocol handler: + +```cpp +#include +#include + +void openUserUrl(const QString& userInput) { + // VULNERABLE: No validation of the URL scheme + QDesktopServices::openUrl(QUrl(userInput)); +} +``` + +An attacker could provide a URL like `file:///etc/passwd` or `smb://attacker-server/share` to access unauthorized resources. + +The corrected version validates the URL scheme before opening: + +```cpp +#include +#include + +void openUserUrl(const QString& userInput) { + QUrl url(userInput); + QString scheme = url.scheme().toLower(); + + // Only allow safe protocols + if (scheme == "http" || scheme == "https") { + QDesktopServices::openUrl(url); + } else { + // Log error or show warning to user + qWarning() << "Rejected unsafe URL scheme:" << scheme; + } +} +``` + +For system command invocations: + +```cpp +void openUserUrlViaShell(const char* userInput) { + // VULNERABLE: Untrusted input passed to xdg-open + char cmd[512]; + snprintf(cmd, sizeof(cmd), "xdg-open '%s'", userInput); + system(cmd); +} +``` + +Should be corrected to validate the scheme: + +```cpp +bool isValidScheme(const char* url) { + return (strncasecmp(url, "http://", 7) == 0 || + strncasecmp(url, "https://", 8) == 0); +} + +void openUserUrlViaShell(const char* userInput) { + if (isValidScheme(userInput)) { + char cmd[512]; + snprintf(cmd, sizeof(cmd), "xdg-open '%s'", userInput); + system(cmd); + } +} +``` + +## References + +- [CWE-939: Improper Authorization in Handler for Custom URL Scheme](https://cwe.mitre.org/data/definitions/939.html) +- [CVE-2022-43550: USB Creator has insufficiently protected credentials](https://ubuntu.com/security/CVE-2022-43550) diff --git a/cpp/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql b/cpp/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql new file mode 100644 index 0000000..60fd383 --- /dev/null +++ b/cpp/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql @@ -0,0 +1,124 @@ +/** + * @name Potentially unguarded protocol handler invocation + * @id tob/cpp/unguarded-protocol-handler + * @description Detects calls to URL protocol handlers with untrusted input that may not be properly validated for dangerous protocols + * @kind path-problem + * @tags security + * external/cwe/cwe-939 + * @precision medium + * @problem.severity warning + * @security-severity 6.5 + * @group security + */ + +import cpp +private import semmle.code.cpp.ir.dataflow.TaintTracking +private import semmle.code.cpp.security.FlowSources +private import semmle.code.cpp.security.CommandExecution +private import semmle.code.cpp.controlflow.IRGuards + +/** + * Holds when `call` invokes a system function (system, popen, exec*) with a command string + * that contains a URL protocol handler, `sink` is the argument carrying the URL, and + * `handlerType` describes which handler is invoked. + */ +predicate isShellProtocolHandlerSink(FunctionCall call, Expr sink, string handlerType) { + call.getTarget() instanceof SystemFunction and + exists(StringLiteral sl | + sl.getParent*() = call.getArgument(0) and + ( + sl.getValue().regexpMatch("(?i).*rundll32.*url\\.dll.*FileProtocolHandler.*") and + handlerType = "rundll32 url.dll,FileProtocolHandler" + or + sl.getValue().regexpMatch("(?i).*xdg-open.*") and + handlerType = "xdg-open" + or + sl.getValue().regexpMatch("(?i).*\\bopen\\b.*") and + handlerType = "open" + ) + ) and + sink = call.getArgument(0) +} + +/** + * Holds when `call` invokes QDesktopServices::openUrl and `sink` is the URL argument. + */ +predicate isQtProtocolHandlerSink(FunctionCall call, Expr sink) { + call.getTarget().hasQualifiedName("", "QDesktopServices", "openUrl") and + sink = call.getArgument(0) +} + +/** + * Gets the sink expression for a given DataFlow::Node matching either Qt or shell handler sinks. + */ +Expr getSinkExpr(DataFlow::Node sink) { + result = sink.asExpr() or + result = sink.asIndirectExpr() +} + +/** + * Holds when `g` is a guard condition that validates the URL scheme of expression `e`. + * Flow is considered safe on the `branch` edge. + */ +predicate urlSchemeGuardChecks(IRGuardCondition g, Expr e, boolean branch) { + branch = [true, false] and + exists(FunctionCall fc | g.getUnconvertedResultExpression().getAChild*() = fc | + // C string comparison on the URL (strcmp, strncmp, etc.) + fc.getTarget().getName() = + [ + "strcmp", "strncmp", "strcasecmp", "strncasecmp", "strstr", "strcasestr", "_stricmp", + "_strnicmp" + ] and + e = fc.getAnArgument() + or + // Qt QString::startsWith check + fc.getTarget().hasQualifiedName("", "QString", "startsWith") and + e = fc.getQualifier() + or + // Qt QUrl::scheme() comparison + exists(FunctionCall schemeCall | + schemeCall.getTarget().hasQualifiedName("", "QUrl", "scheme") and + fc.getAnArgument() = schemeCall and + e = schemeCall.getQualifier() + ) + ) +} + +/** + * Configuration for tracking untrusted data to protocol handler invocations. + */ +module PotentiallyUnguardedProtocolHandlerConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + predicate isSink(DataFlow::Node sink) { + isQtProtocolHandlerSink(_, getSinkExpr(sink)) + or + isShellProtocolHandlerSink(_, getSinkExpr(sink), _) + } + + predicate isBarrier(DataFlow::Node node) { + node = DataFlow::BarrierGuard::getABarrierNode() + or + node = DataFlow::BarrierGuard::getAnIndirectBarrierNode() + } +} + +module PotentiallyUnguardedProtocolHandlerFlow = + TaintTracking::Global; + +import PotentiallyUnguardedProtocolHandlerFlow::PathGraph + +from + PotentiallyUnguardedProtocolHandlerFlow::PathNode source, + PotentiallyUnguardedProtocolHandlerFlow::PathNode sink, FunctionCall call, string callType +where + PotentiallyUnguardedProtocolHandlerFlow::flowPath(source, sink) and + ( + isQtProtocolHandlerSink(call, getSinkExpr(sink.getNode())) and + callType = "QDesktopServices::openUrl()" + or + isShellProtocolHandlerSink(call, getSinkExpr(sink.getNode()), callType) + ) +select call, source, sink, + callType + " is called with untrusted input from $@ without proper URL scheme validation.", + source.getNode(), "this source" diff --git a/cpp/test/include/libc/stdint.h b/cpp/test/include/libc/stdint.h index d02afcd..12c4a23 100644 --- a/cpp/test/include/libc/stdint.h +++ b/cpp/test/include/libc/stdint.h @@ -6,6 +6,8 @@ typedef unsigned char uint8_t; typedef unsigned short uint16_t; typedef unsigned int uint32_t; +typedef unsigned long size_t; +typedef long ssize_t; #endif #else // --- else USE_HEADERS diff --git a/cpp/test/include/libc/stdlib.h b/cpp/test/include/libc/stdlib.h index 7f28000..e35eb79 100644 --- a/cpp/test/include/libc/stdlib.h +++ b/cpp/test/include/libc/stdlib.h @@ -7,6 +7,8 @@ extern "C" { #endif +extern int system(const char *); + extern void *reallocf(void *, unsigned long); int rand(void) { diff --git a/cpp/test/include/libc/string_stubs.h b/cpp/test/include/libc/string_stubs.h index d589139..c9ff6b1 100644 --- a/cpp/test/include/libc/string_stubs.h +++ b/cpp/test/include/libc/string_stubs.h @@ -28,6 +28,8 @@ extern int wprintf(const wchar_t * format, ...); extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2); extern void perror(const char *s); extern int puts(const char *s); +extern int strcmp(const char *, const char *); +extern int strncmp(const char *, const char *, size_t); extern void openlog(const char*, int, int); extern void syslog(int, const char*, ...); diff --git a/cpp/test/include/libc/unistd.h b/cpp/test/include/libc/unistd.h index 7163a23..16f2352 100644 --- a/cpp/test/include/libc/unistd.h +++ b/cpp/test/include/libc/unistd.h @@ -7,6 +7,8 @@ extern "C" { #endif +ssize_t read(int, void *, size_t); + void _exit(int); #ifdef __cplusplus diff --git a/cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.expected b/cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.expected new file mode 100644 index 0000000..672f879 --- /dev/null +++ b/cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.expected @@ -0,0 +1,12 @@ +edges +| PotentiallyUnguardedProtocolHandler.cpp:27:11:27:13 | read output argument | PotentiallyUnguardedProtocolHandler.cpp:28:29:28:31 | *buf | provenance | | +| PotentiallyUnguardedProtocolHandler.cpp:39:11:39:13 | read output argument | PotentiallyUnguardedProtocolHandler.cpp:42:31:42:33 | *buf | provenance | | +nodes +| PotentiallyUnguardedProtocolHandler.cpp:27:11:27:13 | read output argument | semmle.label | read output argument | +| PotentiallyUnguardedProtocolHandler.cpp:28:29:28:31 | *buf | semmle.label | *buf | +| PotentiallyUnguardedProtocolHandler.cpp:39:11:39:13 | read output argument | semmle.label | read output argument | +| PotentiallyUnguardedProtocolHandler.cpp:42:31:42:33 | *buf | semmle.label | *buf | +subpaths +#select +| 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 | +| 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 | diff --git a/cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qlref b/cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qlref new file mode 100644 index 0000000..de5b795 --- /dev/null +++ b/cpp/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qlref @@ -0,0 +1 @@ +security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql diff --git a/java/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md b/java/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md new file mode 100644 index 0000000..445201d --- /dev/null +++ b/java/src/docs/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.md @@ -0,0 +1,99 @@ +# Potentially unguarded protocol handler invocation + +This query detects calls to URL protocol handlers with untrusted input that may not be properly validated for dangerous protocols. This vulnerability is related to CWE-939 (Improper Authorization in Handler for Custom URL Scheme) and aligns with CVE-2022-43550. + +When Java applications invoke protocol handlers (like `Desktop.browse()`, `Runtime.exec()` with `xdg-open`/`open`, or `rundll32 url.dll,FileProtocolHandler` on Windows), untrusted URLs can potentially trigger dangerous protocols such as `file://`, `smb://`, or other custom handlers that may lead to unauthorized file access, command execution, or other security issues. + +## Detected patterns + +The query identifies several common protocol handler invocation patterns: + +- **Java AWT**: `Desktop.browse(URI)` - the platform-agnostic standard +- **Windows**: `Runtime.exec()` with `rundll32 url.dll,FileProtocolHandler` +- **Linux**: `Runtime.exec()` with `xdg-open` +- **macOS**: `Runtime.exec()` with `open` command + +## Recommendation + +Always validate URL schemes before passing them to protocol handlers. Only allow safe protocols like `http://` and `https://`. Reject or sanitize URLs containing potentially dangerous protocols. + +## Example + +The following vulnerable code passes untrusted input directly to a protocol handler: + +```java +import java.awt.Desktop; +import java.net.URI; + +public class UrlOpener { + public void openUserUrl(String userInput) throws Exception { + // VULNERABLE: No validation of the URL scheme + Desktop.getDesktop().browse(new URI(userInput)); + } +} +``` + +An attacker could provide a URL like `file:///etc/passwd` or `smb://attacker-server/share` to access unauthorized resources. + +The corrected version validates the URL scheme before opening: + +```java +import java.awt.Desktop; +import java.net.URI; + +public class UrlOpener { + public void openUserUrl(String userInput) throws Exception { + URI uri = new URI(userInput); + String scheme = uri.getScheme(); + + // Only allow safe protocols + if ("http".equalsIgnoreCase(scheme) || "https".equalsIgnoreCase(scheme)) { + Desktop.getDesktop().browse(uri); + } else { + throw new SecurityException("Rejected unsafe URL scheme: " + scheme); + } + } +} +``` + +For system command invocations: + +```java +public class UrlOpener { + public void openUserUrlViaShell(String userInput) throws Exception { + // VULNERABLE: Untrusted input passed to xdg-open + Runtime.getRuntime().exec(new String[]{"xdg-open", userInput}); + } +} +``` + +Should be corrected to validate the scheme: + +```java +import java.net.URI; + +public class UrlOpener { + private boolean isValidScheme(String url) { + try { + URI uri = new URI(url); + String scheme = uri.getScheme(); + return "http".equalsIgnoreCase(scheme) || "https".equalsIgnoreCase(scheme); + } catch (Exception e) { + return false; + } + } + + public void openUserUrlViaShell(String userInput) throws Exception { + if (isValidScheme(userInput)) { + Runtime.getRuntime().exec(new String[]{"xdg-open", userInput}); + } else { + throw new SecurityException("Invalid or unsafe URL scheme"); + } + } +} +``` + +## References + +- [CWE-939: Improper Authorization in Handler for Custom URL Scheme](https://cwe.mitre.org/data/definitions/939.html) +- [CVE-2022-43550: USB Creator has insufficiently protected credentials](https://ubuntu.com/security/CVE-2022-43550) diff --git a/java/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qhelp b/java/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qhelp new file mode 100644 index 0000000..b5495da --- /dev/null +++ b/java/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qhelp @@ -0,0 +1,35 @@ + + + +

+ URL protocol handlers are used to handle any URL scheme supported by the native desktop. + If URL inputs to these handlers are untrusted and not properly sanitized, they can be + used to perform unintended actions by another application registered to handle the same + protocol. +

+
+ +

+ Review protocol handler sinks and ensure that any URL inputs are properly sanitized if + they come from untrusted sources. +

+
+ + +

+ In the bad example, an untrusted URL is passed directly to Desktop.browse() + without any scheme validation. + In the good example, the URL scheme is checked with getScheme() before + invoking the handler. +

+
+ +
  • Positive Security: Allow + arbitrary URLs, expect arbitrary code execution +
  • +
  • CVE-2022-43550: Jitsi Desktop Client + RCE By Interacting with Malicious URL Schemes on Windows +
  • +
    +
    \ No newline at end of file diff --git a/java/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql b/java/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql new file mode 100644 index 0000000..33b3058 --- /dev/null +++ b/java/src/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql @@ -0,0 +1,166 @@ +/** + * @name Potentially unguarded protocol handler invocation + * @id tob/java/unguarded-protocol-handler + * @description Detects calls to URL protocol handlers with untrusted input that may not be properly validated for dangerous protocols + * @kind path-problem + * @tags security + * external/cwe/cwe-939 + * @precision medium + * @problem.severity warning + * @security-severity 6.5 + * @group security + */ + +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.controlflow.Guards +import semmle.code.java.security.ExternalProcess + +/** + * Generic case: invoke protocol handling through OS's protocol handling utilities. This aligns with CVE-2022-43550. + */ +class ShellProtocolHandler extends ArgumentToExec { + ShellProtocolHandler() { + // Single string: "rundll32 url.dll,FileProtocolHandler " + url or "xdg-open " + url + this instanceof StringArgumentToExec and + exists(StringLiteral sl | + sl.getParent*() = this and + sl.getValue() + .regexpMatch("(?i).*(rundll32.*url\\.dll.*FileProtocolHandler|xdg-open|\\bopen\\b).*") + ) + or + // Array: {"rundll32", "url.dll,FileProtocolHandler", url} or {"xdg-open", url} + this.getType().(Array).getElementType() instanceof TypeString and + exists(ArrayInit init, StringLiteral handler | + init = this.(ArrayCreationExpr).getInit() and + handler = init.getAnInit() and + handler.getValue().regexpMatch("(?i)(rundll32(\\.exe)?|xdg-open|open|/usr/bin/open)") + ) + } + + Expr getUrlArgument() { + // For single string exec, the URL is tainted into the concatenated string + result = this and this instanceof StringArgumentToExec + or + // For arrays with rundll32, find elements after "url.dll,FileProtocolHandler" + exists(ArrayInit init, int urlIdx, int dllIdx | + init = this.(ArrayCreationExpr).getInit() and + result = init.getInit(urlIdx) and + init.getInit(dllIdx) + .(StringLiteral) + .getValue() + .regexpMatch("(?i)url\\.dll.*FileProtocolHandler") and + urlIdx > dllIdx + ) + or + // For arrays with xdg-open/open, find elements after the handler + exists(ArrayInit init, int urlIdx, int handlerIdx | + init = this.(ArrayCreationExpr).getInit() and + result = init.getInit(urlIdx) and + init.getInit(handlerIdx) + .(StringLiteral) + .getValue() + .regexpMatch("(?i)(xdg-open|open|/usr/bin/open)") and + urlIdx > handlerIdx + ) + } + + string getHandlerType() { + exists(StringLiteral sl | + sl.getParent*() = this and + ( + sl.getValue().regexpMatch("(?i).*rundll32.*url\\.dll.*FileProtocolHandler.*") and + result = "rundll32 url.dll,FileProtocolHandler" + or + sl.getValue().regexpMatch("(?i).*xdg-open.*") and + result = "xdg-open" + or + sl.getValue().regexpMatch("(?i).*\\bopen\\b.*") and + result = "open" + ) + ) + } +} + +/** + * A call to Desktop.browse() method. This is the platform-agnostic standard now. + * NOTE(alan): the URLRedirect query will likely also trigger due to a little imprecision there + */ +class DesktopBrowseProtocolHandler extends MethodCall { + DesktopBrowseProtocolHandler() { + this.getMethod().hasName("browse") and + this.getMethod().getDeclaringType().hasQualifiedName("java.awt", "Desktop") + } + + Expr getUrlArgument() { result = this.getArgument(0) } +} + +/** + * A sanitizer node that represents URL scheme validation + */ +class UrlSchemeValidationSanitizer extends DataFlow::Node { + UrlSchemeValidationSanitizer() { + exists(MethodCall mc | + mc = this.asExpr() and + ( + // String comparison on the untrusted URL + mc.getMethod().hasName(["equals", "equalsIgnoreCase", "startsWith", "matches"]) and + exists(MethodCall getScheme | + getScheme.getParent*() = mc and + getScheme.getMethod().hasName(["getScheme", "getProtocol"]) + ) + or + // URL string contains check like: url.contains("://") + mc.getMethod().hasName(["contains", "startsWith", "matches", "indexOf"]) and + exists(StringLiteral sl | sl = mc.getAnArgument() | sl.getValue().regexpMatch(".*://.*")) + or + // Pattern matching on the string representation + mc.getMethod().hasName(["matches", "find"]) and + mc.getQualifier().getType().(RefType).hasQualifiedName("java.util.regex", "Matcher") + ) + ) + } +} + +/** + * Configuration for tracking untrusted data to protocol handler invocations + */ +module PotentiallyUnguardedProtocolHandlerConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + predicate isSink(DataFlow::Node sink) { + exists(DesktopBrowseProtocolHandler call | sink.asExpr() = call.getUrlArgument()) + or + exists(ShellProtocolHandler call | sink.asExpr() = call.getUrlArgument()) + } + + predicate isBarrier(DataFlow::Node node) { node instanceof UrlSchemeValidationSanitizer } +} + +module PotentiallyUnguardedProtocolHandlerFlow = + TaintTracking::Global; + +import PotentiallyUnguardedProtocolHandlerFlow::PathGraph + +from + PotentiallyUnguardedProtocolHandlerFlow::PathNode source, + PotentiallyUnguardedProtocolHandlerFlow::PathNode sink, Expr call, string callType +where + PotentiallyUnguardedProtocolHandlerFlow::flowPath(source, sink) and + ( + exists(DesktopBrowseProtocolHandler dbc | + call = dbc and + sink.getNode().asExpr() = dbc.getUrlArgument() and + callType = "Desktop.browse()" + ) + or + exists(ShellProtocolHandler shc | + call = shc and + sink.getNode().asExpr() = shc.getUrlArgument() and + callType = shc.getHandlerType() + ) + ) +select call, source, sink, + callType + " is called with untrusted input from $@ without proper URL scheme validation.", + source.getNode(), "this source" diff --git a/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.expected b/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.expected new file mode 100644 index 0000000..14f3d34 --- /dev/null +++ b/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.expected @@ -0,0 +1,26 @@ +edges +| PotentiallyUnguardedProtocolHandler.java:10:22:10:48 | getParameter(...) : String | PotentiallyUnguardedProtocolHandler.java:11:45:11:47 | url : String | provenance | Src:MaD:46190 | +| PotentiallyUnguardedProtocolHandler.java:11:45:11:47 | url : String | PotentiallyUnguardedProtocolHandler.java:11:37:11:48 | new URI(...) | provenance | MaD:44428 Sink:MaD:43969 | +| PotentiallyUnguardedProtocolHandler.java:20:22:20:48 | getParameter(...) : String | PotentiallyUnguardedProtocolHandler.java:21:27:21:29 | url : String | provenance | Src:MaD:46190 | +| PotentiallyUnguardedProtocolHandler.java:21:19:21:30 | new URI(...) : URI | PotentiallyUnguardedProtocolHandler.java:23:41:23:43 | uri | provenance | Sink:MaD:43969 | +| PotentiallyUnguardedProtocolHandler.java:21:27:21:29 | url : String | PotentiallyUnguardedProtocolHandler.java:21:19:21:30 | new URI(...) : URI | provenance | MaD:44428 | +| PotentiallyUnguardedProtocolHandler.java:34:22:34:48 | getParameter(...) : String | PotentiallyUnguardedProtocolHandler.java:35:27:35:29 | url : String | provenance | Src:MaD:46190 | +| PotentiallyUnguardedProtocolHandler.java:35:19:35:30 | new URI(...) : URI | PotentiallyUnguardedProtocolHandler.java:38:41:38:43 | uri | provenance | Sink:MaD:43969 | +| PotentiallyUnguardedProtocolHandler.java:35:27:35:29 | url : String | PotentiallyUnguardedProtocolHandler.java:35:19:35:30 | new URI(...) : URI | provenance | MaD:44428 | +nodes +| PotentiallyUnguardedProtocolHandler.java:10:22:10:48 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| PotentiallyUnguardedProtocolHandler.java:11:37:11:48 | new URI(...) | semmle.label | new URI(...) | +| PotentiallyUnguardedProtocolHandler.java:11:45:11:47 | url : String | semmle.label | url : String | +| PotentiallyUnguardedProtocolHandler.java:20:22:20:48 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| PotentiallyUnguardedProtocolHandler.java:21:19:21:30 | new URI(...) : URI | semmle.label | new URI(...) : URI | +| PotentiallyUnguardedProtocolHandler.java:21:27:21:29 | url : String | semmle.label | url : String | +| PotentiallyUnguardedProtocolHandler.java:23:41:23:43 | uri | semmle.label | uri | +| PotentiallyUnguardedProtocolHandler.java:34:22:34:48 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| PotentiallyUnguardedProtocolHandler.java:35:19:35:30 | new URI(...) : URI | semmle.label | new URI(...) : URI | +| PotentiallyUnguardedProtocolHandler.java:35:27:35:29 | url : String | semmle.label | url : String | +| PotentiallyUnguardedProtocolHandler.java:38:41:38:43 | uri | semmle.label | uri | +subpaths +#select +| PotentiallyUnguardedProtocolHandler.java:11:9:11:49 | browse(...) | PotentiallyUnguardedProtocolHandler.java:10:22:10:48 | getParameter(...) : String | PotentiallyUnguardedProtocolHandler.java:11:37:11:48 | new URI(...) | Desktop.browse() is called with untrusted input from $@ without proper URL scheme validation. | PotentiallyUnguardedProtocolHandler.java:10:22:10:48 | getParameter(...) | this source | +| PotentiallyUnguardedProtocolHandler.java:23:13:23:44 | browse(...) | PotentiallyUnguardedProtocolHandler.java:20:22:20:48 | getParameter(...) : String | PotentiallyUnguardedProtocolHandler.java:23:41:23:43 | uri | Desktop.browse() is called with untrusted input from $@ without proper URL scheme validation. | PotentiallyUnguardedProtocolHandler.java:20:22:20:48 | getParameter(...) | this source | +| PotentiallyUnguardedProtocolHandler.java:38:13:38:44 | browse(...) | PotentiallyUnguardedProtocolHandler.java:34:22:34:48 | getParameter(...) : String | PotentiallyUnguardedProtocolHandler.java:38:41:38:43 | uri | Desktop.browse() is called with untrusted input from $@ without proper URL scheme validation. | PotentiallyUnguardedProtocolHandler.java:34:22:34:48 | getParameter(...) | this source | diff --git a/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java b/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java new file mode 100644 index 0000000..37436d3 --- /dev/null +++ b/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java @@ -0,0 +1,98 @@ +import java.awt.Desktop; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import javax.servlet.http.HttpServletRequest; + +public class PotentiallyUnguardedProtocolHandler { + + public void bad1(HttpServletRequest request) throws IOException, URISyntaxException { + String url = request.getParameter("url"); + Desktop.getDesktop().browse(new URI(url)); + } + + public void bad2(String userInput) throws IOException, URISyntaxException { + URI uri = new URI(userInput); + Desktop.getDesktop().browse(uri); + } + + public void safe1(HttpServletRequest request) throws IOException, URISyntaxException { + String url = request.getParameter("url"); + URI uri = new URI(url); + if (uri.getScheme().equals("https") || uri.getScheme().equals("http")) { + Desktop.getDesktop().browse(uri); + } + } + + public void safe2(String userInput) throws IOException, URISyntaxException { + if (userInput.startsWith("https://") || userInput.startsWith("http://")) { + Desktop.getDesktop().browse(new URI(userInput)); + } + } + + public void bad3(HttpServletRequest request) throws IOException, URISyntaxException { + String url = request.getParameter("url"); + URI uri = new URI(url); + // Weak check - only checks if scheme exists, not what it is + if (uri.getScheme() != null) { + Desktop.getDesktop().browse(uri); + } + } + + public void safe3(String userInput) throws IOException, URISyntaxException { + URI uri = new URI(userInput); + String scheme = uri.getScheme(); + if (scheme != null && (scheme.equalsIgnoreCase("http") || scheme.equalsIgnoreCase("https"))) { + Desktop.getDesktop().browse(uri); + } + } + + public void safe4() throws IOException, URISyntaxException { + Desktop.getDesktop().browse(new URI("https://example.com")); + } + + public void bad4_rundll32(String userInput) throws IOException { + String[] cmd = { "rundll32", "url.dll,FileProtocolHandler", userInput }; + Runtime.getRuntime().exec(cmd); + } + + public void bad6_rundll32(HttpServletRequest request) throws IOException { + String url = request.getParameter("url"); + ProcessBuilder pb = new ProcessBuilder("rundll32", "url.dll,FileProtocolHandler", url); + pb.start(); + } + + public void safe5_rundll32(HttpServletRequest request) throws IOException, URISyntaxException { + String url = request.getParameter("url"); + URI uri = new URI(url); + if (uri.getScheme().equals("https") || uri.getScheme().equals("http")) { + String[] cmd = { "rundll32", "url.dll,FileProtocolHandler", url }; + Runtime.getRuntime().exec(cmd); + } + } + + public void safe6_rundll32(String userInput) throws IOException { + if (userInput.startsWith("https://") || userInput.startsWith("http://")) { + String[] cmd = { "rundll32", "url.dll,FileProtocolHandler", userInput }; + Runtime.getRuntime().exec(cmd); + } + } + + public void safe7_rundll32() throws IOException { + Runtime.getRuntime().exec("rundll32 url.dll,FileProtocolHandler https://example.com"); + } + + public void bad7_xdgopen(String userInput) throws IOException { + String[] cmd = { "xdg-open", userInput }; + Runtime.getRuntime().exec(cmd); + } + + public void safe8_xdgopen(HttpServletRequest request) throws IOException, URISyntaxException { + String url = request.getParameter("url"); + URI uri = new URI(url); + if (uri.getScheme().equals("https") || uri.getScheme().equals("http")) { + String[] cmd = { "xdg-open", url }; + Runtime.getRuntime().exec(cmd); + } + } +} diff --git a/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qlref b/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qlref new file mode 100644 index 0000000..de5b795 --- /dev/null +++ b/java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.qlref @@ -0,0 +1 @@ +security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.ql