10 ms·
To folks wondering what the issue is about, I'll give a short summary that I myself needed. Typically a logging library has one job to do: swallow the string a
by lewisjoe 5y ago
To folks wondering what the issue is about, I'll give a short summary that I myself needed.
Typically a logging library has one job to do: swallow the string as if it's some black box and spit it elsewhere as per provided configurations. Log4j though, doesn't treat strings as black boxes. It inspects its contents and checks if it contains any "variables" that need to be resolved before spitting out.
Now there's a bunch of ways to interpolate "variables" into log content. For example something like "Logging from ${java:vm}" will print "Logging from Oracle JVM". I'm not sure but you get the idea.
One way to resolve a variable using a custom Java resolver is by looking it up through a remote class hosted in some LDAP server, say "${jndi:ldap://someremoteclass}" (I'm still not quite sure why LDAP comes into the picture). Turns out, by including "." in some part of the URL to this remote class, Log4j lets off its guard & simply looks up to that server and dynamically loads the class file.
The fix has introduced ways to configure an allowed set of hosts/protocols/etc and forces Log4j to go through this configuration such that these dynamic resolutions don't land on an random/evil server.
- brabel 5y agoThese "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 agoAccording to a comment here it does https://news.ycombinator.com/item?id=29506397 https://news.ycombinator.com/item?id=29506397
- brabel 5y agoConfirmed myself here... yes, it does :(
- deleted 5y ago[deleted]
- danmur 5y agoThat's very surprising.
- deleted 5y ago[deleted]
- vincnetas 5y agowow 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 agoHere 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 agoThat 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 agoJNDI 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 agoThe 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
- janekm 5y agoThough of course "debug.log(stuffIGotFromPeer)" is also very common (and as you point out should always be avoided).
- lvh 5y agoFortunately (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 agoIf "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 agoThe 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 agoThe 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.
- mshanu 5y agoTrying to put my head around, why is this log.debug("user-agent=" + userAgent); bad?
- ehsankia 5y agoGenerally, 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 agoBecause 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 agook, so its only gc overhead, and no security issue with it?
- jameshart 5y agoNo. 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 agoThat'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 agoI 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 agoOSS 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 agoI 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.
- spullara 5y agoThis is just stupid. Logging should not do any side effects except writing to the log.
- dikei 5y agoI'm not defending Log4j, but this error can really happen to many logging libraries. All logging libraries contain some kind of template engine as a performance optimization, in order to avoid actually generating the output string (can be costly) if logging is disabled. And template engines have always been a major source of vulnerabilities.
- ninkendo 5y agoEliding the string interpolation doesn’t necessarily have to be a feature of the logging library, the language itself can have affordances for this. For example in swift, log.debug("my name is \(expensiveCalculate(name))") doesn’t have to evaluate “expensiveCalculate(name)” unless the logger actually opts to instantiate the string (which it can skip if say, debug logging is disabled.) This is because Swift’s string interpolation is implemented as lazily-evaluated closures, and all the “debug” method has to do is tag the input as an @autoclosure and it can avoid evaluation until it actually calls the closure. No templating is needed, just native string interpolation provided by the language.
- dikei 5y agoI would consider a programming language's native string interpolation as a form of templating, but to each his own.
- ninkendo 5y agoI was specifically responding to: > All logging libraries contain some kind of template engine as a performance optimization This doesn't need to be the case for some languages. But also, a language's native string interpolation doesn't need to be implemented as any sort of runtime template engine in the first place, because the compiler can compose the the components of the interpolated string at compile time. This compile-time/runtime split is IMO the key differentiator here, because runtime string interpolation is much more prone to bugs/RCE (since an untrusted string may show up as your template string.) To use an example, in swift, `f1("Some \(string) with \(vars)")` is fundamentally different than calling `f2("Some {} with {}", string, vars)`, because of the fact that such a method signature for "f2" may allow an untrusted string to show up as the first argument, which isn't possible in f1. (And no, an external string that happens to have \()'s in it will not be expanded... it has to be a string literal in the source code, which the compiler can see, for interpolation to happen.)
- fauria 5y ago> I'm still not quite sure why LDAP comes into the picture According to https://www.blackhat.com/docs/us-16/materials/us-16-Munoz-A-Journey-From-JNDI-LDAP-Manipulation-To-RCE.pdf https://www.blackhat.com/docs/us-16/materials/us-16-Munoz-A-... , SecurityManager is not enforced on remote class loading when using JNDI's LDAP server provider interface.
- isbvhodnvemrwvn 5y agoLet's be honest, barely anyone even uses SecurityManager in the real world.
- jameshart 5y agoEven the variable interpolation is a security risk. If I have the ability to trigger execution of a process on some service, and one of the things it does is return me the logs form that process, it might be somewhat surprising to the host of that process that if I can pass in "${java:vm}" as input, the logs might leak information about what version of the JVM it's running... What else might you be able to get a system to leak to you if you can control input and read log output?
- twic 5y ago> Turns out, by including "." in some part of the URL to this remote class, Log4j lets off its guard & simply looks up to that server and dynamically loads the class file. No it doesn't. That was disabled by default in 2009, and was disabled by default in every release of Java 8 or later: https://github.com/openjdk/jdk8u/commit/006e84fc77a582552e71a888db1be31407b783c7 https://github.com/openjdk/jdk8u/commit/006e84fc77a582552e71... Unless i am mistaken, i don't believe the attack as described by LunaSec actually works against a default-configured JVM released any time in the last decade.
- movover 5y agoThis is my understanding of it as well. While the bug is still bad due to the fact that a JVM instance will connect to the attacker's endpoint, any JVM above 8u121 wouldn't execute the code with Java's default configuration. It's also mentioned as part of the release notes for 8u121: https://www.oracle.com/java/technologies/javase/8u121-relnotes.html https://www.oracle.com/java/technologies/javase/8u121-relnot... Edit: Looking deeper into it; the JDK version used within the POC's GitHub, from the screenshot in that repo, is 8u20, released in 2014.
- twic 5y agoAha! It's more complicated than i thought: https://mbechler.github.io/2021/12/10/PSA_Log4Shell_JNDI_Injection/ https://mbechler.github.io/2021/12/10/PSA_Log4Shell_JNDI_Inj...
- grrrrrrreat 5y agoDoes this affect SL4j wrappers over log4j as well ?
- exec 5y agoYes. I managed to reproduce the issue with slf4j + log4j2.
- sathishv 5y agocan you please share code sample? and what versions you used? i am unable to replicate with log4j 1.2.12, slf4j 1.7.6, java8-151
- orgesballa 5y agoAffected versions >= 2.0.0, < 2.0.11 < 1.11.10 So you are safe!
- kikowi 5y agoDo you have any reference about < 1.11.10 being affected? I though versions 1.x are not affected.