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.
| private predicate isSpringMvcViewResult(Expr value) { | ||
| contributesToReturn(value) and | ||
| value.getEnclosingCallable() instanceof SpringRequestMappingMethod and | ||
| not value.getEnclosingCallable().(SpringRequestMappingMethod).isResponseBody() |
There was a problem hiding this comment.
Spring processes view objects before the response-body handler, so a @RestController returning new RedirectView(next) still performs a redirect. This condition excludes that case, as well as redirecting ModelAndView results. Please keep the response-body exclusion for string view names, but allow returned view objects. A regression test for a @RestController returning a user-controlled redirect would cover this.
There was a problem hiding this comment.
Fixed this and added regression tests.
| 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)).
|
|
||
| pragma[nomagic] | ||
| private predicate appendedToRedirectPrefix(DataFlow::ExprNode exprNode) { | ||
| exists(SpringRedirectPrefix prefix | exprNode.asExpr() = prefix.getAnAppendedExpression()) |
There was a problem hiding this comment.
This can match a redirect: literal in the middle of an ordinary view name. For example, String base = "home"; return base + "redirect:" + next; marks next as a sink, although the result starts with homeredirect: and is not a Spring redirect. Please require redirect: to start the resulting view name, and add a negative test for this case.
There was a problem hiding this comment.
Fixed. The view name must start with redirect:. Added your example as a negative test.
This branch has not been deployed
3 participants
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: