Skip to content

Commit abe0c78

Browse files
theinfosecguytheinfosecguy
committed
Fix Spring redirect view result modeling
1 parent 64c9ecc commit abe0c78

4 files changed

Lines changed: 819 additions & 62 deletions

File tree

‎java/ql/lib/change-notes/2026-08-17-spring-url-redirect-sinks.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
---
22
category: minorAnalysis
33
---
4-
* The `java/unvalidated-url-redirection` query now detects untrusted URLs used in Spring MVC
5-
`RedirectView` objects and `redirect:` view names, including view names constructed in helper
6-
methods called by request handlers.
4+
* The `java/unvalidated-url-redirection` query now detects untrusted URLs used in returned Spring MVC
5+
`RedirectView` objects and leading `redirect:` view names, including helper-generated names passed
6+
to returned `ModelAndView` objects. View objects returned by response-body handlers are also covered.

‎java/ql/lib/semmle/code/java/security/UrlRedirect.qll‎

Lines changed: 177 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ import semmle.code.java.frameworks.ApacheHttp
99
private import semmle.code.java.dataflow.ExternalFlow
1010
private import semmle.code.java.dataflow.FlowSinks
1111
private import semmle.code.java.dataflow.StringPrefixes
12+
private import semmle.code.java.dataflow.SSA
1213
private import semmle.code.java.dataflow.TaintTracking
14+
private import semmle.code.java.controlflow.Dominance
1315
private import semmle.code.java.frameworks.JaxWS
1416
private import semmle.code.java.frameworks.spring.SpringController
1517
private import semmle.code.java.security.RequestForgery
@@ -56,29 +58,30 @@ private class ApacheUrlRedirectSink extends UrlRedirectSink {
5658
/** An expression appended to a Spring `"redirect:"` view-name returned by a request handler. */
5759
private class SpringUrlRedirectPrefixSink extends UrlRedirectSink {
5860
SpringUrlRedirectPrefixSink() {
59-
appendedToRedirectPrefix(this) and
60-
(
61-
isSpringMvcReturnedString(this.asExpr())
62-
or
63-
isSpringModelAndViewName(this.asExpr())
61+
exists(Expr name, SpringRedirectPrefix prefix |
62+
(isSpringMvcReturnedString(name) or isSpringModelAndViewName(name)) and
63+
prefix = getViewNamePrefix(name) and
64+
appendedToRedirectPrefix(prefix, this) and
65+
TaintTracking::localTaint(this, DataFlow::exprNode(name))
6466
)
6567
}
6668
}
6769

6870
/** A call to a helper that returns a Spring `"redirect:"` view name. */
6971
private class SpringUrlRedirectHelperSink extends UrlRedirectSink {
7072
SpringUrlRedirectHelperSink() {
71-
exists(MethodCall call |
73+
exists(Expr name, MethodCall call |
7274
this.asExpr() = call and
73-
isSpringMvcViewResult(call) and
75+
(isSpringMvcReturnedString(name) or isSpringModelAndViewName(name)) and
76+
call = getViewNamePrefix(name) and
7477
returnsRedirectViewName(call.getCallee().getSourceDeclaration())
7578
)
7679
}
7780
}
7881

7982
pragma[nomagic]
80-
private predicate appendedToRedirectPrefix(DataFlow::ExprNode exprNode) {
81-
exists(SpringRedirectPrefix prefix | exprNode.asExpr() = prefix.getAnAppendedExpression())
83+
private predicate appendedToRedirectPrefix(SpringRedirectPrefix prefix, DataFlow::ExprNode exprNode) {
84+
exprNode.asExpr() = prefix.getAnAppendedExpression()
8285
}
8386

8487
private class SpringRedirectPrefix extends InterestingPrefix {
@@ -87,61 +90,194 @@ private class SpringRedirectPrefix extends InterestingPrefix {
8790
override int getOffset() { result = 0 }
8891
}
8992

90-
private predicate contributesToReturn(Expr value) {
91-
exists(ReturnStmt ret |
92-
ret.getEnclosingCallable() = value.getEnclosingCallable() and
93-
DataFlow::localFlow(DataFlow::exprNode(value), DataFlow::exprNode(ret.getExpr()))
93+
/** Relates a local read to its reaching SSA definitions. */
94+
private predicate localViewDefinition(Expr value, SsaDefinition def) {
95+
value.getUnderlyingExpr().(VarRead).getVariable() instanceof LocalVariableDecl and
96+
def.getARead() = value.getUnderlyingExpr()
97+
}
98+
99+
/** Gets a terminal local value, retaining unknown or merged definitions as unknown reads. */
100+
pragma[nomagic]
101+
private Expr getALocalViewValue(Expr value) {
102+
result = value.getUnderlyingExpr() and
103+
(
104+
not exists(SsaDefinition def | localViewDefinition(value, def))
105+
or
106+
exists(SsaDefinition def |
107+
localViewDefinition(value, def) and
108+
not exists(def.(SsaExplicitWrite).getValue())
109+
)
110+
)
111+
or
112+
exists(SsaExplicitWrite def |
113+
localViewDefinition(value, def) and
114+
result = getALocalViewValue(def.getValue())
94115
)
95116
}
96117

97-
/** Holds if `value` contributes string content to the return value of its callable. */
98-
private predicate contributesToReturnedString(Expr value) {
99-
exists(ReturnStmt ret |
100-
ret.getEnclosingCallable() = value.getEnclosingCallable() and
101-
TaintTracking::localTaint(DataFlow::exprNode(value), DataFlow::exprNode(ret.getExpr()))
118+
/** Gets the unique local value, excluding uncertain origins. */
119+
private Expr getLocalViewValue(Expr value) {
120+
result = unique(Expr terminal | terminal = getALocalViewValue(value))
121+
}
122+
123+
/** Gets the leading expression of a complete view name, following only left-hand concatenation. */
124+
pragma[nomagic]
125+
private Expr getViewNamePrefix(Expr value) {
126+
result = getLocalViewValue(value) and not result instanceof AddExpr
127+
or
128+
result = getViewNamePrefix(getLocalViewValue(value).(AddExpr).getLeftOperand())
129+
}
130+
131+
/** Holds if a call writes the URL or view-name property of a Spring view. */
132+
private predicate isSpringViewPropertyWrite(MethodCall call, string property) {
133+
property = "url" and
134+
call.getMethod()
135+
.getSourceDeclaration()
136+
.hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView", "setUrl")
137+
or
138+
property = "view" and
139+
call.getMethod()
140+
.getSourceDeclaration()
141+
.hasQualifiedName("org.springframework.web.servlet", "ModelAndView",
142+
["setViewName", "setView"])
143+
}
144+
145+
/** Gets the definite local allocation behind `value`, excluding ambiguous or merged values. */
146+
private ClassInstanceExpr getViewAllocation(Expr value) { result = getLocalViewValue(value) }
147+
148+
/** Holds if an allocation can be changed outside the local, recognized property writes. */
149+
private predicate viewEscapes(ClassInstanceExpr allocation, string property) {
150+
property = ["url", "view"] and
151+
(
152+
exists(Call call |
153+
DataFlow::localFlow(DataFlow::exprNode(allocation), DataFlow::exprNode(call.getAnArgument()))
154+
)
155+
or
156+
exists(MethodCall call |
157+
DataFlow::localFlow(DataFlow::exprNode(allocation), DataFlow::exprNode(call.getQualifier())) and
158+
not isSpringViewPropertyWrite(call, property)
159+
)
160+
or
161+
exists(AssignExpr assign |
162+
(assign.getDest() instanceof FieldAccess or assign.getDest() instanceof ArrayAccess) and
163+
DataFlow::localFlow(DataFlow::exprNode(allocation), DataFlow::exprNode(assign.getSource()))
164+
)
165+
or
166+
exists(ArrayInit init |
167+
DataFlow::localFlow(DataFlow::exprNode(allocation), DataFlow::exprNode(init.getAnInit()))
168+
)
169+
or
170+
exists(MemberRefExpr reference |
171+
DataFlow::localFlow(DataFlow::exprNode(allocation),
172+
DataFlow::exprNode(reference.getReceiverExpr()))
173+
)
174+
or
175+
exists(LocalScopeVariable variable, VarAccess access |
176+
access.getVariable() = variable and
177+
DataFlow::localFlow(DataFlow::exprNode(allocation), DataFlow::exprNode(access)) and
178+
variable.getAnAccess().getEnclosingCallable() != variable.getCallable()
179+
)
180+
)
181+
}
182+
183+
/** Holds if a constant property replacement completes before this particular returned view. */
184+
private predicate viewPropertyReplacedBeforeReturn(
185+
Call write, Expr value, ReturnStmt ret, string property
186+
) {
187+
exists(ClassInstanceExpr allocation, MethodCall replacement |
188+
allocation = getViewAllocation(value) and
189+
allocation = getViewAllocation(ret.getExpr()) and
190+
not viewEscapes(allocation, property) and
191+
(
192+
property = "url" and
193+
allocation
194+
.getConstructedType()
195+
.hasQualifiedName("org.springframework.web.servlet.view", "RedirectView")
196+
or
197+
property = "view" and
198+
allocation
199+
.getConstructedType()
200+
.hasQualifiedName("org.springframework.web.servlet", "ModelAndView")
201+
) and
202+
isSpringViewPropertyWrite(replacement, property) and
203+
exists(replacement.getArgument(0).(CompileTimeConstantExpr).getStringValue()) and
204+
getViewAllocation(replacement.getQualifier()) = allocation and
205+
not allocation.getParent*() instanceof LoopStmt and
206+
not replacement.getParent*() instanceof LoopStmt and
207+
not ret.getParent*() instanceof LoopStmt and
208+
strictlyDominates(write.getControlFlowNode(), replacement.getControlFlowNode()) and
209+
dominates(replacement.getControlFlowNode(), ret.getControlFlowNode()) and
210+
dominates(replacement.getControlFlowNode().getANormalSuccessor(), ret.getControlFlowNode()) and
211+
// A later or unordered write may replace the constant again. Possible aliases suffice here.
212+
not exists(MethodCall other |
213+
other != replacement and
214+
isSpringViewPropertyWrite(other, property) and
215+
DataFlow::localFlow(DataFlow::exprNode(allocation), DataFlow::exprNode(other.getQualifier())) and
216+
not strictlyDominates(other.getControlFlowNode(), replacement.getControlFlowNode())
217+
)
102218
)
103219
}
104220

105221
/** Holds if `value` contributes to a view returned from a Spring MVC request handler. */
106222
pragma[nomagic]
107-
private predicate isSpringMvcViewResult(Expr value) {
108-
contributesToReturn(value) and
109-
value.getEnclosingCallable() instanceof SpringRequestMappingMethod and
110-
not value.getEnclosingCallable().(SpringRequestMappingMethod).isResponseBody()
223+
private predicate isSpringMvcViewResult(Expr value, Call write, string property) {
224+
property = ["url", "view"] and
225+
exists(ReturnStmt ret |
226+
ret.getEnclosingCallable() = value.getEnclosingCallable() and
227+
ret.getEnclosingCallable() instanceof SpringRequestMappingMethod and
228+
DataFlow::localFlow(DataFlow::exprNode(value), DataFlow::exprNode(ret.getExpr())) and
229+
not viewPropertyReplacedBeforeReturn(write, value, ret, property)
230+
)
111231
}
112232

113-
/** Holds if `value` contributes string content to a view name returned by a request handler. */
114-
private predicate isSpringMvcReturnedString(Expr value) {
115-
contributesToReturnedString(value) and
116-
value.getEnclosingCallable() instanceof SpringRequestMappingMethod and
117-
not value.getEnclosingCallable().(SpringRequestMappingMethod).isResponseBody()
233+
/** Holds if `name` is a string view name returned by a request handler. */
234+
private predicate isSpringMvcReturnedString(Expr name) {
235+
exists(ReturnStmt ret, SpringRequestMappingMethod handler |
236+
handler = name.getEnclosingCallable() and
237+
ret.getEnclosingCallable() = handler and
238+
not handler.isResponseBody() and
239+
not handler instanceof SpringModelAttributeMethod and
240+
name = ret.getExpr()
241+
)
118242
}
119243

120-
/** Holds if `value` contributes to the view name of a returned `ModelAndView`. */
121-
private predicate isSpringModelAndViewName(Expr value) {
244+
/** Holds if `name` is the string view name of a returned `ModelAndView`. */
245+
private predicate isSpringModelAndViewName(Expr name) {
122246
exists(ClassInstanceExpr newModelAndView |
123247
newModelAndView
124248
.getConstructedType()
125249
.hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
126-
TaintTracking::localTaint(DataFlow::exprNode(value),
127-
DataFlow::exprNode(newModelAndView.getArgument(0))) and
128-
isSpringMvcViewResult(newModelAndView)
250+
newModelAndView.getConstructor().getParameter(0).getType() instanceof TypeString and
251+
name = newModelAndView.getArgument(0) and
252+
isSpringMvcViewResult(newModelAndView, newModelAndView, "view")
129253
)
130254
}
131255

132-
/** Holds if `callable` returns a view name constructed with the Spring `"redirect:"` prefix. */
256+
/** Holds if a name is constant or is constructed from the exact redirect prefix. */
257+
private predicate isConstantOrRedirectViewName(Expr name) {
258+
exists(getLocalViewValue(name).(CompileTimeConstantExpr).getStringValue())
259+
or
260+
getViewNamePrefix(name) instanceof SpringRedirectPrefix
261+
}
262+
263+
/** Holds if `callable` returns redirect names without mixing in other dynamic view names. */
133264
pragma[nomagic]
134265
private predicate returnsRedirectViewName(Callable callable) {
135-
exists(DataFlow::ExprNode appended, ReturnStmt ret |
136-
appendedToRedirectPrefix(appended) and
137-
appended.asExpr().getEnclosingCallable() = callable and
266+
exists(SpringRedirectPrefix prefix, DataFlow::ExprNode appended, ReturnStmt ret |
267+
callable.(Method).getReturnType() instanceof TypeString and
268+
forall(ReturnStmt r | r.getEnclosingCallable() = callable |
269+
isConstantOrRedirectViewName(r.getExpr())
270+
) and
138271
ret.getEnclosingCallable() = callable and
272+
prefix = getViewNamePrefix(ret.getExpr()) and
273+
appendedToRedirectPrefix(prefix, appended) and
139274
TaintTracking::localTaint(appended, DataFlow::exprNode(ret.getExpr()))
140275
)
141276
or
142-
exists(MethodCall call |
143-
call.getEnclosingCallable() = callable and
144-
contributesToReturn(call) and
277+
exists(MethodCall call, ReturnStmt ret |
278+
callable.(Method).getReturnType() instanceof TypeString and
279+
ret = unique(ReturnStmt r | r.getEnclosingCallable() = callable) and
280+
call = getViewNamePrefix(ret.getExpr()) and
145281
returnsRedirectViewName(call.getCallee().getSourceDeclaration())
146282
)
147283
}
@@ -153,7 +289,7 @@ private class SpringRedirectViewSink extends UrlRedirectSink {
153289
newRedirectView
154290
.getConstructedType()
155291
.hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and
156-
isSpringMvcViewResult(newRedirectView) and
292+
isSpringMvcViewResult(newRedirectView, newRedirectView, "url") and
157293
this.asExpr() = newRedirectView.getArgument(0)
158294
)
159295
}
@@ -173,7 +309,7 @@ private class SpringRedirectViewSetUrlSink extends UrlRedirectSink {
173309
.(RefType)
174310
.getASupertype*()
175311
.hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and
176-
isSpringMvcViewResult(setUrl.getQualifier()) and
312+
isSpringMvcViewResult(setUrl.getQualifier(), setUrl, "url") and
177313
this.asExpr() = setUrl.getArgument(0)
178314
)
179315
}

0 commit comments

Comments
 (0)