Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions java/ql/src/experimental/Security/CWE/CWE-094/FlowUtils.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import java
import semmle.code.java.dataflow.FlowSources

/**
* Holds if `fromNode` to `toNode` is a dataflow step that returns data from
* a bean by calling one of its getters.
*/
predicate hasGetterFlow(DataFlow::Node fromNode, DataFlow::Node toNode) {
exists(MethodAccess ma, Method m | ma.getMethod() = m |
m instanceof GetterMethod and
ma.getQualifier() = fromNode.asExpr() and
ma = toNode.asExpr()
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
<qhelp>

<overview>
<p>
Jakarta Expression Language (EL) is an expression language for Java applications.
There is a single language specification and multiple implementations
such as Glassfish, Juel, Apache Commons EL, etc.
The language allows invocation of methods available in the JVM.
If an expression is built using attacker-controlled data,
and then evaluated, it may allow the attacker to run arbitrary code.
</p>
</overview>

<recommendation>
<p>
It is generally recommended to avoid using untrusted data in an EL expression.
Before using untrusted data to build an EL expression, the data should be validated
to ensure it is not evaluated as expression language. If the EL implementation offers
configuring a sandbox for EL expressions, they should be run in a restrictive sandbox
that allows accessing only explicitly allowed classes. If the EL implementation
does not support sandboxing, consider using other expression language implementations
with sandboxing capabilities such as Apache Commons JEXL or the Spring Expression Language.
</p>
</recommendation>

<example>
<p>
The following example shows how untrusted data is used to build and run an expression
using the JUEL interpreter:
</p>
<sample src="UnsafeExpressionEvaluationWithJuel.java" />

<p>
JUEL does not support running expressions in a sandbox. To prevent running arbitrary code,
incoming data has to be checked before including it in an expression. The next example
uses a Regex pattern to check whether a user tries to run an allowed expression or not:
</p>
<sample src="SaferExpressionEvaluationWithJuel.java" />

</example>

<references>
<li>
Eclipse Foundation:
<a href="https://projects.eclipse.org/projects/ee4j.el">Jakarta Expression Language</a>.
</li>
<li>
Jakarta EE documentation:
<a href="https://javadoc.io/doc/jakarta.el/jakarta.el-api/latest/index.html">Jakarta Expression Language API</a>
</li>
<li>
OWASP:
<a href="https://owasp.org/www-community/vulnerabilities/Expression_Language_Injection">Expression Language Injection</a>.
</li>
<li>
JUEL:
<a href="http://juel.sourceforge.net">Home page</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No this page does not seem to support HTTPS; in case someone else asked themselves this question)

</li>
</references>
</qhelp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* @name Jakarta Expression Language injection
* @description Evaluation of a user-controlled expression
* may lead to arbitrary code execution.
* @kind path-problem
* @problem.severity error
* @precision high
* @id java/javaee-expression-injection
* @tags security
* external/cwe/cwe-094
*/

import java
import JakartaExpressionInjectionLib
import DataFlow::PathGraph

from DataFlow::PathNode source, DataFlow::PathNode sink, JakartaExpressionInjectionConfig conf
where conf.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Jakarta Expression Language injection from $@.",
source.getNode(), "this user input"
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import java
import FlowUtils
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.dataflow.TaintTracking

/**
* A taint-tracking configuration for unsafe user input
* that is used to construct and evaluate an expression.
*/
class JakartaExpressionInjectionConfig extends TaintTracking::Configuration {
JakartaExpressionInjectionConfig() { this = "JakartaExpressionInjectionConfig" }

override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }

override predicate isSink(DataFlow::Node sink) { sink instanceof ExpressionEvaluationSink }

override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
any(TaintPropagatingCall c).taintFlow(fromNode, toNode) or
hasGetterFlow(fromNode, toNode)
}
}

/**
* A sink for Expresssion Language injection vulnerabilities,
* i.e. method calls that run evaluation of an expression.
*/
private class ExpressionEvaluationSink extends DataFlow::ExprNode {
ExpressionEvaluationSink() {
exists(MethodAccess ma, Method m, Expr taintFrom |
ma.getMethod() = m and taintFrom = this.asExpr()
|
m.getDeclaringType() instanceof ValueExpression and
m.hasName(["getValue", "setValue"]) and
ma.getQualifier() = taintFrom
or
m.getDeclaringType() instanceof MethodExpression and
m.hasName("invoke") and
ma.getQualifier() = taintFrom
or
m.getDeclaringType() instanceof LambdaExpression and
m.hasName("invoke") and
ma.getQualifier() = taintFrom
or
m.getDeclaringType() instanceof ELProcessor and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defineFunction looks also somewhat dangerous. If I understand it correctly it allows you to defined a function referring to any static method (assuming className or method is user-controlled).

(Though based on the source code it looks like it cannot be used to initialize a gadget class without further actions.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it definitely looks somewhat dangerous. If someone can control class and method names, then it can call the method via the specified function name. Then, the attacker has two options. The first option is to use the function name in an expression that the attacker controls. That's not interesting because the attacker can already run arbitrary expression. The second option is to wait for the function to be called later. The attacker will need to control the arguments. Or, maybe the function has no arguments. This sounds a bit difficult to exploit although it doesn't seem to be impossible. Anyway, the risk looks pretty low to me. If no objections, let's not add defineFunction as a sink. Please let me know if you find a good scenario how defineFunction can be exploited and I'll update the query. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more of the second option. The application allows the user to specify method and maybe also class name and then calls the method itself . For some dangerous methods the argument value would not even matter, e.g. for System.exit(int) regardless of what int value the application uses and what its intended meaning is, it is a valid argument for the exit call and will stop the JVM.

Though such a scenario is rather unlikely. On the other hand if the user is able to affect the class name of defineFunction in any way, then this could already be a security concern, regardless of how likely it is. My feeling is that false positives are rather unlikely there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also cover setVariable? It does not evaluate the expression immediately, but maybe a user is able to replace a variable which is then later referenced by the application. (though I am not completely sure regarding this)

Copy link
Contributor Author

@artem-smotrakov artem-smotrakov Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about covering setVariable in this query but then I didn't add it because it doesn't evaluate the expression as you noticed. On the other hand, we can assume that the injected code is likely to be run a bit later when the variable is used. I've added this sink. Thanks for the suggestion!

m.hasName(["eval", "getValue", "setValue"]) and
ma.getArgument(0) = taintFrom
or
m.getDeclaringType() instanceof ELProcessor and
m.hasName("setVariable") and
ma.getArgument(1) = taintFrom
)
}
}

/**
* Defines method calls that propagate tainted expressions.
*/
private class TaintPropagatingCall extends Call {
Expr taintFromExpr;

TaintPropagatingCall() {
taintFromExpr = this.getArgument(1) and
(
exists(Method m | this.(MethodAccess).getMethod() = m |
m.getDeclaringType() instanceof ExpressionFactory and
m.hasName(["createValueExpression", "createMethodExpression"]) and
taintFromExpr.getType() instanceof TypeString
)
or
exists(Constructor c | this.(ConstructorCall).getConstructor() = c |
c.getDeclaringType() instanceof LambdaExpression and
taintFromExpr.getType() instanceof ValueExpression
)
)
}

/**
* Holds if `fromNode` to `toNode` is a dataflow step that propagates
* tainted data.
*/
predicate taintFlow(DataFlow::Node fromNode, DataFlow::Node toNode) {
fromNode.asExpr() = taintFromExpr and toNode.asExpr() = this
}
}

private class JakartaType extends RefType {
JakartaType() { getPackage().hasName(["javax.el", "jakarta.el"]) }
}

private class ELProcessor extends JakartaType {
ELProcessor() { hasName("ELProcessor") }
}

private class ExpressionFactory extends JakartaType {
ExpressionFactory() { hasName("ExpressionFactory") }
}

private class ValueExpression extends JakartaType {
ValueExpression() { hasName("ValueExpression") }
}

private class MethodExpression extends JakartaType {
MethodExpression() { hasName("MethodExpression") }
}

private class LambdaExpression extends JakartaType {
LambdaExpression() { hasName("LambdaExpression") }
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import java
import FlowUtils
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.dataflow.TaintTracking

Expand All @@ -16,7 +17,7 @@ class JexlInjectionConfig extends TaintTracking::Configuration {

override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
any(TaintPropagatingJexlMethodCall c).taintFlow(fromNode, toNode) or
returnsDataFromBean(fromNode, toNode)
hasGetterFlow(fromNode, toNode)
}
}

Expand Down Expand Up @@ -152,18 +153,6 @@ private predicate createsJexlEngine(DataFlow::Node fromNode, DataFlow::Node toNo
)
}

/**
* Holds if `fromNode` to `toNode` is a dataflow step that returns data from
* a bean by calling one of its getters.
*/
private predicate returnsDataFromBean(DataFlow::Node fromNode, DataFlow::Node toNode) {
exists(MethodAccess ma, Method m | ma.getMethod() = m |
m instanceof GetterMethod and
ma.getQualifier() = fromNode.asExpr() and
ma = toNode.asExpr()
)
}

/**
* A methods in the `JexlEngine` class that gets or sets a property with a JEXL expression.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
String input = getRemoteUserInput();
String pattern = "(inside|outside)\\.(temperature|humidity)";
if (!input.matches(pattern)) {
throw new IllegalArgumentException("Unexpected expression");
}
String expression = "${" + input + "}";
ExpressionFactory factory = new de.odysseus.el.ExpressionFactoryImpl();
ValueExpression e = factory.createValueExpression(context, expression, Object.class);
SimpleContext context = getContext();
Object result = e.getValue(context);
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
String expression = "${" + getRemoteUserInput() + "}";
ExpressionFactory factory = new de.odysseus.el.ExpressionFactoryImpl();
ValueExpression e = factory.createValueExpression(context, expression, Object.class);
SimpleContext context = getContext();
Object result = e.getValue(context);
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
edges
| JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:25:31:25:40 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:32:24:32:33 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:40:24:40:33 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:48:24:48:33 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:59:24:59:33 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:67:24:67:33 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:75:24:75:33 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:85:24:85:33 | expression : String |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | JakartaExpressionInjection.java:95:24:95:33 | expression : String |
| JakartaExpressionInjection.java:32:24:32:33 | expression : String | JakartaExpressionInjection.java:34:28:34:37 | expression |
| JakartaExpressionInjection.java:40:24:40:33 | expression : String | JakartaExpressionInjection.java:42:32:42:41 | expression |
| JakartaExpressionInjection.java:48:24:48:33 | expression : String | JakartaExpressionInjection.java:53:13:53:28 | lambdaExpression |
| JakartaExpressionInjection.java:59:24:59:33 | expression : String | JakartaExpressionInjection.java:61:32:61:41 | expression |
| JakartaExpressionInjection.java:67:24:67:33 | expression : String | JakartaExpressionInjection.java:69:43:69:52 | expression |
| JakartaExpressionInjection.java:75:24:75:33 | expression : String | JakartaExpressionInjection.java:79:13:79:13 | e |
| JakartaExpressionInjection.java:85:24:85:33 | expression : String | JakartaExpressionInjection.java:89:13:89:13 | e |
| JakartaExpressionInjection.java:95:24:95:33 | expression : String | JakartaExpressionInjection.java:99:13:99:13 | e |
nodes
| JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
| JakartaExpressionInjection.java:25:31:25:40 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:32:24:32:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:34:28:34:37 | expression | semmle.label | expression |
| JakartaExpressionInjection.java:40:24:40:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:42:32:42:41 | expression | semmle.label | expression |
| JakartaExpressionInjection.java:48:24:48:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:53:13:53:28 | lambdaExpression | semmle.label | lambdaExpression |
| JakartaExpressionInjection.java:59:24:59:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:61:32:61:41 | expression | semmle.label | expression |
| JakartaExpressionInjection.java:67:24:67:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:69:43:69:52 | expression | semmle.label | expression |
| JakartaExpressionInjection.java:75:24:75:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:79:13:79:13 | e | semmle.label | e |
| JakartaExpressionInjection.java:85:24:85:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:89:13:89:13 | e | semmle.label | e |
| JakartaExpressionInjection.java:95:24:95:33 | expression : String | semmle.label | expression : String |
| JakartaExpressionInjection.java:99:13:99:13 | e | semmle.label | e |
#select
| JakartaExpressionInjection.java:34:28:34:37 | expression | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:34:28:34:37 | expression | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
| JakartaExpressionInjection.java:42:32:42:41 | expression | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:42:32:42:41 | expression | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
| JakartaExpressionInjection.java:53:13:53:28 | lambdaExpression | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:53:13:53:28 | lambdaExpression | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
| JakartaExpressionInjection.java:61:32:61:41 | expression | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:61:32:61:41 | expression | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
| JakartaExpressionInjection.java:69:43:69:52 | expression | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:69:43:69:52 | expression | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
| JakartaExpressionInjection.java:79:13:79:13 | e | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:79:13:79:13 | e | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
| JakartaExpressionInjection.java:89:13:89:13 | e | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:89:13:89:13 | e | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
| JakartaExpressionInjection.java:99:13:99:13 | e | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) : InputStream | JakartaExpressionInjection.java:99:13:99:13 | e | Jakarta Expression Language injection from $@. | JakartaExpressionInjection.java:23:25:23:47 | getInputStream(...) | this user input |
Loading