Skip to content

Commit 602f63a

Browse files
author
Porcupiney Hairs
committed
[Java] Add QL for detecting Spring View Manipulation Vulnerabilities.
1 parent ac67c67 commit 602f63a

File tree

10 files changed

+336
-0
lines changed

10 files changed

+336
-0
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.qhelp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
<include src="SpringViewManipulation.qhelp" />
4+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.ql

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/**
2+
* @name Spring Implicit View Manipulation
3+
* @description Untrusted input in a Spring View Controller can lead to RCE.
4+
* @kind problem
5+
* @problem.severity error
6+
* @precision high
7+
* @id java/spring-view-manipulation-implicit
8+
* @tags security
9+
* external/cwe/cwe-094
10+
*/
11+
12+
import java
13+
import SpringViewManipulationLib
14+
15+
private predicate canResultInImplicitViewConversion(Method m) {
16+
m.getReturnType() instanceof VoidType
17+
or
18+
m.getReturnType() instanceof MapType
19+
or
20+
m.getReturnType().(RefType).hasQualifiedName("org.springframework.ui", "Model")
21+
}
22+
23+
private predicate maybeATestMethod(Method m) {
24+
exists(string s |
25+
s = m.getName() or
26+
s = m.getFile().getRelativePath() or
27+
s = m.getDeclaringType().getName()
28+
|
29+
s.matches(["%test%", "%example%", "%exception%"])
30+
)
31+
}
32+
33+
private predicate mayBeExploitable(Method m) {
34+
// There should be a attacker controlled parameter in the URI for the attack to be exploitable.
35+
// This is possible only when there exists a parameter with the Spring `@PathVariable` annotation
36+
// applied to it.
37+
exists(Parameter p |
38+
p = m.getAParameter() and
39+
p.hasAnnotation("org.springframework.web.bind.annotation", "PathVariable") and
40+
// Having a parameter of say type `Long` is non exploitable as Java type
41+
// checking rules are applied prior to view name resolution, rendering the exploit useless.
42+
// hence, here we check for the param type to be a Java `String`.
43+
p.getType() instanceof TypeString and
44+
// Exclude cases where a regex check is applied on a parameter to prevent false positives.
45+
not m.(SpringRequestMappingMethod).getValue().matches("%{%:[%]%}%")
46+
) and
47+
not maybeATestMethod(m)
48+
}
49+
50+
from SpringRequestMappingMethod m
51+
where
52+
thymeleafIsUsed() and
53+
mayBeExploitable(m) and
54+
canResultInImplicitViewConversion(m) and
55+
// If there's a parameter of type`HttpServletResponse`, Spring Framework does not interpret
56+
// it as a view name, but just returns this string in HTTP Response preventing exploitation
57+
// This also applies to `@ResponseBody` annotation.
58+
not m.getParameterType(_) instanceof HttpServletResponse and
59+
// A spring request mapping method which does not have response body annotation applied to it
60+
m.getAnAnnotation().getType() instanceof SpringRequestMappingAnnotationType and
61+
not exists(SpringResponseBodyAnnotationType t | t = m.getAnAnnotation().getType()) and
62+
// `@RestController` inherits `@ResponseBody` internally so it should be ignored.
63+
not m.getDeclaringType() instanceof SpringRestController
64+
select m, "This method may be vulnerable to spring view manipulation vulnerabilities"

‎java/ql/src/experimental/Security/CWE/CWE-094/SpringViewBad.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
@Controller
2+
public class SptingViewManipulationController {
3+
4+
Logger log = LoggerFactory.getLogger(HelloController.class);
5+
6+
@GetMapping("/safe/fragment")
7+
public String Fragment(@RequestParam String section) {
8+
// bad as template path is attacker controlled
9+
return "welcome :: " + section;
10+
}
11+
12+
@GetMapping("/doc/{document}")
13+
public void getDocument(@PathVariable String document) {
14+
// returns void, so view name is taken from URI
15+
log.info("Retrieving " + document);
16+
}
17+
}

‎java/ql/src/experimental/Security/CWE/CWE-094/SpringViewGood.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
@Controller
2+
public class SptingViewManipulationController {
3+
4+
Logger log = LoggerFactory.getLogger(HelloController.class);
5+
6+
@GetMapping("/safe/fragment")
7+
@ResponseBody
8+
public String Fragment(@RequestParam String section) {
9+
// good, as `@ResponseBody` annotation tells Spring
10+
// to process the return values as body, instead of view name
11+
return "welcome :: " + section;
12+
}
13+
14+
@GetMapping("/safe/doc/{document}")
15+
public void getDocument(@PathVariable String document, HttpServletResponse response) {
16+
// good as `HttpServletResponse param tells Spring that the response is already
17+
// processed.
18+
log.info("Retrieving " + document); // FP
19+
}
20+
}

‎java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.qhelp

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
The Spring Expression Language (SpEL) is a powerful expression language
7+
provided by Spring Framework. The language offers many features
8+
including invocation of methods available in the JVM.
9+
</p>
10+
<p>
11+
An unrestricted view name manipulation vulnerability in Spring Framework could lead to attacker-controlled arbitary SpEL expressions being evaluated using attacker-controlled data, which may in turn allow an attacker to run arbitrary code.
12+
</p>
13+
<p>
14+
Note: two related variants of this problem are detected by different queries, `java/spring-view-manipulation` and `java/spring-view-manipulation-implicit`. The first detects taint flow problems where the return types is always <code>String</code>. While the latter, `java/spring-view-manipulation-implicit` detects cases where the request mapping method has a non-string return type such as <code>void</code>.
15+
</p>
16+
</overview>
17+
18+
<recommendation>
19+
<p>
20+
In general, using user input to determine Spring view name should be avoided.
21+
If user input must be included in the expression, the controller can be annotated by
22+
a <code>@ReponseBody</code> annotation. In this case, Spring Framework does not interpret
23+
it as a view name, but just returns this string in HTTP Response. The same applies to using
24+
a <code>@RestController</code> annotation on a class, as internally it inherits <code>@ResponseBody</code>.
25+
</p>
26+
</recommendation>
27+
28+
<example>
29+
<p>
30+
In the following example, the <code>Fragment</code> method uses an externally controlled variable <code>section</code> to generate the view name. Hence, it is vulnerable to Spring View Manipulation attacks.
31+
</p>
32+
<sample src="SpringViewBad.java" />
33+
<p>
34+
This can be easily prevented by using the <code>ResponseBody</code> annotation which marks the reponse is already processed preventing exploitation of Spring View Manipulation vulnerabilities. Alternatively, this can also be fixed by adding a <code>HttpServletResponse</code> parameter to the method definition as shown in the example below.
35+
</p>
36+
<sample src="SpringViewGood.java" />
37+
</example>
38+
39+
<references>
40+
<li>
41+
Veracode Research : <a href="https://github.com/veracode-research/spring-view-manipulation/">Spring View Manipulation </a>
42+
</li>
43+
<li>
44+
Spring Framework Reference Documentation: <a href="https://docs.spring.io/spring/docs/4.2.x/spring-framework-reference/html/expressions.html">Spring Expression Language (SpEL)</a>
45+
</li>
46+
<li>
47+
OWASP: <a href="https://owasp.org/www-community/vulnerabilities/Expression_Language_Injection">Expression Language Injection</a>
48+
</li>
49+
</references>
50+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.ql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Spring View Manipulation
3+
* @description Untrusted input in a Spring View can lead to RCE.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @precision high
7+
* @id java/spring-view-manipulation
8+
* @tags security
9+
* external/cwe/cwe-094
10+
*/
11+
12+
import java
13+
import SpringViewManipulationLib
14+
import DataFlow::PathGraph
15+
16+
from DataFlow::PathNode source, DataFlow::PathNode sink, SpringViewManipulationConfig conf
17+
where
18+
thymeleafIsUsed() and
19+
conf.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink, "Potential Spring Expression Language injection from $@.",
21+
source.getNode(), "this user input"

‎java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
/**
2+
* Provides classes for reasoning about Spring View Manipulation vulnerabilities
3+
*/
4+
5+
import java
6+
import semmle.code.java.dataflow.FlowSources
7+
import semmle.code.java.dataflow.TaintTracking
8+
import semmle.code.java.frameworks.spring.Spring
9+
import SpringFrameworkLib
10+
11+
/** Holds if `Thymeleaf` templating engine is used in the project. */
12+
predicate thymeleafIsUsed() {
13+
exists(Pom p |
14+
p.getADependency().getArtifact().getValue() in [
15+
"spring-boot-starter-thymeleaf", "thymeleaf-spring4", "springmvc-xml-thymeleaf",
16+
"thymeleaf-spring5"
17+
]
18+
)
19+
or
20+
exists(SpringBean b | b.getClassNameRaw().matches("org.thymeleaf.spring%"))
21+
}
22+
23+
/**
24+
* A taint-tracking configuration for unsafe user input
25+
* that can lead to Spring View Manipulation vulnerabilities.
26+
*/
27+
class SpringViewManipulationConfig extends TaintTracking::Configuration {
28+
SpringViewManipulationConfig() { this = "Spring View Manipulation Config" }
29+
30+
override predicate isSource(DataFlow::Node source) {
31+
source instanceof RemoteFlowSource or
32+
source instanceof WebRequestSource
33+
}
34+
35+
override predicate isSink(DataFlow::Node sink) { sink instanceof SpringViewManipulationSink }
36+
37+
override predicate isSanitizer(DataFlow::Node node) {
38+
// Block flows like
39+
// ```
40+
// a = "redirect:" + taint`
41+
// ```
42+
exists(AddExpr e, StringLiteral sl |
43+
node.asExpr() = e.getControlFlowNode().getASuccessor*() and
44+
sl = e.getLeftOperand*() and
45+
sl.getRepresentedString().matches(["redirect:%", "ajaxredirect:%", "forward:%"])
46+
)
47+
or
48+
// Block flows like
49+
// ```
50+
// x.append("redirect:");
51+
// x.append(tainted());
52+
// return x.toString();
53+
//
54+
// "redirect:".concat(taint)
55+
//
56+
// String.format("redirect:%s",taint);
57+
// ```
58+
exists(Call ca, StringLiteral sl |
59+
(
60+
sl = ca.getArgument(_)
61+
or
62+
sl = ca.getQualifier()
63+
) and
64+
ca = getAStringCombiningCall() and
65+
sl.getRepresentedString().matches(["redirect:%", "ajaxredirect:%", "forward:%"])
66+
|
67+
exists(Call cc | DataFlow::localExprFlow(ca.getQualifier(), cc.getQualifier()) |
68+
cc = node.asExpr()
69+
)
70+
)
71+
}
72+
}
73+
74+
private Call getAStringCombiningCall() {
75+
exists(StringCombiningMethod m | result = m.getAReference())
76+
}
77+
78+
abstract private class StringCombiningMethod extends Method { }
79+
80+
private class AppendableAppendMethod extends StringCombiningMethod {
81+
AppendableAppendMethod() {
82+
exists(RefType t |
83+
t.hasQualifiedName("java.lang", "Appendable") and
84+
this.getDeclaringType().extendsOrImplements*(t) and
85+
this.hasName("append")
86+
)
87+
}
88+
}
89+
90+
private class StringConcatMethod extends StringCombiningMethod {
91+
StringConcatMethod() {
92+
this.getDeclaringType() instanceof TypeString and
93+
this.hasName("concat")
94+
}
95+
}
96+
97+
private class StringFormatMethod extends StringCombiningMethod {
98+
StringFormatMethod() {
99+
this.getDeclaringType() instanceof TypeString and
100+
this.hasName("format")
101+
}
102+
}
103+
104+
/**
105+
* A sink for Spring View Manipulation vulnerabilities,
106+
*/
107+
class SpringViewManipulationSink extends DataFlow::ExprNode {
108+
SpringViewManipulationSink() {
109+
exists(ReturnStmt r, SpringRequestMappingMethod m |
110+
r.getResult() = this.asExpr() and
111+
m.getBody().getAStmt() = r and
112+
not m.isResponseBody() and
113+
r.getResult().getType() instanceof TypeString
114+
)
115+
or
116+
exists(ConstructorCall c | c.getConstructedType() instanceof ModelAndView |
117+
this.asExpr() = c.getArgument(0) and
118+
c.getConstructor().getParameterType(0) instanceof TypeString
119+
)
120+
or
121+
exists(SpringModelAndViewSetViewNameCall c | this.asExpr() = c.getArgument(0))
122+
}
123+
}

‎java/ql/src/semmle/code/java/dataflow/FlowSources.qll

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ private class RemoteTaintedMethod extends Method {
256256
this instanceof ServletRequestGetParameterMethod or
257257
this instanceof ServletRequestGetParameterMapMethod or
258258
this instanceof ServletRequestGetParameterNamesMethod or
259+
this instanceof PortletRenderRequestGetParameterMethod or
259260
this instanceof HttpServletRequestGetQueryStringMethod or
260261
this instanceof HttpServletRequestGetHeaderMethod or
261262
this instanceof HttpServletRequestGetPathMethod or
@@ -308,6 +309,21 @@ class EnvReadMethod extends Method {
308309
}
309310
}
310311

312+
private class PortletRenderRequestGetParameterMethod extends Method {
313+
PortletRenderRequestGetParameterMethod() {
314+
exists(RefType c, Interface t |
315+
c.extendsOrImplements*(t) and
316+
t.hasQualifiedName("javax.portlet", "RenderState") and
317+
this = c.getAMethod()
318+
|
319+
this.hasName([
320+
"getCookies", "getParameter", "getRenderParameters", "getParameterNames",
321+
"getParameterValues", "getParameterMap"
322+
])
323+
)
324+
}
325+
}
326+
311327
/** The type `java.net.InetAddress`. */
312328
class TypeInetAddr extends RefType {
313329
TypeInetAddr() { this.getQualifiedName() = "java.net.InetAddress" }

‎java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,11 @@ class SpringRequestMappingMethod extends SpringControllerMethod {
126126
requestMappingAnnotation.getValue("produces").(CompileTimeConstantExpr).getStringValue()
127127
}
128128

129+
/** Gets the "value" @RequestMapping annotation value, if present. */
130+
string getValue() {
131+
result = requestMappingAnnotation.getValue("value").(CompileTimeConstantExpr).getStringValue()
132+
}
133+
129134
/** Holds if this is considered an `@ResponseBody` method. */
130135
predicate isResponseBody() {
131136
getAnAnnotation().getType() instanceof SpringResponseBodyAnnotationType or

‎java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,19 @@ class SpringNativeWebRequest extends Class {
1717
this.hasQualifiedName("org.springframework.web.context.request", "NativeWebRequest")
1818
}
1919
}
20+
21+
/** Models Spring `servlet` as well as `portlet` package's `ModelAndView` class. */
22+
class ModelAndView extends Class {
23+
ModelAndView() {
24+
hasQualifiedName(["org.springframework.web.servlet", "org.springframework.web.portlet"],
25+
"ModelAndView")
26+
}
27+
}
28+
29+
/** Models a call to the Spring `ModelAndView` class's `setViewName` method. */
30+
class SpringModelAndViewSetViewNameCall extends MethodAccess {
31+
SpringModelAndViewSetViewNameCall() {
32+
getMethod().getDeclaringType() instanceof ModelAndView and
33+
getMethod().hasName("setViewName")
34+
}
35+
}

0 commit comments

Comments
 (0)