Repository navigation
Java: Add Spring MVC URL redirect sinks - #22362
theinfosecguy wants to merge 8 commits into
Conversation
owen-mc
left a comment
owen-mc
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing this.
| SpringUrlRedirectHelperSink() { | ||
| exists(MethodCall call | | ||
| this.asExpr() = call and | ||
| isSpringMvcViewResult(call) and |
There was a problem hiding this comment.
This misses return new ModelAndView(redirect(next));, where the helper returns "redirect:" + next. The helper call supplies the view name, but the returned value is the enclosing ModelAndView, so this direct-return check fails. Please also recognize helper calls used as the name of a returned ModelAndView, and add a regression test for this case.
There was a problem hiding this comment.
Fixed this and added a regression test for new ModelAndView(redirect(next)).
owen-mc
left a comment
owen-mc
left a comment
There was a problem hiding this comment.
Other sinks in UrlRedirect.qll and UrlForwardQuery.qll do not do post-write overwrite analysis, so no need to do it here. It is overly complicated.
|
Overwrite analysis: Removed it to match the existing sinks. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| result = value.getUnderlyingExpr() and | ||
| not result instanceof ConditionalExpr and | ||
| ( | ||
| not exists(SsaDefinition def | localViewDefinition(value, def)) |
owen-mc
left a comment
owen-mc
left a comment
There was a problem hiding this comment.
Sorry for the delay in re-reviewing. This is pretty much ready to merge. You just need to merge in/rebase on main and fix the merge conflict in UrlRedirect.expected by running codeql test run -j 0 --learn java/ql/test/query-tests/security/CWE-601/semmle/tests.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved redirect-modeling issues miss tainted destinations and flag fixed or allowlisted targets, causing both false negatives and false positives.
11 open issues
What changed in this PR
Extends the stable Java URL-redirection query to Spring MVC, addressing #21771’s missed redirect view names and helper-mediated redirects.
Changes:
- Add sink modeling for returned redirect names and
RedirectViewobjects, including helpers andModelAndViewwrappers. - Add positive and negative regression cases while preserving string response-body and model-attribute semantics.
- Update Spring stubs, generated expectations, and release notes.
| File | Description |
|---|---|
java/ql/test/stubs/springframework-5.8.x/org/springframework/web/servlet/view/AbstractView.java |
Add the base View implementation stub. |
java/ql/test/stubs/springframework-5.8.x/org/springframework/web/servlet/view/AbstractUrlBasedView.java |
Connect URL-based views to the stub hierarchy. |
java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected |
Record generated Spring redirect results and flow paths. |
java/ql/test/query-tests/security/CWE-601/semmle/tests/SpringUrlRedirect.java |
Add redirect coverage and false-positive regression fixtures. |
java/ql/test/query-tests/security/CWE-601/semmle/tests/options |
Include Spring stubs during test extraction. |
java/ql/lib/semmle/code/java/security/UrlRedirect.qll |
Model Spring redirect values used by returned views. |
java/ql/lib/change-notes/2026-08-17-spring-url-redirect-sinks.md |
Document the expanded stable-query coverage. |
Review effort: Max (auto)
| this.asExpr() = getRedirectTarget(name) and | ||
| TaintTracking::localTaint(this, DataFlow::exprNode(name)) |
| /** The exact Spring redirect prefix, without a fixed target path or host. */ | ||
| private class SpringRedirectPrefix extends CompileTimeConstantExpr { | ||
| SpringRedirectPrefix() { this.getStringValue() = "redirect:" } | ||
| } |
|
|
||
| /** Relates a local read to its reaching SSA definitions. */ | ||
| private predicate localViewDefinition(Expr value, SsaDefinition def) { | ||
| value.getUnderlyingExpr().(VarRead).getVariable() instanceof LocalVariableDecl and |
There was a problem hiding this comment.
This seems like a small change worth doing.
| not result instanceof ConditionalExpr and | ||
| ( | ||
| not localViewDefinition(value, _) | ||
| or | ||
| exists(SsaDefinition def | | ||
| localViewDefinition(value, def) and | ||
| not exists(def.getAnUltimateDefinition().(SsaExplicitWrite).getValue()) | ||
| ) | ||
| ) | ||
| or | ||
| exists(SsaDefinition def | | ||
| localViewDefinition(value, def) and | ||
| result = getAViewNameValue(def.getAnUltimateDefinition().(SsaExplicitWrite).getValue()) | ||
| ) | ||
| or | ||
| result = getAViewNameValue(value.getUnderlyingExpr().(ConditionalExpr).getABranchExpr()) |
There was a problem hiding this comment.
This seems like a small change that is worth doing.
| exists(MethodCall call, ReturnStmt ret | | ||
| callable.(Method).getReturnType() instanceof TypeString and | ||
| ret = unique(ReturnStmt r | r.getEnclosingCallable() = callable) and | ||
| call = unique(Expr prefix | prefix = getViewNamePrefix(ret.getExpr())) and |
| setUrl | ||
| .getQualifier() | ||
| .getType() |
There was a problem hiding this comment.
This seems worth accepting.
| String next = request.getParameter("next"); // $ Source | ||
| RedirectView view = new RedirectView(next); // $ Alert |
There was a problem hiding this comment.
This should be addressed (assuming you after that they are all FPs)
Fixes #21771.
Adds stable
java/unvalidated-url-redirectioncoverage for Spring MVC redirect view names and returnedRedirectViewobjects, building on the existing experimental Spring coverage.Covers leading
redirect:concatenations, conditional and branch-assigned names, helpers used directly or as returnedModelAndViewnames,RedirectViewconstructors andsetUrl, andModelAndView(View)wrappers. Returned view objects remain covered under response-body annotations; string return values retain their response-body and model-attribute semantics.The sinks follow values used by a returned view. Regression tests cover ordinary and embedded-prefix names, discarded and model-only values, reassignment, distinct receivers, overloads, and sanitizers. As with existing redirect sinks, later property writes do not suppress alerts.
Validation: