9 ms·

There seems to be a misunderstanding here. We have on the one side a garbage feature that should never have been implemented - but if you want to keep it for ba

There seems to be a misunderstanding here. We have on the one side a garbage feature that should never have been implemented - but if you want to keep it for backwards compatibility, sure. But then we have log4j scanning all values instead of only format strings - I think it can be argued that this behavior is a critical bug and was never intended to begin with. It seems to have only come about because whoever implemented the JNDI stuff lost their bearing in the absurd class hierarchies and abstractions in log4j. Of course the last part holds the solution for our backwards compatibility issue. Remove the JNDI nonsense from the default package and move it into an extension package. Whoever wants to keep it can just add that to their dependencies and continue to enjoy logging functions that sometimes also make network connections and block your program.

ehsankia 5y ago
Right, I was also confused by the blame on backward compatibility. You can keep things backward compatible without necessarily making it on by default. There is no reason why `formatMsgNoLookups` should the default. If it is indeed an obscure and hacky feature for backward compatibility, just make it opt-in. People who really care about it will enable it, most people won't have to carry that baggage and we wouldn't be in a situation like this.
- iratewizard 5y ago
  Hindsight is always 20-20.
- Thorrez 5y ago
  >for a feature we all dislike yet needed to keep due to backward compatibility concerns. If they really dislike the feature that much, they likely dislike the code and want to completely delete it. I'm not sure if making it opt-in would make them as happy as fully deleting it, so they are less motivated to make it opt-in than they would be to fully delete it.
  - ehsankia 5y ago
    They could also "fully" delete it by putting it in a separate opt-in package.
jameshart 5y ago
Indeed - as evidence for this, I would submit that slf4j and logback were created to offer a drop in replacement for log4j (slf4j literally provides alternative implementations of the org.apache.log4j.Logger class), but I have never seen anybody complain that "I switched to logback and slf4j and my jndi substitutions stopped working." Nobody thought this was how log4j worked; log4j's documentation for format syntax only covers {} placeholders - the same format that slf4j has grandfathered in from log4j. I agree this feels like a case where they got confused about their internal terminology. Log4j refers to messages with {} placeholders as 'FormattedMessages'; it refers to the log pattern syntax as 'Patterns' in code - but it seems to refer to them as 'log formats' in documentation. Somewhere in this mess, someone hooked up the pattern capabilities into the formatting system.
- unscaled 5y ago
  > but I have never seen anybody complain that "I switched to logback and slf4j and my jndi substitutions stopped working." SLF4J was created to replace Apache Commons Logging and Logback was created to replace Log4j 1.x. Both were created Ceki Gülcü, the original author of Log4j 1.x [1]. Logback came out in 2006. The first beta version of Log4j 2.x was only released 6 years later in 2012, and the JNDI lookup feature was added in 2.0-beta9[2] in 2013! Obviously nobody complained when switching from Log4j 1.x to SLF4J+Logback that a feature from a completely different library (with the same name) that would be created 7 years into the future was not supported. > Somewhere in this mess, someone hooked up the pattern capabilities into the formatting system. That's not what happened. The lookup mechanism (which includes "${jndi:}" lookups) is completely unrelated to the message formatting subsystem. The way formatting and pattern lookups work in log4j2 is: 1. logger.info("Hello {}", "world") creates a FormattedMessage instance with the "Hello {}" format string and a single parameter, "world". 2. The FormattedMessage is wrapped in a LogEvent and routed to the correct appender(s). 3. Most appenders will format the LogEvent with a Layout. In our case, it's PatternLayout we care about[3]. 4. PatternLayout will pre-calculate a set of PatternConverters based on your pattern, so it doesn't have to keep parsing the pattern on every invocation. "%m" will map to MessagePatternConverter. 5. (grossly simplifying zero-garbage and streaming optimizations) Each pattern converter is executed and appends to the final layout text's StringBuilder. 6. (grossly simplifying oh so many things) MessagePatternConverter will first call event.getMessage().getFormattedMessage(). The logic for formatting the message is entirely encapsulated by Message and its subclasses. MessagePatternConverter has no way to distinguish the format string from the user-provided parameters! 7. MessagePatternConverter finally applies the pattern lookups to the formatted message text. The pattern lookup mechanism is completely separate from and orthogonal to the message formatting mechanism. --- That was long-winded, but I had to fight these annoying misconception about "log4j not implemented format strings properly". Now, there are several things I'm not saying here: 1. I don't think more than a handful of people ever relied on lookups working on the log message (formatted or otherwise), as opposed to the pattern in the configuration file. 2. I don't think Log4j should have kept compatibility here. The moment the maintainers implemented "%m{nolookups}" (on version 2.7), they should have made it the default. That being said, I know this is very hard to do in the Java ecosystem. But I think it is time that the Java developer community changes its extremist position regarding compatibility at all costs. 3. I don't think that Log4j should have implemented pattern lookups for text messages to begin with. Even if was just the format string part (which is impossible to do with Log4j's current architecture anyway). 4. I don't think any kind of string formatting should be included in a logging library. If you want to format log messages, use an external formatting function or string interpolation (if you're lucky enough to be using Kotlin or Scala). If it is added, it should only be used as a convenience, and shouldn't do anything more than formatting (like lookups). Relying on developers to always remember that log.info("Hello {}", world) is safe and log.info("Hello {}" + world) gives the entire internet full control of your server is beyond stupid. Even if Log4j went with this silly distinction, I would say it was a horrible design. [1] https://techblog.bozho.net/the-logging-mess/ https://techblog.bozho.net/the-logging-mess/ [2] https://logging.apache.org/log4j/2.x/changes-report.html#a2.0-beta9 https://logging.apache.org/log4j/2.x/changes-report.html#a2.... [3] It seems like PatternLayout is the only layout vulnerable to this bug in log4j2, but it is hard to tell, the implementation being a classic Java mess of deep class hierarchy, liberal use of reflection to control everything and some heroic attempts to break SOLID principles at least 4 times on a single line of code. Take my analysis with a grain of salt. It's a gross simplification of what is unfortunately par for the course in many Java libraries.*
x0x0 5y ago
And enabled by default. That's the most mind-blowing bit of this feature. The backcompat argument is a deflection for shipping a time bomb into people's codebases.
- closewith 5y ago
  To be fair to the maintainers, they didn't ship anything into people's codebases. People chose Log4j and pulled it into their code. FOSS contributers aren't responsible for downstream use of their projects.
  - watwut 5y ago
    I don't think this argument makes much sense, beyond completely legalistic. If you add backdoor to the used library, then yes you shipped it there. You don't have legal liability for consequences when it is found, but it does not make not being the one who shipped it. It just protects you legally, as it should.
    - closewith 5y ago
      > I don't think this argument makes much sense, beyond completely legalistic. The downstream users chose to use Log4j, chose to upgrade to the version where the exploit was introduced, chose not to audit the code. It's their responsibility and theirs alone. A maintainer can release a new version of their software. No-one is under any obligation to use it. They certainly don't reach down into your repos and push their changes. So I agree that the maintainers shipped a release. I certainly don't agree that the shipped it into anyone else's codebase.
      - watwut 5y ago
        > The downstream users chose to use Log4j, chose to upgrade to the version where the exploit was introduced, chose not to audit the code. It's their responsibility and theirs alone. No. It is manipulative framing where responsibility for backdoor is pushed onto users. It is especially ridiculous in the context of industry where not upgrading is seen as irresponsible. And especially ridiculous when the same industry pushes for open source and when companies eventually start to listen and use open source, acts like there was something wrong with using open source.
lultimouomo 5y ago
> But then we have log4j scanning all values instead of only format strings - I think it can be argued that this behavior is a critical bug and was never intended to begin with. It was actually intended behavior, and this is what really boggles the mind! Javadoc says explicitly that variable replacement is recursive, with cycle detection (which will throw! What happens to the log line in this case?) [0]. [0] https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/apache/logging/log4j/core/lookup/StrSubstitutor.html https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/...
- culturedsystems 5y ago
  That link is about variable replacement in config strings, which is intentionally recursive. It doesn't mention the use of the variable replacement mechanism when interpolating values into log messages, which is what makes this vulnerability so bad, and as far as I can see was not intentional.
thanatos519 5y ago
"lost their bearing in the absurd class hierarchies and abstractions" sounds familiar. Java app stack traces are like Neal Stephenson epics, but less entertaining.

ehsankia 5y ago

Right, I was also confused by the blame on backward compatibility. You can keep things backward compatible without necessarily making it on by default. There is no reason why `formatMsgNoLookups` should the default. If it is indeed an obscure and hacky feature for backward compatibility, just make it opt-in. People who really care about it will enable it, most people won't have to carry that baggage and we wouldn't be in a situation like this.

iratewizard 5y ago
Hindsight is always 20-20.
Thorrez 5y ago
>for a feature we all dislike yet needed to keep due to backward compatibility concerns. If they really dislike the feature that much, they likely dislike the code and want to completely delete it. I'm not sure if making it opt-in would make them as happy as fully deleting it, so they are less motivated to make it opt-in than they would be to fully delete it.
- ehsankia 5y ago
  They could also "fully" delete it by putting it in a separate opt-in package.

jameshart 5y ago

Indeed - as evidence for this, I would submit that slf4j and logback were created to offer a drop in replacement for log4j (slf4j literally provides alternative implementations of the org.apache.log4j.Logger class), but I have never seen anybody complain that "I switched to logback and slf4j and my jndi substitutions stopped working." Nobody thought this was how log4j worked; log4j's documentation for format syntax only covers {} placeholders - the same format that slf4j has grandfathered in from log4j. I agree this feels like a case where they got confused about their internal terminology. Log4j refers to messages with {} placeholders as 'FormattedMessages'; it refers to the log pattern syntax as 'Patterns' in code - but it seems to refer to them as 'log formats' in documentation. Somewhere in this mess, someone hooked up the pattern capabilities into the formatting system.

unscaled 5y ago
> but I have never seen anybody complain that "I switched to logback and slf4j and my jndi substitutions stopped working." SLF4J was created to replace Apache Commons Logging and Logback was created to replace Log4j 1.x. Both were created Ceki Gülcü, the original author of Log4j 1.x [1]. Logback came out in 2006. The first beta version of Log4j 2.x was only released 6 years later in 2012, and the JNDI lookup feature was added in 2.0-beta9[2] in 2013! Obviously nobody complained when switching from Log4j 1.x to SLF4J+Logback that a feature from a completely different library (with the same name) that would be created 7 years into the future was not supported. > Somewhere in this mess, someone hooked up the pattern capabilities into the formatting system. That's not what happened. The lookup mechanism (which includes "${jndi:}" lookups) is completely unrelated to the message formatting subsystem. The way formatting and pattern lookups work in log4j2 is: 1. logger.info("Hello {}", "world") creates a FormattedMessage instance with the "Hello {}" format string and a single parameter, "world". 2. The FormattedMessage is wrapped in a LogEvent and routed to the correct appender(s). 3. Most appenders will format the LogEvent with a Layout. In our case, it's PatternLayout we care about[3]. 4. PatternLayout will pre-calculate a set of PatternConverters based on your pattern, so it doesn't have to keep parsing the pattern on every invocation. "%m" will map to MessagePatternConverter. 5. (grossly simplifying zero-garbage and streaming optimizations) Each pattern converter is executed and appends to the final layout text's StringBuilder. 6. (grossly simplifying oh so many things) MessagePatternConverter will first call event.getMessage().getFormattedMessage(). The logic for formatting the message is entirely encapsulated by Message and its subclasses. MessagePatternConverter has no way to distinguish the format string from the user-provided parameters! 7. MessagePatternConverter finally applies the pattern lookups to the formatted message text. The pattern lookup mechanism is completely separate from and orthogonal to the message formatting mechanism. --- That was long-winded, but I had to fight these annoying misconception about "log4j not implemented format strings properly". Now, there are several things I'm not saying here: 1. I don't think more than a handful of people ever relied on lookups working on the log message (formatted or otherwise), as opposed to the pattern in the configuration file. 2. I don't think Log4j should have kept compatibility here. The moment the maintainers implemented "%m{nolookups}" (on version 2.7), they should have made it the default. That being said, I know this is very hard to do in the Java ecosystem. But I think it is time that the Java developer community changes its extremist position regarding compatibility at all costs. 3. I don't think that Log4j should have implemented pattern lookups for text messages to begin with. Even if was just the format string part (which is impossible to do with Log4j's current architecture anyway). 4. I don't think any kind of string formatting should be included in a logging library. If you want to format log messages, use an external formatting function or string interpolation (if you're lucky enough to be using Kotlin or Scala). If it is added, it should only be used as a convenience, and shouldn't do anything more than formatting (like lookups). Relying on developers to always remember that log.info("Hello {}", world) is safe and log.info("Hello {}" + world) gives the entire internet full control of your server is beyond stupid. Even if Log4j went with this silly distinction, I would say it was a horrible design. [1] https://techblog.bozho.net/the-logging-mess/ https://techblog.bozho.net/the-logging-mess/ [2] https://logging.apache.org/log4j/2.x/changes-report.html#a2.0-beta9 https://logging.apache.org/log4j/2.x/changes-report.html#a2.... [3] It seems like PatternLayout is the only layout vulnerable to this bug in log4j2, but it is hard to tell, the implementation being a classic Java mess of deep class hierarchy, liberal use of reflection to control everything and some heroic attempts to break SOLID principles at least 4 times on a single line of code. Take my analysis with a grain of salt. It's a gross simplification of what is unfortunately par for the course in many Java libraries.*

x0x0 5y ago

And enabled by default. That's the most mind-blowing bit of this feature. The backcompat argument is a deflection for shipping a time bomb into people's codebases.

closewith 5y ago
To be fair to the maintainers, they didn't ship anything into people's codebases. People chose Log4j and pulled it into their code. FOSS contributers aren't responsible for downstream use of their projects.
- watwut 5y ago
  I don't think this argument makes much sense, beyond completely legalistic. If you add backdoor to the used library, then yes you shipped it there. You don't have legal liability for consequences when it is found, but it does not make not being the one who shipped it. It just protects you legally, as it should.
  - closewith 5y ago
    > I don't think this argument makes much sense, beyond completely legalistic. The downstream users chose to use Log4j, chose to upgrade to the version where the exploit was introduced, chose not to audit the code. It's their responsibility and theirs alone. A maintainer can release a new version of their software. No-one is under any obligation to use it. They certainly don't reach down into your repos and push their changes. So I agree that the maintainers shipped a release. I certainly don't agree that the shipped it into anyone else's codebase.
    - watwut 5y ago
      > The downstream users chose to use Log4j, chose to upgrade to the version where the exploit was introduced, chose not to audit the code. It's their responsibility and theirs alone. No. It is manipulative framing where responsibility for backdoor is pushed onto users. It is especially ridiculous in the context of industry where not upgrading is seen as irresponsible. And especially ridiculous when the same industry pushes for open source and when companies eventually start to listen and use open source, acts like there was something wrong with using open source.

lultimouomo 5y ago

> But then we have log4j scanning all values instead of only format strings - I think it can be argued that this behavior is a critical bug and was never intended to begin with. It was actually intended behavior, and this is what really boggles the mind! Javadoc says explicitly that variable replacement is recursive, with cycle detection (which will throw! What happens to the log line in this case?) [0]. [0] https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/apache/logging/log4j/core/lookup/StrSubstitutor.html https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/...

culturedsystems 5y ago
That link is about variable replacement in config strings, which is intentionally recursive. It doesn't mention the use of the variable replacement mechanism when interpolating values into log messages, which is what makes this vulnerability so bad, and as far as I can see was not intentional.

thanatos519 5y ago

"lost their bearing in the absurd class hierarchies and abstractions" sounds familiar. Java app stack traces are like Neal Stephenson epics, but less entertaining.