Repository navigation
Java: Add Spring MVC URL redirect sinks - #22362
theinfosecguy wants to merge 4 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.
| /** Gets the leading expression of a complete view name, following only left-hand concatenation. */ | ||
| pragma[nomagic] | ||
| private Expr getViewNamePrefix(Expr value) { | ||
| result = getLocalViewValue(value) and not result instanceof AddExpr |
There was a problem hiding this comment.
The prefix walk stops at conditional expressions, so return next == null ? redirect("/") : redirect(next); produces no alert when next is request-controlled. This is the same ternary-return pattern as the original report in #21771, so that reported case is still missed. Conditional concatenations and branch-assigned local names are also missed. Please analyze the possible value branches and add the original pattern as a positive regression test.
| exists(ReturnStmt ret | | ||
| ret.getEnclosingCallable() = value.getEnclosingCallable() and | ||
| ret.getEnclosingCallable() instanceof SpringRequestMappingMethod and | ||
| DataFlow::localFlow(DataFlow::exprNode(value), DataFlow::exprNode(ret.getExpr())) and |
There was a problem hiding this comment.
Requiring direct local flow to the return expression misses return new ModelAndView(new RedirectView(next));. The returned object is the outer ModelAndView, so the inner redirect view fails this check. The same happens with view.setUrl(next); return new ModelAndView(view);. Both perform redirects, but neither reports when next is request-controlled. Please follow the ModelAndView(View) argument when identifying returned redirect views, and add regression tests for both forms.
|
|
||
| pragma[nomagic] | ||
| private predicate appendedToRedirectPrefix(SpringRedirectPrefix prefix, DataFlow::ExprNode exprNode) { | ||
| exprNode.asExpr() = prefix.getAnAppendedExpression() |
There was a problem hiding this comment.
Straight-line local prefix variables (String prefix = "redirect:"; return prefix + next;) fail because getViewNamePrefix follows prefix back to the "redirect:" literal in the variable declaration, where it has no enclosing AddExpr, so InterestingPrefix.getAnAppendedExpression in appendedToRedirectPrefix returns nothing.
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:string concatenations, helper-generated names returned directly or supplied to returnedModelAndViewobjects,RedirectViewconstructor overloads, andsetUrl. Returned view objects remain covered under response-body annotations; string return values retain their response-body and model-attribute semantics.The sinks follow the actual returned view and its view-name argument. Precision checks cover ordinary view names, discarded and model-only values, helper sanitizers, and definite local constant replacements. Views that escape or have ambiguous aliases are handled conservatively.
Validation: