6 ms·

These "special" strings that Log4j parse must be in the formatting string though, right? External Strings should normally be logged as parameters, not included

by brabel 5y ago

These "special" strings that Log4j parse must be in the formatting string though, right? External Strings should normally be logged as parameters, not included in the format String. For example: // this is ok log.debug("user-agent={}", userAgent); // this is bad log.debug("user-agent=" + userAgent); Does this vulnerability still work on the first case? EDIT: the answer is yes, just tried it myself.

erk__ 5y ago
According to a comment here it does https://news.ycombinator.com/item?id=29506397 https://news.ycombinator.com/item?id=29506397
- brabel 5y ago
  Confirmed myself here... yes, it does :(
  - deleted 5y ago
    [deleted]
danmur 5y ago
That's very surprising.
deleted 5y ago
[deleted]
vincnetas 5y ago
wow it's like SQL injection but even when using user input as parameter it does not get sanitised. Really curious what was motivation for such behaviour.
- niea_11 5y ago
  Here is the jira ticket that introduced the behaviour (jndi lookup) : https://issues.apache.org/jira/browse/LOG4J2-313 https://issues.apache.org/jira/browse/LOG4J2-313
  - josefx 5y ago
    That explains the jndi lookup, but not why variables are parsed when they are not part of the format string. That just asks for unexpected issues and exploits.
  - vincnetas 5y ago
    JNDI lookup in it self is not a problem, problem is that user input is not sanitised and can include templates which can have JNDI lookup in them. I would expect user input with {} template symbols to be escaped and not evaluated.
    - niea_11 5y ago
      The method responsible for variable substitution is here [1]. There are other lookup mechanisms, (didn't check them all) but they only retrieve environment/static values (this one [2] for example retrieves kubernetes attributes). I think the jndi one is the only one that load and execute code. Edit : I think my understanding of the docs is incorrect so ignore the next paragraph From the documentation [3], I have the impression that these variables should be evaluated only when loading the pattern layout configuration. But in reality they are also evaluated when formatting the final log message (log.info(...)). I agree that the input should be sanitized but only if the formatting behavior is a bug and was not intentional. One possible explanation for the current situation, is that developers assumed that all lookup mechanism will retrieve only static values (env variables for example). And then another dev introduced the jndi lookup which execute code, but no one noticed the impact on the already existing behavior (evaluation variable when formatting the final msg). Edit 1: https://github.com/apache/logging-log4j2/blob/9df31f73b62ba23e335d548843547f72b3684a66/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java#L928 https://github.com/apache/logging-log4j2/blob/9df31f73b62ba2... 2: https://github.com/apache/logging-log4j2/blob/c2b07e37995004555c211cdf0bb169d6a6a6f96b/log4j-kubernetes/src/main/java/org/apache/logging/log4j/kubernetes/KubernetesLookup.java https://github.com/apache/logging-log4j2/blob/c2b07e37995004... 3: https://logging.apache.org/log4j/2.x/manual/lookups.html https://logging.apache.org/log4j/2.x/manual/lookups.html
      - 5y ago
- chrisjc 5y ago
  Yup, log injection is something that I've been hearing a lot about recently, but I really only thought about it being able to affect systems that consume the logs. This approach is pretty ingenious.
- smsm42 5y ago
  That's way worse than that - that's like SQL server looks into your data and if it sees something that looks like SQL it executes it right there. I wonder how it took until now to find an exploit...
janekm 5y ago
Though of course "debug.log(stuffIGotFromPeer)" is also very common (and as you point out should always be avoided).
- lvh 5y ago
  Fortunately (and for reasons I can't begin to understand), debug and info logging levels seem to be safe, but error is not. Technically better, but somehow... morally worse?
- Pxtl 5y ago
  If "debug.log(stuffIGotFromPeer)" is dangerous, then the problem is with debug.log and not my code. Safe logging is not too high of a bar.
  - jhugo 5y ago
    The point is that the first argument is the format string. debug.log("{}", stuffIGotFromPeer) should generally be safe (this bug is an example of when it isn't, though)
    - jameshart 5y ago
      The fact that the first argument is interpreted as a format string even when you aren't supplying format arguments is a violation of 'principle of least surprise'. The availability of format parameters that can access environment data - let alone remotely loaded code - is a feature most people won't discover unless they go looking for it.
      - rixed 5y ago
        How are varargs implemented in the JVM ABI? Is the called function even aware of how many parameters were actually passed, or does it have to rely on the format string for that?
        jameshart 5y ago
mshanu 5y ago
Trying to put my head around, why is this log.debug("user-agent=" + userAgent); bad?
- ehsankia 5y ago
  Generally, most logging frameworks have two parts, the format string, and the parameters. A good logging library will also avoid calling str()/toString() if the log isn't emitted (for example, if it's a debug log but minimum level is info). You have something similar when building database queries, generally you should have a base template into which you insert arguments. The library generally should take care of escaping things and also preventing things like SQL injections.
- jameshart 5y ago
  Because the string concatenation requires allocation of a new string, which will need to be garbage collected, regardless of whether or not the log.debug actually needs it. Using a format and args lets you call the method with only references to existing objects, no additional string needs to be allocated unless the log method actually needs to generate the string to log (and it might even be able to use streaming to output the log and never even allocate the string) When you’re doing things like putting trace logs with all your parameters in at the top of every method call, the memory and GC pressure of generating unnecessary strings can be significant.
  - mshanu 5y ago
    ok, so its only gc overhead, and no security issue with it?
    - jameshart 5y ago
      No. I don’t think anybody generally expects log message parameterization to do anything like escaping or white space normalization or anything to the parameters that wouldn’t also be done to the message string. If you are using a logger to output a message which you want to be able to parse based on delimiters, say, it would be up to you to escape any parameters you were incorporating into it to ensure they don’t confuse your parser.
smsm42 5y ago
That's stunning. People are screaming about SQL injections and such for decades now, every "programming 101 for complete doofuses" course has a chapter about it in the first ten pages, we have tools upon tools to detect patterns of using untrusted data as control... And yet, one of the most popular logging toolkits in one of the most popular languages has it built in as a feature - literally using untrusted and unfiltered outside data as a pattern - and it takes until 2021 (almost 2022, happy new year!) to realize this is bad??? There were so many problems with untrusted data used as control flow, how it didn't raise any alarms before?
- unclebucknasty 5y ago
  I mean it's so inexplicably bad that it's hard to imagine it being an innocent mistake. You have to wonder if opening a ticket for such a feature then having someone (or yourself under another account) build it in such an egregious way is a possible vector for deliberately creating such exploits. If this feature was default enabled, then it's even more suspect. It's just such an esoteric thing. When you factor in this kind of thing with recent revelations about backdoored NPM packages, you have to wonder if OSS is even tenable. I don't want to go all the sky is falling here, But the default model of assuming that someone's paying attention doesn't seem to cut it. It works well in most cases, but it's that 2% or 5% that's a doozy.
  - janto 5y ago
    OSS definitely has a reliability and credibility problem on its hands. Codes of Conduct that prioritize belonging over, say, competence and truthfulness is not helping. We need more people like the old Linus that are more concerned with the quality of the artifact than increasing the size of a community
    - KarlKemp 5y ago
      I think we need fewer people that somehow manage to mangle logic to such a degree that they blame security vulnerabilities on the willingness of some project to state that it's generally preferred to treat each other with some respect. Linus' insults were always cringeworthy. Sometimes they may have been justified, at other times they were just needlessly hurtful and revealed some sort of weakness on his part. In any case, he always had the power to say no with just that single word. Log4J doesn't even have a code of conduct in its source tree, though I guess Apache probably has one. You'll get a free Log4j update if you find any instances of it having an impact on the project.

by brabel 5y ago

erk__ 5y ago

According to a comment here it does https://news.ycombinator.com/item?id=29506397 https://news.ycombinator.com/item?id=29506397

brabel 5y ago
Confirmed myself here... yes, it does :(
- deleted 5y ago
  [deleted]

danmur 5y ago

That's very surprising.

deleted 5y ago

[deleted]

vincnetas 5y ago

wow it's like SQL injection but even when using user input as parameter it does not get sanitised. Really curious what was motivation for such behaviour.

niea_11 5y ago
Here is the jira ticket that introduced the behaviour (jndi lookup) : https://issues.apache.org/jira/browse/LOG4J2-313 https://issues.apache.org/jira/browse/LOG4J2-313
- josefx 5y ago
  That explains the jndi lookup, but not why variables are parsed when they are not part of the format string. That just asks for unexpected issues and exploits.
- vincnetas 5y ago
  JNDI lookup in it self is not a problem, problem is that user input is not sanitised and can include templates which can have JNDI lookup in them. I would expect user input with {} template symbols to be escaped and not evaluated.
  - niea_11 5y ago
    The method responsible for variable substitution is here [1]. There are other lookup mechanisms, (didn't check them all) but they only retrieve environment/static values (this one [2] for example retrieves kubernetes attributes). I think the jndi one is the only one that load and execute code. Edit : I think my understanding of the docs is incorrect so ignore the next paragraph From the documentation [3], I have the impression that these variables should be evaluated only when loading the pattern layout configuration. But in reality they are also evaluated when formatting the final log message (log.info(...)). I agree that the input should be sanitized but only if the formatting behavior is a bug and was not intentional. One possible explanation for the current situation, is that developers assumed that all lookup mechanism will retrieve only static values (env variables for example). And then another dev introduced the jndi lookup which execute code, but no one noticed the impact on the already existing behavior (evaluation variable when formatting the final msg). Edit 1: https://github.com/apache/logging-log4j2/blob/9df31f73b62ba23e335d548843547f72b3684a66/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java#L928 https://github.com/apache/logging-log4j2/blob/9df31f73b62ba2... 2: https://github.com/apache/logging-log4j2/blob/c2b07e37995004555c211cdf0bb169d6a6a6f96b/log4j-kubernetes/src/main/java/org/apache/logging/log4j/kubernetes/KubernetesLookup.java https://github.com/apache/logging-log4j2/blob/c2b07e37995004... 3: https://logging.apache.org/log4j/2.x/manual/lookups.html https://logging.apache.org/log4j/2.x/manual/lookups.html
    - 5y ago
chrisjc 5y ago
Yup, log injection is something that I've been hearing a lot about recently, but I really only thought about it being able to affect systems that consume the logs. This approach is pretty ingenious.
smsm42 5y ago
That's way worse than that - that's like SQL server looks into your data and if it sees something that looks like SQL it executes it right there. I wonder how it took until now to find an exploit...

janekm 5y ago

Though of course "debug.log(stuffIGotFromPeer)" is also very common (and as you point out should always be avoided).

lvh 5y ago
Fortunately (and for reasons I can't begin to understand), debug and info logging levels seem to be safe, but error is not. Technically better, but somehow... morally worse?
Pxtl 5y ago
If "debug.log(stuffIGotFromPeer)" is dangerous, then the problem is with debug.log and not my code. Safe logging is not too high of a bar.
- jhugo 5y ago
  The point is that the first argument is the format string. debug.log("{}", stuffIGotFromPeer) should generally be safe (this bug is an example of when it isn't, though)
  - jameshart 5y ago
    The fact that the first argument is interpreted as a format string even when you aren't supplying format arguments is a violation of 'principle of least surprise'. The availability of format parameters that can access environment data - let alone remotely loaded code - is a feature most people won't discover unless they go looking for it.
    - rixed 5y ago
      How are varargs implemented in the JVM ABI? Is the called function even aware of how many parameters were actually passed, or does it have to rely on the format string for that?
      - jameshart 5y ago

mshanu 5y ago

Trying to put my head around, why is this log.debug("user-agent=" + userAgent); bad?

ehsankia 5y ago
Generally, most logging frameworks have two parts, the format string, and the parameters. A good logging library will also avoid calling str()/toString() if the log isn't emitted (for example, if it's a debug log but minimum level is info). You have something similar when building database queries, generally you should have a base template into which you insert arguments. The library generally should take care of escaping things and also preventing things like SQL injections.
jameshart 5y ago
Because the string concatenation requires allocation of a new string, which will need to be garbage collected, regardless of whether or not the log.debug actually needs it. Using a format and args lets you call the method with only references to existing objects, no additional string needs to be allocated unless the log method actually needs to generate the string to log (and it might even be able to use streaming to output the log and never even allocate the string) When you’re doing things like putting trace logs with all your parameters in at the top of every method call, the memory and GC pressure of generating unnecessary strings can be significant.
- mshanu 5y ago
  ok, so its only gc overhead, and no security issue with it?
  - jameshart 5y ago
    No. I don’t think anybody generally expects log message parameterization to do anything like escaping or white space normalization or anything to the parameters that wouldn’t also be done to the message string. If you are using a logger to output a message which you want to be able to parse based on delimiters, say, it would be up to you to escape any parameters you were incorporating into it to ensure they don’t confuse your parser.

smsm42 5y ago

That's stunning. People are screaming about SQL injections and such for decades now, every "programming 101 for complete doofuses" course has a chapter about it in the first ten pages, we have tools upon tools to detect patterns of using untrusted data as control... And yet, one of the most popular logging toolkits in one of the most popular languages has it built in as a feature - literally using untrusted and unfiltered outside data as a pattern - and it takes until 2021 (almost 2022, happy new year!) to realize this is bad??? There were so many problems with untrusted data used as control flow, how it didn't raise any alarms before?

unclebucknasty 5y ago
I mean it's so inexplicably bad that it's hard to imagine it being an innocent mistake. You have to wonder if opening a ticket for such a feature then having someone (or yourself under another account) build it in such an egregious way is a possible vector for deliberately creating such exploits. If this feature was default enabled, then it's even more suspect. It's just such an esoteric thing. When you factor in this kind of thing with recent revelations about backdoored NPM packages, you have to wonder if OSS is even tenable. I don't want to go all the sky is falling here, But the default model of assuming that someone's paying attention doesn't seem to cut it. It works well in most cases, but it's that 2% or 5% that's a doozy.
- janto 5y ago
  OSS definitely has a reliability and credibility problem on its hands. Codes of Conduct that prioritize belonging over, say, competence and truthfulness is not helping. We need more people like the old Linus that are more concerned with the quality of the artifact than increasing the size of a community
  - KarlKemp 5y ago
    I think we need fewer people that somehow manage to mangle logic to such a degree that they blame security vulnerabilities on the willingness of some project to state that it's generally preferred to treat each other with some respect. Linus' insults were always cringeworthy. Sometimes they may have been justified, at other times they were just needlessly hurtful and revealed some sort of weakness on his part. In any case, he always had the power to say no with just that single word. Log4J doesn't even have a code of conduct in its source tree, though I guess Apache probably has one. You'll get a free Log4j update if you find any instances of it having an impact on the project.