I‘d like to go into detail on some of the changes in 2.17.0, why they’re so important, and how they relate to both CVE-2021-45046 and CVE-2021-45105.
The substitution of untrusted log data allowed access to code that was never meant to be exposed. Lookups should be triggered only by configuration and the logging framework (including custom layout/appender/etc plugins). Not by user-provided inputs.
Substitution within the contents of a rendered log message provided the largest opportunity for untrusted inputs to be evaluated by the string replacement system. This was removed (by default) in 2.15.0, and removed entirely in 2.16.0 after public disclosure.
Despite message replacements being removed in 2.15.0, other pathways still exist (based on the configuration) which can evaluate untrusted user-provided values. For example, a PatternLayout using the pattern %p %t %c $${ctx:userAgent} %m%n[1][2] would re-evaluate config.getStrSubstitutor().replace(event, " ${ctx:userAgent} ") for each log-event due to the LiteralPatternConverter. The substitutor would evaluate recursively, and invoke unexpected lookups. At worst triggering something like jndi or an exception preventing logging, but even resulting in logging that doesn't match the literal data input is a very serious bug. For example if MDC.put("userAgent", "${lower:FOO}") then ${ctx:userAgent} would evaluate to literal foo instead of literal ${lower:FOO}
This is not isolated to the PatternLayout, rather anywhere lookups in which the result contained user-provided data. e.g. ${ctx:USER_PROVIDED}, ${event:Message}. For example <Routes pattern="$${ctx:userAgent}"> would be impacted as well.
In 2.17.0 we resolved this class of issues with a simple idea: Recursive evaluation is allowed while parsing the configuration (no user-input/LogEvent data is present, and configuration breaks are to be avoided) however when log-events themselves are being evaluated we never recursively evaluate substitutions. That's it. That means when ${lower:${event:Message}} is evaluated for message Hello, World! the result is hello, world!, and when evaluated for ${java:version} the result is the string literal ${java:version} which itself is not evaluated.
$${ctx:userAgent} so the value itself is not evaluated, however even ${ctx:userAgent} may be vulnerable because it's not likely that a thread-context value is set when the configuration itself is evaluated, in which case no replacement occurs and the value is equivalent to the $${ctx:userAgent} example.%X{userAgent} in the PatternLayout instead of ${ctx:userAgent}. It‘s safer, more obvious, and more efficient. LogEventPatternConverters are pluggable, don’t rely on stringly typed data, and are created when the pattern is parsed, not re-scanned on a per-LogEvent basis.There are certainly cases in which this change impacts existing configurations, however I believe that the safety is well worth the trade-off. For example, this test configuration had to be updated because routes are created based on a log-event, thus we cannot allow recursive evaluation. An argument can be made that the inputs must already be sanitized to avoid path traversal, however the filename isn't the only place these lookups can be evaluated, and the type of sanitization differs dramatically between validating against string replacement data, and validating against unexpected path-based attacks.
In a perfect world I‘d love to swap the types used for substitution to avoid stringly matching anything, using the type system to differentiate between trusted lookups and user-input data, however in such a large, widely-used framework, that’s not something we can do, certainly not quickly or easily.
Complex Lookups :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
This example uses complex lookups based on the ThreadContext (aka MDC), however it continues to work as expected and prevents user-provided data from key1 or key2 from being substituted.
<-- Note that lookups are all escaped to prevent them from being evaluated immediately when the Pattern is constructed --> <PatternLayout pattern="%d %p %t %c $${lower:$${ctx:key1:-$${ctx:key2}}} %m%n"/>
Pattern Reuse Via Lookups :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
This is not impacted either:
<Properties> <Property name="defaultPattern">%d %p %t %c %m%n</Property> </Properties> <Appenders> <Console name="console"> <PatternLayout pattern="${defaultPattern}"/> </Console> </Appenders>
Lazy Pattern Lookups Which include Lazy Lookups :x::x::x:
However, in this case, evaluation depends on a lazily-replaced property which itself is lazily evaluated:
<Properties> <Property name="messageFormat">$${event:Message}</Property> </Properties> <Appenders> <Console name="console"> <PatternLayout pattern="%c $${messageFormat}%n"/> </Console> </Appenders>
The upgrade to 2.17.0 would change the output for the following log line:
LogManager.getLogger(com.example.Main.class).info("example");
- com.example.Main example + com.example.Main ${event:Message}
This can be resolved by unescaping the property reference in the pattern, allowing the pattern itself to be fully evaluated when the configuration is loaded, but preventing message contents from being further interpolated due to the runtime-substitution protections.
Note that this example is contrived, and in practice one should use the %m pattern here rather than a lookup, however this example applies to a general shape of usage and some extrapolation may be required for your configuration.
<Properties> <Property name="messageFormat">$${event:Message}</Property> </Properties> <Appenders> <Console name="console"> - <PatternLayout pattern="%c $${messageFormat}%n"/> + <PatternLayout pattern="%c ${messageFormat}%n"/> </Console> </Appenders>
RoutingAppender with lazy property references in routes :x::x::x:
RoutingAppender routes are lazily evaluated when they‘re needed, which means a LogEvent (thus potentially user-provided data) is available. Everything that could be represented before can still be done, however it’s no longer possible to extract some pieces to shared properties.
Let's take the example from the test liked above
<Configuration status="WARN" name="RoutingTest"> <Properties> <Property name="filename">target/routing1/routingtest-$${sd:type}.log</Property> </Properties> <Appenders> <Routing name="Routing"> <Routes> <Route> <RollingFile name="Routing-${sd:type}" fileName="${filename}" filePattern="target/routing1/test1-${sd:type}.%i.log.gz"> <PatternLayout> <Pattern>%d %p %C{1.} [%t] %m%n</Pattern> </PatternLayout> <SizeBasedTriggeringPolicy size="500" /> </RollingFile> </Route> </Routes> </Routing> </Appenders> <Loggers> <Root level="info"> <AppenderRef ref="Routing"/> </Root> </Loggers> </Configuration>
The reference to fileName="${filename}" is no longer evaluated recursively to fileName="target/routing1/routingtest-${sd:type}.log" however the ${sd:type} is left as a string literal, not expanded as it was before.
The fix in this case is to inline the fileName property value into the RollingFile filename attribute, removing the extra$ escaping the ${sd:type} lookup like so:
<Routing name="Routing"> <Routes> <Route> - <RollingFile name="Routing-${sd:type}" fileName="${filename}" + <RollingFile name="Routing-${sd:type}" fileName="target/routing1/routingtest-${sd:type}.log" filePattern="target/routing1/test1-${sd:type}.%i.log.gz"> <PatternLayout> <Pattern>%d %p %C{1.} [%t] %m%n</Pattern> </PatternLayout> <SizeBasedTriggeringPolicy size="500" /> </RollingFile> </Route> </Routes> </Routing>