Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions java/ql/lib/semmle/code/java/security/RequestForgery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,24 @@ private class HostComparisonSanitizer extends RequestForgerySanitizer {
this = DataFlow::BarrierGuard<isHostComparisonSanitizer/3>::getABarrierNode()
}
}

/**
* A qualifier in a call to a `.matches()` method that is a sanitizer for URL redirects.
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation mentions 'URL redirects' but this sanitizer is for request forgery (SSRF), not URL redirection. The comment should refer to 'request forgery' or 'SSRF' to match the actual usage context.

Copilot uses AI. Check for mistakes.
*
* Matches any method call where the method is named `matches`.
*/
private predicate isMatchesSanitizer(Guard guard, Expr e, boolean branch) {
guard =
any(MethodCall method |
method.getMethod().getName() = "matches" and
e = method.getQualifier() and
branch = true
)
}

/**
* A qualifier in a call to `.matches()` that is a sanitizer for URL redirects.
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation mentions 'URL redirects' but should refer to 'request forgery' or 'SSRF' to accurately describe the sanitizer's purpose in this context.

Copilot uses AI. Check for mistakes.
*/
private class MatchesSanitizer extends RequestForgerySanitizer {
MatchesSanitizer() { this = DataFlow::BarrierGuard<isMatchesSanitizer/3>::getABarrierNode() }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Calls to `String.matches` are now treated as sanitizers for the `java/ssrf` query.
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,30 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
String unsafeUri10 = String.format("%s://%s:%s%s", "http", "myserver.com", "80", request.getParameter("baduri10")); // $ Source
HttpRequest unsafer10 = HttpRequest.newBuilder(new URI(unsafeUri10)).build(); // $ Alert
client.send(unsafer10, null); // $ Alert

// GOOD: sanitisation by regexp validation
String safeUri10 = "https://example.com/";
String param10 = request.getParameter("uri10");
if (param10.matches("[a-zA-Z0-9/_-]+")) {
safeUri10 = safeUri10 + param10;
}
HttpRequest r10 = HttpRequest.newBuilder(new URI(safeUri10)).build();
client.send(r10, null);


String param11 = request.getParameter("uri11");
validate(param11);
String safeUri11 = "https://example.com/" + param11;
HttpRequest r11 = HttpRequest.newBuilder(new URI(safeUri11)).build();
client.send(r11, null);
Comment on lines 123 to 133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a minute, are these two test cases even testing the regex sanitizer? If I've understood the existing sanitizers correctly, then the concatenation with a fixed url ending in / like "https://example.com/" + param11 is itself a sanitizer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two test cases need to replace the fixed-string url "https://example.com/" with something that isn't analyzable as a potential url-prefix by the StringPrefixes library. Maybe just get the string from a collection field that we pretend is initialized or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; the first test is actually ok, since safeUri10 is not a constant, but I'll update that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified that the updated tests both result in alerts when the sanitizer is disabled.

} catch (Exception e) {
// TODO: handle exception
}
}

private void validate(String s) {
if (!s.matches("[a-zA-Z0-9/_-]+")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing / here sounds like playing with fire, so let's not do that.

Suggested change
if (!s.matches("[a-zA-Z0-9/_-]+")) {
if (!s.matches("[a-zA-Z0-9_-]+")) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I happily took what Copilot gave me ;-)

throw new IllegalArgumentException("Invalid ID");
}
}
}
2 changes: 1 addition & 1 deletion misc/scripts/create-change-note.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
# - What language the change note is for
# - Whether it's a query or library change (the string `src` or `lib`)
# - The name of the change note (in kebab-case)
# - The category of the change.
# - The category of the change (see https://github.com/github/codeql/blob/main/docs/change-notes.md#change-categories).

# The change note will be created in the `{language}/ql/{subdir}/change-notes` directory, where `subdir` is either `src` or `lib`.

Expand Down