Java: Query for detecting Jakarta Expression Language injections #5471

artem-smotrakov · 2021-03-21T18:44:03Z

Jakarta EE contains a specification for an expression language (EL) and API for interpreters. There are multiple implementations of the API, for example, JUEL, Apache Commons EL, Glassfish has its own implementation. 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 in the worst case.

I'd like to propose a new experimental query that looks for potential expression injections when an application uses the Jakarta EL.

Jakarta EE used to be Java EE in the past. Originally, the API for the EL was located in the javax.el package. After Eclipse Foundation had taken over Java EE, the package was renamed to jakarta.el. The query covers both versions.

The query detects a known RCE in OpenFaces.

- Added JakartaExpressionInjection.ql - Added a qhelp file with examples

Marcono1234

I mostly looked for typos, but I hope the review comments are useful nonetheless.
The maintainers will probably be able to perform a better content-wise review.

And slightly unrelated, in case you did not know, GitHub also supports multi-line source code links, e.g. (random example):

codeql/.lgtm.yml

Lines 5 to 6 in 34e2562


	test:

So you could adjust your comment on the OpenFaces GitHub issue.

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjection.qhelp

Marcono1234 · 2021-03-22T17:08:28Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjection.qhelp

+  Apache Foundation:
+  <a href="https://commons.apache.org/dormant/commons-el/">Apache Commons EL</a>


Might not actually be a good idea to refer to a framework in "dormant" state last released in 2003.
Maybe instead refer to JEXL?

Yeah, makes sense. I am not sure that JEXL implements the Jakarta EL. I just removed the link.

Marcono1234 · 2021-03-22T17:19:05Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjectionLib.qll

+      m.hasName("invoke") and
+      ma.getQualifier() = taintFrom
+      or
+      m.getDeclaringType() instanceof ELProcessor and


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.)

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!

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.

Marcono1234 · 2021-03-22T17:20:53Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjectionLib.qll

+      m.hasName("invoke") and
+      ma.getQualifier() = taintFrom
+      or
+      m.getDeclaringType() instanceof ELProcessor and


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)

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!

Marcono1234 · 2021-03-22T17:25:49Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjectionLib.qll

+    taintFromExpr = this.getArgument(1) and
+    exists(Method m | this.(MethodAccess).getMethod() = m |


The exists should probably be in parentheses?
Otherwise for the other exists below trainFromExpr is not bound to the call argument.

Good catch! Thanks!

java/ql/src/experimental/Security/CWE/CWE-094/SaferExpressionEvaluationWithJuel.java

tamasvajk · 2021-03-26T10:24:48Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjectionLib.qll

+  MethodExpression() { hasName("MethodExpression") }
+}
+
+private class LambdaExpression extends RefType {


Shouldn't the base type be JakartaType?

Yup. Thanks!

Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>

artem-smotrakov · 2021-04-08T17:53:45Z

@Marcono1234 @tamasvajk Thanks for the review! I've addressed your comments -- please have a look.

tamasvajk · 2021-04-12T09:37:46Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjection.qhelp

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


Could you please adjust the casing of this file reference?

I forgot to rename it here, thanks!

tamasvajk · 2021-04-12T09:37:50Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjection.qhelp

+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" />


Could you please adjust the casing of this file reference?

smowton · 2021-04-12T15:37:13Z

java/ql/src/experimental/Security/CWE/CWE-094/InjectionLib.qll

+ * Holds if `fromNode` to `toNode` is a dataflow step that returns data from
+ * a bean by calling one of its getters.
+ */
+predicate returnsDataFromBean(DataFlow::Node fromNode, DataFlow::Node toNode) {


This doesn't seem to have anything to do with beans, it's just any old getter. Rename hasGetterFlow?

Well, the predicate doesn't fully describe a Java bean but I meant a call to a getter of a bean. No problem, I renamed it to hasGetterFlow.

smowton · 2021-04-12T15:37:57Z

java/ql/src/experimental/Security/CWE/CWE-094/InjectionLib.qll

@@ -0,0 +1,14 @@
+import java


InjectionLib is an odd name for a file containing only a taint step traversing getters. Given it's query-local perhaps just Utils or FlowUtils?

Sure, I renamed it to FlowUtils.

smowton · 2021-04-12T15:40:27Z

java/ql/src/experimental/Security/CWE/CWE-094/JakartaExpressionInjection.qhelp

+<sample src="UnsafeExpressionEvaluationWithJUEL.java" />
+
+<p>
+JUEL does not support to run expressions in a sandbox. To prevent running arbitrary code,


Suggested change

JUEL does not support to run expressions in a sandbox. To prevent running arbitrary code,

JUEL does not support running expressions in a sandbox. To prevent running arbitrary code,

artem-smotrakov · 2021-04-12T18:45:15Z

@tamasvajk @smowton Thanks for your comments -- I've updated the changeset.

smowton · 2021-04-13T11:02:13Z

One last thing: could you label your tests with comments like // BAD (untrusted input to evalExpression) (when a case should be flagged) or // GOOD (untrusted input validated by xyz) when it shouldn't?

artem-smotrakov · 2021-04-14T16:32:19Z

One last thing: could you label your tests with comments like // BAD (untrusted input to evalExpression) (when a case should be flagged) or // GOOD (untrusted input validated by xyz) when it shouldn't?

@smowton Sure, I've updated the tests.

Changes applied

artem-smotrakov added 3 commits March 21, 2021 21:19

Added query for Jakarta EL injections

73e940d

- Added JakartaExpressionInjection.ql - Added a qhelp file with examples

Added tests for Jakarta expression injection

adb1ed3

Cover both javax.el and jakarta.el packages

6c24699

artem-smotrakov requested a review from a team as a code owner March 21, 2021 18:44

github-actions bot added documentation Java labels Mar 21, 2021

artem-smotrakov mentioned this pull request Mar 21, 2021

[Java] Query for detecting Jakarta Expression Language injections github/securitylab#324

Closed

1 task

smowton assigned tamasvajk Mar 22, 2021

tamasvajk self-requested a review March 22, 2021 11:18

Marcono1234 reviewed Mar 22, 2021

View reviewed changes

tamasvajk reviewed Mar 26, 2021

View reviewed changes

artem-smotrakov and others added 5 commits April 7, 2021 20:55

Fixed typos

80ac2af

Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>

Removed a reference to Apache Commons EL

3d8e173

LambdaExpression should extend JakartaType

c13ee08

Always bind arguments in TaintPropagatingCall

a764a79

Added setVariable() sink

b39a3ab

artem-smotrakov force-pushed the el-injection branch from 61c4f0b to b39a3ab Compare April 8, 2021 17:42

tamasvajk previously requested changes Apr 12, 2021

View reviewed changes

smowton reviewed Apr 12, 2021

View reviewed changes

Renaming in java/ql/src/experimental/Security/CWE/CWE-094

b96b665

Added comments for tests

97186b3

smowton approved these changes Apr 15, 2021

View reviewed changes

smowton merged commit fa36ba9 into github:main Apr 15, 2021

artem-smotrakov deleted the el-injection branch April 15, 2021 11:40

		Apache Foundation:
		<a href="https://commons.apache.org/dormant/commons-el/">Apache Commons EL</a>

		taintFromExpr = this.getArgument(1) and
		exists(Method m \| this.(MethodAccess).getMethod() = m \|

	JUEL does not support to run expressions in a sandbox. To prevent running arbitrary code,
	JUEL does not support running expressions in a sandbox. To prevent running arbitrary code,

Java: Query for detecting Jakarta Expression Language injections #5471

Java: Query for detecting Jakarta Expression Language injections #5471

Uh oh!

Conversation

artem-smotrakov commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marcono1234 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-smotrakov Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-smotrakov commented Apr 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-smotrakov commented Apr 12, 2021

Uh oh!

smowton commented Apr 13, 2021

Uh oh!

artem-smotrakov commented Apr 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

artem-smotrakov commented Mar 21, 2021 •

edited

Loading

Marcono1234 left a comment •

edited

Loading

artem-smotrakov Apr 8, 2021 •

edited

Loading