Skip to content

Commit b0fa8df

Browse files
aschackmullaschackmull
authored
Merge pull request github#4214 from porcupineyhairs/springViewManipulation
[Java] Add QL for detecting Spring View Manipulation Vulnerabilities.
2 parents 2957131 + 394c82d commit b0fa8df

File tree

9 files changed

+341
-0
lines changed

9 files changed

+341
-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: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
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+
/** Models methods from the `javax.portlet.RenderState` package which return data from externally controlled sources. */
24+
class PortletRenderRequestMethod extends Method {
25+
PortletRenderRequestMethod() {
26+
exists(RefType c, Interface t |
27+
c.extendsOrImplements*(t) and
28+
t.hasQualifiedName("javax.portlet", "RenderState") and
29+
this = c.getAMethod()
30+
|
31+
this.hasName([
32+
"getCookies", "getParameter", "getRenderParameters", "getParameterNames",
33+
"getParameterValues", "getParameterMap"
34+
])
35+
)
36+
}
37+
}
38+
39+
/**
40+
* A taint-tracking configuration for unsafe user input
41+
* that can lead to Spring View Manipulation vulnerabilities.
42+
*/
43+
class SpringViewManipulationConfig extends TaintTracking::Configuration {
44+
SpringViewManipulationConfig() { this = "Spring View Manipulation Config" }
45+
46+
override predicate isSource(DataFlow::Node source) {
47+
source instanceof RemoteFlowSource or
48+
source instanceof WebRequestSource or
49+
source.asExpr().(MethodAccess).getMethod() instanceof PortletRenderRequestMethod
50+
}
51+
52+
override predicate isSink(DataFlow::Node sink) { sink instanceof SpringViewManipulationSink }
53+
54+
override predicate isSanitizer(DataFlow::Node node) {
55+
// Block flows like
56+
// ```
57+
// a = "redirect:" + taint`
58+
// ```
59+
exists(AddExpr e, StringLiteral sl |
60+
node.asExpr() = e.getControlFlowNode().getASuccessor*() and
61+
sl = e.getLeftOperand*() and
62+
sl.getRepresentedString().matches(["redirect:%", "ajaxredirect:%", "forward:%"])
63+
)
64+
or
65+
// Block flows like
66+
// ```
67+
// x.append("redirect:");
68+
// x.append(tainted());
69+
// return x.toString();
70+
//
71+
// "redirect:".concat(taint)
72+
//
73+
// String.format("redirect:%s",taint);
74+
// ```
75+
exists(Call ca, StringLiteral sl |
76+
(
77+
sl = ca.getArgument(_)
78+
or
79+
sl = ca.getQualifier()
80+
) and
81+
ca = getAStringCombiningCall() and
82+
sl.getRepresentedString().matches(["redirect:%", "ajaxredirect:%", "forward:%"])
83+
|
84+
exists(Call cc | DataFlow::localExprFlow(ca.getQualifier(), cc.getQualifier()) |
85+
cc = node.asExpr()
86+
)
87+
)
88+
}
89+
}
90+
91+
private Call getAStringCombiningCall() {
92+
exists(StringCombiningMethod m | result = m.getAReference())
93+
}
94+
95+
abstract private class StringCombiningMethod extends Method { }
96+
97+
private class AppendableAppendMethod extends StringCombiningMethod {
98+
AppendableAppendMethod() {
99+
exists(RefType t |
100+
t.hasQualifiedName("java.lang", "Appendable") and
101+
this.getDeclaringType().extendsOrImplements*(t) and
102+
this.hasName("append")
103+
)
104+
}
105+
}
106+
107+
private class StringConcatMethod extends StringCombiningMethod {
108+
StringConcatMethod() {
109+
this.getDeclaringType() instanceof TypeString and
110+
this.hasName("concat")
111+
}
112+
}
113+
114+
private class StringFormatMethod extends StringCombiningMethod {
115+
StringFormatMethod() {
116+
this.getDeclaringType() instanceof TypeString and
117+
this.hasName("format")
118+
}
119+
}
120+
121+
/**
122+
* A sink for Spring View Manipulation vulnerabilities,
123+
*/
124+
class SpringViewManipulationSink extends DataFlow::ExprNode {
125+
SpringViewManipulationSink() {
126+
exists(ReturnStmt r, SpringRequestMappingMethod m |
127+
r.getResult() = this.asExpr() and
128+
m.getBody().getAStmt() = r and
129+
not m.isResponseBody() and
130+
r.getResult().getType() instanceof TypeString
131+
)
132+
or
133+
exists(ConstructorCall c | c.getConstructedType() instanceof ModelAndView |
134+
this.asExpr() = c.getArgument(0) and
135+
c.getConstructor().getParameterType(0) instanceof TypeString
136+
)
137+
or
138+
exists(SpringModelAndViewSetViewNameCall c | this.asExpr() = c.getArgument(0))
139+
}
140+
}

‎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: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,23 @@ class SpringNativeWebRequest extends Class {
1717
this.hasQualifiedName("org.springframework.web.context.request", "NativeWebRequest")
1818
}
1919
}
20+
21+
/**
22+
* A Spring `ModelAndView` class. This is either
23+
* `org.springframework.web.servlet.ModelAndView` or
24+
* `org.springframework.web.portlet.ModelAndView`.
25+
*/
26+
class ModelAndView extends Class {
27+
ModelAndView() {
28+
hasQualifiedName(["org.springframework.web.servlet", "org.springframework.web.portlet"],
29+
"ModelAndView")
30+
}
31+
}
32+
33+
/** A call to the Spring `ModelAndView.setViewName` method. */
34+
class SpringModelAndViewSetViewNameCall extends MethodAccess {
35+
SpringModelAndViewSetViewNameCall() {
36+
getMethod().getDeclaringType() instanceof ModelAndView and
37+
getMethod().hasName("setViewName")
38+
}
39+
}

0 commit comments

Comments
 (0)