Java: Experimental query for Log4j JNDI Injection

[atorralba]

This is an experimental query for detecting CVE-2021-44228.

[fproulx]

Good job @atorralba was looking for that throughout the day =)

[Marcono1234]

Is this based on / related to #7054? Would it make sense to use a kind other than logging here to avoid clashes with that other pull request, and because these findings here have a higher impact?

Though I assume this query here is mostly only useful for security researchers to find attack vectors (which can still be valuable); it should not be used to reason about whether an application is affected by the Log4j 2 vulnerability because:

• Dependencies might use Log4j 2; this will not be detected by this query
• This query likely has false negatives
• There are bridging bindings (e.g. "Log4j 2 SLF4J Binding") which redirect log messages of other logging frameworks to Log4j 2

[securingdev]

Have we considered looking at this open source query as an alternative way of finding this vulnerability in code?

[sj]

Few suggestions for improvements to the documentation

[atorralba]

Thank you so much for your review @Marcono1234 and @sj!

Is this based on / related to #7054? Would it make sense to use a kind other than logging here to avoid clashes with that other pull request, and because these findings here have a higher impact?

This query is intended to be short-lived and remain in experimental. The intention is to provide a tool for users to check for potential exploitation paths in their code if they need it, but not to run it continuously as part of code scanning. As such, we don't intend to merge the Log Injection query for the moment, which should avoid potential clashes. Of course, if this was going to be merged into the main query set, your suggestion would make total sense, so thank you!

Though I assume this query here is mostly only useful for security researchers to find attack vectors (which can still be valuable); it should not be used to reason about whether an application is affected by the Log4j 2 vulnerability because:

• Dependencies might use Log4j 2; this will not be detected by this query
• This query likely has false negatives
• There are bridging bindings (e.g. "Log4j 2 SLF4J Binding") which redirect log messages of other logging frameworks to Log4j 2

As stated above, this query is intended to be a tool to help users evaluating their code, but of course it's not a silver bullet. For instance, Dependabot is actually more useful for detecting the issue, because it'll report if the vulnerable version is used. But for those that aren't able to activate Dependabot (or additionally to it), this query should provide a good alternative to obtain some information.

Regarding bindings, while you're right that they could be also sinks, we've discussed it internally and, for the moment, we are going to try to maintain a high TP ratio in this query. If we added this without knowing whether the binding is actually bridging to Log4j, we'd be raising FP alerts.

[atorralba]

Have we considered looking at this open source query as an alternative way of finding this vulnerability in code?

Yes, thanks @securingdev! There are several community-provided queries that do similar things out there. In this case, we decided to keep the explicit CSV format for defining sinks, because we think it's more comprehensive. Is there anything that you think our query is lacking compared to those?

[securingdev]

I would recommend we look at merging in the following section of code (or something similar) to catch any edge cases we might have missed:

exists(RefType t, Method m |
      t.hasQualifiedName("org.apache.log4j", ["Category", "Logger", "LogBuilder"]) // Log4j v1
      or
      t.hasQualifiedName("org.apache.logging.log4j", ["Logger", "LogBuilder", "LoggerManager"]) // Log4j v2 or
      or
      t.hasQualifiedName("org.apache.logging.log4j.core", ["Logger", "LogBuilder", "LoggerManager"]) // Log4j v2
      or
      t.hasQualifiedName("org.apache.logging.log4j.status", "StatusLogger") // Log4j Status logger
      or
      t.hasQualifiedName("org.slf4j", ["Logger", "LoggingEventBuilder"]) and // SLF4J Logger is used when Log4j core is on classpath
      log4JJarCoreJarFilePresent()
    |
      (
        m.getDeclaringType().getASourceSupertype*() = t or
        m.getDeclaringType().extendsOrImplements*(t)
      ) and
      m.getReturnType() instanceof VoidType and
      this = m.getAReference()
    )
  }

The following idea was also worth considering in some capacity:

predicate log4JJCoreJarFile(JarFile file) { file.getBaseName().matches("%log4j-core%") }

predicate log4JJarCoreJarFilePresent() { log4JJCoreJarFile(_) }

[atorralba]

The problem with that approach is that it would probably introduce some FPs like, as stated above, catching and throwing. Given how sensitive this query is security-wise, I would prefer not to raise false issues and keep a high TP ratio.

Happy to change it, though, if several of us think the risk of raising some FP is acceptable in exchange of having less potential FNs.

[ambiso]

Do I understand correctly - these queries are to detect whether a project is vulnerable to specifically CVE-2021-44228?

Are there queries that should/would have detected the vulnerability inside of log4j2?

[sj]

Do I understand correctly - these queries are to detect whether a project is vulnerable to specifically CVE-2021-44228?

That's correct. And it will detect other cases of Log4j log injection (which is something you might generally want to avoid).

Are there queries that should/would have detected the vulnerability inside of log4j2?

That's under active investigation. If you have any suggestions on how to approach this with CodeQL we'd love to hear from you!

[securingdev]

Will this query also catch cases of CVE-2021-45046 related to Log4J?

[atorralba]

Will this query also catch cases of CVE-2021-45046 related to Log4J?

Yes, the vulnerability pattern remains the same (untrusted data written to logs), it's just that the patch was incomplete for certain non-default configurations.

[Marcono1234]

Do I understand correctly - these queries are to detect whether a project is vulnerable to specifically CVE-2021-44228?

That's correct.

@ambiso, might be good to clarify here, as mentioned by atorralba, that this is no "silver bullet". If this query does not have any findings, you should not interpret that as not being vulnerable. If you have log4j-core as (transitive) dependency, you should definitely use one of the mitigation methods listed on the Log4j 2 security notes. They have also released a new version targeting Java 7, in case that was an issue for you.

---

Yes, the vulnerability pattern remains the same (untrusted data written to logs), it's just that the patch was incomplete for certain non-default configurations.

CVE-2021-45046 seems to be caused by legitimate usage of lookups in the log config (e.g. log4j2.xml) whose substituted value is attacker controlled (and resolved recursively). This blog post explains it quite well: https://www.lunasec.io/docs/blog/log4j-zero-day-update-on-cve-2021-45046/

That vulnerability is normally not triggered directly by log message injection (unless someone uses $${event:Message} in their log config, which is rather unlikely), but instead by other values being stored in some way, e.g. in the Log4j 2 ThreadContext or possibly implicitly in the ServletContext, see the lookup types here: https://logging.apache.org/log4j/2.x/manual/lookups.html
Therefore to me it looks like this query does not cover CVE-2021-45046.

[atorralba]

(unless someone uses $${event:Message} in their log config, which is rather unlikely)

That's what I was referring to with non-default configurations, in the sense that the CVE mentioned an "incomplete fix" of the original vulnerability.

instead by other values being stored in some way, e.g. in the Log4j 2 ThreadContext or possibly implicitly in the ServletContext

But you're right, there are more sinks that could be modeled to cover CVE-2021-45046 (e.g. ThreadContext.put). The urgency of doing that, however, is up for debate because of the somewhat limited impact of this in >=2.15.0 (DoS, if anything, as mentioned here), although it seems to be another RCE vector in <2.15.0.

Thanks for the clarification @Marcono1234.

[atorralba]

Also, there seems to be a third vulnerability affecting 2.15.0 being disclosed soon. Depending on how that works, maybe it will make sense to extend this query to completely cover both CVE-2021-45046 and this new variant.

[sirdarckcat]

is anyone working on that atm? @atorralba

[atorralba]

#7423 aims to cover CVE-2021-45046 completely.