-
Notifications
You must be signed in to change notification settings - Fork 7
Add PotentiallyUnguardedProtocolHandler query for Java and C++ #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
...-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java
Dismissed
Show dismissed
Hide dismissed
...-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java
Dismissed
Show dismissed
Hide dismissed
...-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java
Dismissed
Show dismissed
Hide dismissed
| public void bad4_rundll32(HttpServletRequest request) throws IOException { | ||
| String url = request.getParameter("url"); | ||
| // Single string command with concatenation | ||
| Runtime.getRuntime().exec("rundll32 url.dll,FileProtocolHandler " + url); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical test
user-provided value
| public void bad6_rundll32(HttpServletRequest request) throws IOException { | ||
| String url = request.getParameter("url"); | ||
| // ProcessBuilder with list | ||
| ProcessBuilder pb = new ProcessBuilder("rundll32", "url.dll,FileProtocolHandler", url); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical test
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To fix the problem, we need to ensure that user input supplied to the command is properly validated and cannot be used to trigger malicious command execution or open arbitrary protocols. The safest approach is to check that the url parameter represents only URLs with an http or https scheme, matching the already-correct approaches in neighboring "safe" functions in the file.
Specifically, before constructing and starting the ProcessBuilder, we should:
- Parse the user-provided input as a
URI. - Check that the scheme is
httporhttps. - Only if this check passes, proceed with the command execution as before.
This fix only needs additional code in the bad6_rundll32 method. No import changes are needed, as the required classes (URI, URISyntaxException) are already imported.
The method must be updated to perform the scheme check, and must declare or handle URISyntaxException. For consistency with the file's style, using a throws clause is acceptable.
-
Copy modified line R69 -
Copy modified lines R71-R76
| @@ -66,11 +66,14 @@ | ||
| Runtime.getRuntime().exec(cmd); | ||
| } | ||
|
|
||
| public void bad6_rundll32(HttpServletRequest request) throws IOException { | ||
| public void bad6_rundll32(HttpServletRequest request) throws IOException, URISyntaxException { | ||
| String url = request.getParameter("url"); | ||
| // ProcessBuilder with list | ||
| ProcessBuilder pb = new ProcessBuilder("rundll32", "url.dll,FileProtocolHandler", url); | ||
| pb.start(); | ||
| // Validate that url scheme is http or https before executing command | ||
| URI uri = new URI(url); | ||
| if ("http".equalsIgnoreCase(uri.getScheme()) || "https".equalsIgnoreCase(uri.getScheme())) { | ||
| 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")) { | ||
| Runtime.getRuntime().exec("rundll32 url.dll,FileProtocolHandler " + url); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical test
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To fix this problem, the code should avoid passing user input into the shell by concatenation. Instead, validate the URL properly and use the overloaded method of Runtime.exec that takes a command array, so that metacharacters are not interpreted by the shell. Additionally, further sanitize or validate the URL to ensure it is a legitimate web URL. The most robust fix is to:
- Parse the user input as a URI.
- Check that the scheme is
"http"or"https". - Pass the value as a separate argument to
Runtime.exec(String[])rather than using concatenation.
Alternatively, if possible, restrict the allowed URLs using a whitelist or rigorous pattern. However, according to the code's intent and surrounding examples, switching to an array-based invocation with validation is most appropriate.
Lines to change:
In safe5_rundll32 (lines 76–82), specifically the line:
Runtime.getRuntime().exec("rundll32 url.dll,FileProtocolHandler " + url);
Change to:
Runtime.getRuntime().exec(new String[] { "rundll32", "url.dll,FileProtocolHandler", url });
No new imports are needed, as all required classes are already imported.
-
Copy modified line R80
| @@ -77,7 +77,7 @@ | ||
| String url = request.getParameter("url"); | ||
| URI uri = new URI(url); | ||
| if (uri.getScheme().equals("https") || uri.getScheme().equals("http")) { | ||
| Runtime.getRuntime().exec("rundll32 url.dll,FileProtocolHandler " + url); | ||
| Runtime.getRuntime().exec(new String[] { "rundll32", "url.dll,FileProtocolHandler", url }); | ||
| } | ||
| } | ||
|
|
| // xdg-open test cases (Linux) | ||
| public void bad7_xdgopen(HttpServletRequest request) throws IOException { | ||
| String url = request.getParameter("url"); | ||
| Runtime.getRuntime().exec("xdg-open " + url); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical test
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To fix this issue, we must ensure that the parameter passed to the xdg-open command cannot be exploited for command injection. The best general fix is to validate the user-supplied URL to ensure only legitimate URLs are allowed, limiting the schemes to http and https (as seen in neighboring "safe" methods in the same file). Then, construct the command using an array form of exec(), which avoids shell command line parsing altogether, reducing injection risk.
Specifically, in bad7_xdgopen, parse the string as a URI, check that its scheme is "http" or "https", and if so, use Runtime.getRuntime().exec(new String[]{"xdg-open", url}). This mirrors the safe patterns in the file while preserving original logic, and only permits opening actual network URLs.
Changes need to be made in bad7_xdgopen:
- Parse
urlas a URI. - Check its scheme.
- If permitted, proceed with the exec as an array form to avoid shell parsing.
Also, add athrows URISyntaxExceptionto the exception list for the method, asnew URI(url)may throw this.
-
Copy modified line R96 -
Copy modified lines R98-R101
| @@ -93,9 +93,12 @@ | ||
| } | ||
|
|
||
| // xdg-open test cases (Linux) | ||
| public void bad7_xdgopen(HttpServletRequest request) throws IOException { | ||
| public void bad7_xdgopen(HttpServletRequest request) throws IOException, URISyntaxException { | ||
| String url = request.getParameter("url"); | ||
| Runtime.getRuntime().exec("xdg-open " + url); | ||
| URI uri = new URI(url); | ||
| if ("http".equals(uri.getScheme()) || "https".equals(uri.getScheme())) { | ||
| Runtime.getRuntime().exec(new String[]{"xdg-open", url}); | ||
| } | ||
| } | ||
|
|
||
| public void bad8_xdgopen(String userInput) throws IOException { |
| String url = request.getParameter("url"); | ||
| URI uri = new URI(url); | ||
| if (uri.getScheme().equals("https") || uri.getScheme().equals("http")) { | ||
| Runtime.getRuntime().exec("xdg-open " + url); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical test
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
General fix:
To fix this vulnerability, always avoid inserting raw user input into system command strings without rigorous validation and escaping. Instead, use the variant of exec (or ProcessBuilder) that accepts a string array, so that arguments are interpreted literally rather than by a shell. Additionally, ensure proper validation so only intended URLs (with correct schema and structure) are used.
Detailed fix for the code shown:
In method safe8_xdgopen, instead of concatenating url into a string and executing it as a shell command, pass the arguments as a String array:
new String[] { "xdg-open", url }Also, maintain the existing scheme check (as in line 109). To be extra secure, you may also want to validate that url is a valid HTTP/HTTPS URI.
Files/lines to change:
File: java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java
Change lines 110:
Runtime.getRuntime().exec("xdg-open " + url);to:
Runtime.getRuntime().exec(new String[] {"xdg-open", url});No extra imports or methods are required.
-
Copy modified line R110
| @@ -107,7 +107,7 @@ | ||
| String url = request.getParameter("url"); | ||
| URI uri = new URI(url); | ||
| if (uri.getScheme().equals("https") || uri.getScheme().equals("http")) { | ||
| Runtime.getRuntime().exec("xdg-open " + url); | ||
| Runtime.getRuntime().exec(new String[] {"xdg-open", url}); | ||
| } | ||
| } | ||
|
|
| // open test cases (macOS) | ||
| public void bad9_open(HttpServletRequest request) throws IOException { | ||
| String url = request.getParameter("url"); | ||
| Runtime.getRuntime().exec("open " + url); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical test
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
The best general-purpose fix is to avoid passing unsanitized user input directly into the shell command. Instead, validate that url is safe; for a handler like macOS open, typically only http(s) URLs are expected.
A robust fix is to:
- Parse and validate the URL, ensuring that only acceptable schemes ("http", "https") are allowed.
- Optionally, use the overload of
Runtime.execthat accepts a String array, supplying "open" as one element and the URL as another. This avoids string concatenation and disables any shell interpretation. - Validation should be done before the exec call. If the input is invalid, do not launch the command.
For this file, replace the vulnerable code in method bad9_open (lines 119-122), so user input must be validated for scheme and then passed as a separate arg. Mirror the approach used in safer methods above, such as safe10_open.
Required changes:
- In
bad9_open, check that the URL starts withhttp://orhttps://(or, stronger, parse as URI and check the scheme). - Use
Runtime.getRuntime().exec(new String[]{"open", url}); - If the validation fails, do not run the command (optionally: throw exception, log, or silently ignore).
No new imports are needed, as java.net.URI and java.net.URISyntaxException are already imported.
-
Copy modified line R119 -
Copy modified lines R121-R124
| @@ -116,9 +116,12 @@ | ||
| } | ||
|
|
||
| // open test cases (macOS) | ||
| public void bad9_open(HttpServletRequest request) throws IOException { | ||
| public void bad9_open(HttpServletRequest request) throws IOException, URISyntaxException { | ||
| String url = request.getParameter("url"); | ||
| Runtime.getRuntime().exec("open " + url); | ||
| URI uri = new URI(url); | ||
| if ("https".equals(uri.getScheme()) || "http".equals(uri.getScheme())) { | ||
| Runtime.getRuntime().exec(new String[]{"open", url}); | ||
| } | ||
| } | ||
|
|
||
| public void bad10_open(String userInput) throws IOException { |
| public void safe10_open(HttpServletRequest request) throws IOException, URISyntaxException { | ||
| String url = request.getParameter("url"); | ||
| if (url.startsWith("https://") || url.startsWith("http://")) { | ||
| Runtime.getRuntime().exec("/usr/bin/open " + url); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical test
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
The best way to fix this vulnerability is to avoid passing user input directly to a shell, and instead invoke system utilities through an argument array, so the input is never interpreted by a command shell. This is achieved by using Runtime.exec(String[]) or ProcessBuilder. Additionally, further validation should occur to ensure only intended URLs (for example, well-formed http/https URLs) are accepted. It's also best to use the absolute path to the executable as the first argument, followed by the user input as a separate argument, thus avoiding shell interpretation entirely.
For file java/test/query-tests/security/PotentiallyUnguardedProtocolHandler/PotentiallyUnguardedProtocolHandler.java, function safe10_open, replace the string concatenation with a parameterized command, and consider using new ProcessBuilder(...) or Runtime.getRuntime().exec(String[]) with an array.
-
Copy modified line R132
| @@ -129,7 +129,7 @@ | ||
| public void safe10_open(HttpServletRequest request) throws IOException, URISyntaxException { | ||
| String url = request.getParameter("url"); | ||
| if (url.startsWith("https://") || url.startsWith("http://")) { | ||
| Runtime.getRuntime().exec("/usr/bin/open " + url); | ||
| Runtime.getRuntime().exec(new String[]{"/usr/bin/open", url}); | ||
| } | ||
| } | ||
|
|
No description provided.