Skip to content

Java: Add a query for MVEL injections - #3329

Merged
aschackmull merged 10 commits into
github:masterfrom
artem-smotrakov:mvel-injection
Jun 8, 2020
Merged

Java: Add a query for MVEL injections#3329
aschackmull merged 10 commits into
github:masterfrom
artem-smotrakov:mvel-injection

Conversation

@artem-smotrakov

@artem-smotrakov artem-smotrakov commented Apr 23, 2020

Copy link
Copy Markdown
Contributor

I'd like to add a query that looks for expression language injections with MVEL. Here is a list of main updates:

  • Added experimental/Security/CWE/CWE-094/MvelInjection.ql.
  • Added experimental/Security/CWE/CWE-094/MvelInjectionLib.qll.
  • Added a qhelp file with an example of vulnerable code.
  • Added tests and stubs for mvel2-2.4.7.

MVEL is a powerful expression language that allows, in particular, calling arbitrary methods. That may lead to arbitrary code execution. In past, there were several issues (not reported by myself) due to unsafe evaluation of MVEL expressions:

Currently, the query doesn't find the CVEs above. To make it work, the query needs to be updated with additional taint propagation steps that take into account internal structure of Elasticsearch and Drools. This internal structure is not visible to users via public APIs. I didn't model this internal structure because I am not sure if the users can benefit from it.

@artem-smotrakov

Copy link
Copy Markdown
Contributor Author

At first, I thought that only static methods from the MVEL class are sinks. But then I realized that there are many more sinks. I've updated the query.


class MvelCompiledScript extends RefType {
MvelCompiledScript() { hasQualifiedName("org.mvel2.jsr223", "MvelCompiledScript") }
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm missing org.mvel2.templates.TemplateCompiler.compileTemplate

@artem-smotrakov artem-smotrakov May 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup! There are actually even more sinks for templates. I'll update the query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've updated the query.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! 👍

@artem-smotrakov

Copy link
Copy Markdown
Contributor Author

I also added a sink for MVELRuntime.execute() and simplified the test a bit.

@artem-smotrakov
artem-smotrakov requested a review from a team as a code owner May 31, 2020 18:17
@artem-smotrakov

Copy link
Copy Markdown
Contributor Author

Fixed similar issues that were found in #3291

@anticomputer

Copy link
Copy Markdown

GHSL scope and impact feedback: this is a high impact query that will find potential RCE vulnerabilities. We have verified this query has a low FP rate in its result set.

@aschackmull aschackmull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM.

Comment on lines +102 to +105

predicate test() {
exists(ConstructorCall cc | cc.getConstructedType() instanceof CompiledAccExpression)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
predicate test() {
exists(ConstructorCall cc | cc.getConstructedType() instanceof CompiledAccExpression)
}

Looks like test code that was left by mistake.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops!

and then runs it in the default powerfull context.
</p>
<sample src="UnsafeMvelExpressionEvaluation.java" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
</example>

Missing close tag.

@artem-smotrakov

Copy link
Copy Markdown
Contributor Author

@aschackmull Thanks for the review! I've addressed your comments.

@aschackmull
aschackmull merged commit 8513c69 into github:master Jun 8, 2020
@artem-smotrakov
artem-smotrakov deleted the mvel-injection branch June 10, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants