Java: Add a query for SpEL injections #3291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

aschackmull merged 1 commit into github:master from artem-smotrakov:spel-injection

Jun 5, 2020

Contributor

artem-smotrakov commented Apr 18, 2020 •

edited

Loading

Here is a list of main updates:

Added experimental/Security/CWE/CWE-094/SpelInjection.ql and a couple of libraries.
Added a qhelp file with a few examples.
Added tests and stubs for Spring Framework.

The query finds CVE-2018-1273 that is an RCE with Spring Data Commons 1.13.10 due to a SpEL injection. To catch the issue, a database should contain both Spring Framework 4.3.14 and Spring Data Commons 1.13.10 because tainted data travels for a while inside the framework before it reaches a sink in MapDataBinder. It also looks like the CodeQL CLI doesn't recognize Lombok. Therefore, Spring Data Commons needs to be built with this hack.

artem-smotrakov requested review from felicitymay and a team as code owners

April 18, 2020 17:37

artem-smotrakov mentioned this pull request

CodeQL query for SpEL injections github/securitylab#67

Closed

1 task

artem-smotrakov changed the title ~~Java: Add a query for SPeL injections~~ Java: Add a query for SpEL injections

artem-smotrakov force-pushed the spel-injection branch from 4709af1 to 01055e7 Compare

April 18, 2020 18:13

aschackmull added the Java label

pwntester reviewed

View reviewed changes

java/ql/src/experimental/Security/CWE/CWE-094/SpringFrameworkLib.qll Outdated

+                }
+              }
+              class NativeWebRequest extends RefType {

Contributor

pwntester May 1, 2020

This one seems to not be used

Contributor Author

artem-smotrakov May 1, 2020

You're absolutely right. At first, I thought that NativeWebRequest may be used as an additional source but then it turned out that it doesn't bring much value. I've removed the class. Thanks!

artem-smotrakov force-pushed the spel-injection branch from 01055e7 to 893acfb Compare

May 1, 2020 13:10

aschackmull reviewed

View reviewed changes

Contributor

aschackmull left a comment

The QL code is generally looking very good, but I've made some inline comments.

java/ql/src/experimental/Security/CWE/CWE-094/SpelInjectionLib.qll Outdated

+              /**
+               * A configuration for safe evaluation context that may be used in expression evaluation.
+               */
+              class SafeEvaluationContextFlowConfig extends TaintTracking2::Configuration {

Contributor

aschackmull May 26, 2020

I'd suggest changing TaintTracking2::Configuration to DataFlow2::Configuration, as data flow seems more appropriate for this check than taint flow.

Contributor Author

artem-smotrakov May 31, 2020

Yup, makes sense.

java/ql/src/experimental/Security/CWE/CWE-094/SpelInjectionLib.qll Outdated

+              import semmle.code.java.dataflow.FlowSources
+              import semmle.code.java.dataflow.TaintTracking2
+              import SpringFrameworkLib
+              import DataFlow

Contributor

aschackmull May 26, 2020

It's a bit cleaner if this import is removed - most things appear to be referenced with their qualified names anyway.

Contributor Author

artem-smotrakov May 31, 2020

Yeah, this import is redundant.

java/ql/src/experimental/Security/CWE/CWE-094/SpelInjectionLib.qll Outdated

+                ExpressionEvaluationSink() {
+                  exists(MethodAccess ma, Method m | m = ma.getMethod() |
+                    m instanceof ExpressionEvaluationMethod and
+                    (getExpr() = ma or getExpr() = ma.getQualifier()) and

Contributor

aschackmull May 26, 2020

Shouldn't this just be

Suggested change

      
                  (getExpr() = ma or getExpr() = ma.getQualifier()) and
          
                  getExpr() = ma.getQualifier() and

java/ql/src/experimental/Security/CWE/CWE-094/SpelInjectionLib.qll Outdated

+                  m.getDeclaringType().getAnAncestor*() instanceof ExpressionParser and
+                  m.hasName("parseExpression") and
+                  ma.getAnArgument() = node1.asExpr() and
+                  (node2.asExpr() = ma.getQualifier() or node2.asExpr() = ma)

Contributor

aschackmull May 26, 2020

Is a call to parseExpression supposed to taint the qualifier? Shouldn't this just be

Suggested change

      
                (node2.asExpr() = ma.getQualifier() or node2.asExpr() = ma)
          
                node2.asExpr() = ma

java/ql/src/experimental/Security/CWE/CWE-094/SpelInjectionLib.qll Outdated

+              predicate isSimpleEvaluationContextConstructorCall(Expr expr) {
+                exists(ConstructorCall cc |
+                  cc.getConstructedType() instanceof SimpleEvaluationContext and
+                  (cc = expr or cc.getQualifier() = expr)

Contributor

aschackmull May 26, 2020

The qualifier of the constructor call? Surely this should just be

Suggested change

      
                (cc = expr or cc.getQualifier() = expr)
          
                cc = expr

java/ql/src/experimental/Security/CWE/CWE-094/SpelInjectionLib.qll Outdated

+                exists(MethodAccess ma, Method m | ma.getMethod() = m |
+                  m.getDeclaringType() instanceof SimpleEvaluationContextBuilder and
+                  m.hasName("build") and
+                  (ma = expr or ma.getQualifier() = expr)

Contributor

aschackmull May 26, 2020

I'm less certain here, but including the qualifier looks superfluous to me?

Suggested change

      
                (ma = expr or ma.getQualifier() = expr)
          
                ma = expr

java/ql/src/experimental/Security/CWE/CWE-094/SpringFrameworkLib.qll Outdated

+                      m.hasName("getParameterNames") or
+                      m.hasName("getParameterMap")
+                    ) and
+                    (ma = asExpr() or ma.getQualifier() = asExpr())

Contributor

aschackmull May 26, 2020

Is there a reason to include the qualifier as the source instead of just the method result?

Suggested change

      
                  (ma = asExpr() or ma.getQualifier() = asExpr())
          
                  ma = asExpr()

java/ql/src/experimental/Security/CWE/CWE-094/SpringFrameworkLib.qll Outdated

+              predicate createMutablePropertyValuesStep(DataFlow::Node node1, DataFlow::Node node2) {
+                exists(ConstructorCall cc | cc.getConstructedType() instanceof MutablePropertyValues |
+                  node1.asExpr() = cc.getAnArgument() and
+                  (node2.asExpr() = cc or node2.asExpr() = cc.getQualifier())

Contributor

aschackmull May 26, 2020

Suggested change

      
                (node2.asExpr() = cc or node2.asExpr() = cc.getQualifier())
          
                node2.asExpr() = cc

Contributor

aibaars commented May 27, 2020

This week I ran into CVE-2018-1260 which was found by the same researcher as CVE-2018-1273

Fixed by: spring-attic/spring-security-oauth@adb1e6d
Blog post: https://blog.h3xstream.com/2018/05/beware-of-magic-spell-part-2-cve-2018.html

Fixed by: spring-projects/spring-data-commons@613cf08
Blog post: https://blog.h3xstream.com/2018/05/beware-of-magic-spell-part-1-cve-2018.html

Contributor Author

artem-smotrakov commented May 28, 2020

@aschackmull Thanks for the review and suggestions for the qualifiers. Let me have a closer look.


          Java: Add a query for SpEL injections

df3adee

- Added experimental/Security/CWE/CWE-094/SpelInjection.ql
  and a couple of libraries
- Added a qhelp file with a few examples
- Added tests and stubs for Spring

artem-smotrakov force-pushed the spel-injection branch from 893acfb to df3adee Compare

May 31, 2020 17:53

artem-smotrakov requested a review from a team as a code owner

May 31, 2020 17:53

Contributor Author

artem-smotrakov commented May 31, 2020

@aschackmull There was a bit of mess with qualifiers - thanks for your suggestions! I've addressed the comments.

artem-smotrakov requested review from aschackmull and removed request for a team

May 31, 2020 17:55

artem-smotrakov mentioned this pull request

Java: Add a query for MVEL injections #3329

Merged

Contributor

aschackmull commented Jun 5, 2020

@aschackmull There was a bit of mess with qualifiers - thanks for your suggestions! I've addressed the comments.

I'm about to look at your latest changes, but this is made a bit more difficult than it ought to be due to the commits being squashed together. Please don't squash commits in the future once review has started.

aschackmull approved these changes

View reviewed changes

aschackmull merged commit e4e51b5 into github:master

artem-smotrakov deleted the spel-injection branch

June 5, 2020 13:52

atorralba mentioned this pull request

Java: Promote SpEL Injection query from experimental #6037

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java