9 ms·
> This looks worrying, but if you read the issue thread it seems that this can only be triggered if you can edit the pattern string of the logger. This is somet
by koolba 5y ago
> This looks worrying, but if you read the issue thread it seems that this can only be triggered if you can edit the pattern string of the logger. This is something you can only do on the server itself (if made configurable) or in the build artefact that you deploy.
Plenty of existing code does things like:
log.info(“foo: “ + request.getFoo());
Rather than using fixed format strings and {} place holders. You’re not supposed to, but it’s far from uncommon.
- karatinversion 5y agoOh yes, I’ve seen several programs that spend ~30% of their CPU cycles formatting strings that are immediately thrown away because the log level is not high enough. Now you can also include vulnerabilities with no extra effort!
- dmurray 5y agoIn Java? Surely one of the benefits of the JIT is to compile logger.debug() into a no-op if the log level is not high enough.
- clon 5y agoYou would still be doing the job of concatenating together the error message, possibly rendering some complex objects to a string, that is then fed to the no-op. The point parent was making is that there is also a performance aspect to this, in addition to the security aspect.
- chrisseaton 5y ago> You would still be doing the job of concatenating together the error message, possibly rendering some complex objects to a string, that is then fed to the no-op. Why would you still do be doing that work? If a value goes into a no-op, then the value isn't computed. (In theory - I'm sure it doesn't always work out 100% of the time in practice.)
- christophilus 5y agoUnless the JVM is sure the logic is not effectful, it couldn’t eliminate it.
- chrisseaton 5y agoThe JVM knows what string concatenation does.
- envp 5y agoBut what if one of the arguments is a function call? Then it isn't easy to prove there are no side effects
- chrisseaton 5y agoIf it’s too big to in-line then yes.
- vlovich123 5y agoThat’s says absolutely nothing about whether the function has a side effect.
- chrisseaton 5y agoIf you can inline it into the compilation unit, then you can see if it has side effects or not.
- deleted 5y ago[deleted]
- alserio 5y agoIsn't the point that you don't generally stuff your log of things that are only strings but of things that can become strings? (Asking since I know of your work with truffle)
- isbvhodnvemrwvn 5y agoLogger levels are mutable to allow switching them at runtime, JIT can not make an assumption that the log level will stay the same.
- akvadrako 5y agoSure it can, since the JIT has access to runtime information and all the code. It could reorder the steps so first the log level is checked, then that block is run.
- hamburglar 5y agolog.debug(“foo count: “ + fooHandler.incrementAndReturnFooCount()); Should the JIT call incrementAndReturnFooCount if debug logs are disabled? This is a long-recognized pitfall of C preprocessor macros that look like function calls but may simply be defined away, causing unexpected behavior.
- akvadrako 5y agoIt should call it if it has side effects, otherwise not. Java isn't C; the runtime can be a lot smarter.
- hamburglar 5y agoI guess I’m not up on the state of the art with respect to the JRE’s ability to determine whether arbitrary code has side effects. That’s quite a broad problem. If I write fizzbuzz but use log.debug for output, does it optimize my main() to be empty unless I run it with debug enabled?
- alserio 5y agoI believe the problem is just that the toString methods implicitly called when concatenating strings and objects, can execute side effects. And the JIT has to keep those effects if it cannot prove they are just heating up your CPU
- millerm 5y agoHow could it? The log level is not a constant. It is evaluated at runtime, every time. You can’t compile that out. Am I wrong?
- alserio 5y agoThe JIT works by making assumptions using runtime information, and by discarding compiled code when the conditions change and the assumptions are not valid anymore.
- vbezhenar 5y agoJVM is not that smart.
- jboy55 5y agoYears ago we had a system that was performing really badly. It turns out that it spent over 50% of its time on log.debug(xmldoc) The debug method took a string, and Java was converting the xmldoc to a string. This was back in the 1.4/1.5 days.
- planb 5y agoUp until last week, I thought you’re not supposed to do this out of performance reasons (string needs to be evaluated even if not logged). That log4j does anything else on this string than writing it to some log destination is the real problem and the cause of all these bugs…
- rcxdude 5y agoIt's also a very bad idea in e.g. C, where printf has all kinds of exploitable behaviour if the attacker controls the format string passed into it. Format strings are a kind of code, and should be treated as such when considering injection attacks (by far the worst aspect of the log4j bug what that it would expand these strings on the final formatted strings: this was completely unnecessary for the features they were aiming to support and made the vulnerability even worse).
- layer8 5y agoSince version 2.16, log4j doesn’t perform such substitutions on the message string anymore, unless explicitly requested by the log pattern in the configuration (i.e. by `%msg{lookup}`).
- deleted 5y ago[deleted]
- layer8 5y agoThat makes no difference for the ${…} substitutions, which also aren’t applied to messages anymore. The new issue is about log patterns defined in the appender configurations.
- thom 5y agoI thought log4j performed no escaping so even if you use its logging calls that take parameters you’re still vulnerable?
- layer8 5y agoThat was changed with version 2.16. The lookups are not applied to either the message format or the message arguments anymore, only to log patterns defined in the configuration.
- nyuszika7h 5y agoThis is why Python's logging module treats the argument as a literal string without performing expansions if you only give it a single argument. That avoids such issues.
- acdha 5y agoThis is really making me feel better about badgering our contractors to use format strings for effective logging to Sentry.
- layer8 5y agoUsing format strings makes no difference for the log4j vulnerabilities. The stage at which the vulnerable substitutions are applied is after the application-level format string has already been formatted. The vulnerabilities occur at the level of applying the Appender log pattern, which is a mechanism separate from the application-level format strings.
- acdha 5y agoAh, the person I was replying to had conflated the two? I retract my feeling of relief, as is the course for this week.
- layer8 5y agoTo be fair, most descriptions of the vulnerabilities and of the fixes do a bad job of explaining when and where the substitutions are performed and how they (don’t) relate to application-level format strings.
- skybrian 5y agoIt seems like the ability to declare a parameter to be a constant string (no runtime computation allowed) would have been useful here, or in any printf-like API. If you really want to do that, it could be using a different function.
- ericbarrett 5y agoI think Go's log package, and similar, have the best take on this: log.Print(fixed_string) log.Printf(format_string, args...) Simple, unambiguous.
- skybrian 5y agoWell, almost. If you change Print with one argument to Printf, the compiler doesn’t complain. Maybe lint or vet will warn about it. If it were a special type then the compiler could do it.
- rixed 5y agoJust for the record, it is possible to type check format strings with regard to their actual arguments. OCaml does this, for instance, probably since its inception ~30 years ago or so.