LOG4J2-3198: Log4j2 no longer formats lookups in messages by default

[carterkozak]

Lookups in messages are confusing, and muddy the line between logging APIs
and implementation. Given a particular API, there's an expectation that a
particular shape of call will result in specific results. However, lookups
in messages can be passed into JUL and will result in resolved output in
log4j formatted output, but not any other implementations despite no direct
dependency on those implementations.

There's also a cost to searching formatted message strings for particular
escape sequences which define lookups. This feature is not used as far as
we've been able to tell searching github and stackoverflow, so it's
unnecessary for every log event in every application to burn several cpu
cycles searching for the value.

We've had several users run into confusion due to this feature and request
a way to turn it off globally. Off should be the default to make the framework
behave more reasonably. Cases when lookups are used would be more obvious
and efficient if the lookup code itself was written directly as a log-line parameter,
for example:

if (log.isInfoEnabled()) {
-    log.error("Foo {ctx:bar}");
+    log.error("Foo {}", ThreadContext.get("bar"));
}

[vy]

@carterkozak, this PR only touches to the PatternLayout. Shouldn't we rather consolidate all lookup related code into a single place and disable it there? Otherwise, JSON Template, Gelf, JSON, XML, and YAML layouts will be left out.

[carterkozak]

@vy I think this is the only place which uses lookups in this way, did I miss something?

[vy]

@carterkozak, you're right. I've got confused by lookups used in configurations. Simply searching for ${" in the code base gave me false positives. I have checked if logger.info("${env:USER}") causes a lookup when JSON, JSON Template, or Gelf layout is used, and indeed none triggers a lookup.

[rgoers]

I don't actually see MessagePatternConverter in the list of files that were changed.

I am unclear as to why a system property was needed if only Pattern Layout is affected.

Looking at the existing MessagePatternConverter it looks like you already added support to bypass lookup in 2017. But this is controlled by adding a "nolookups" option to the message. I would suggest simply reversing this so that lookups are disabled by default and adding a "lookups" option will enable them. That way we don't need the system property and this can be selectively applied where desired. We should still tolerate the nolookups option but it will effectively be a no-op.

[carterkozak]

I am unclear as to why a system property was needed if only Pattern Layout is affected.

We had hundreds of services, each of which with default configurations and per-install overrides, where developers copied patterns from other services (some of which using logj1.2) -- the property was helpful because I was able to set it in a shared log4j2.component.properties and apply the behavior everywhere.

I would suggest simply reversing this so that lookups are disabled by default and adding a "lookups" option will enable them.

I had considered this and decided it may cause more friction for consumers who want to revert the behavior, but I agree with you that the end result is much more desirable. I'll remove the property and take this approach :-)

[carterkozak]

I've updated this PR removing the property, allowing only explicit opt-in via %m{lookups}

[carterkozak]

@rgoers Do you have any additional feedback for this change?

[rgoers]

I'm not sure why this wasn't closed as merged by GitHub but I manually merged it.

[SR-G]

Does anyone know "when" exactly inside the LOG4J2 stack this bug may be triggered ? Is it only at "rendering" level, or may it be before ?

For the context, in my case : all logs are routed by LOG4J2 (2.14.0), but trapped by a specific appender in which LoggingEvent are transformed into something else, and these transformed objects are then stored into an ElasticSearch database. After that, the display of these logs is done in a proprietary way (without LOG4J2 being involved in any way). So my feeling is that in this very specific situation, this security issue could not be triggered ... assuming the problematic code is only triggered inside LOG4J2 layout rendering. Hence this question, any ideas about "when" this problem can be triggered ?

[vy]

@SR-G, no, your use case is actually pretty common: feeding Log4j to ELK. Actually, we even have a layout to ease this process: JsonTemplateLayout. In a nutshell, it was only PatternLayout that was performing interpolation on the formatted message, hence the rest of the layouts (incl. JsonTemplateLayout) are not affected by this vulnerability. Though right now there are 3 CVEs and depending on the use of lookups of your proprietary appender+layout, you might be affected.