7 ms·

I'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 performanc

by dikei 5y ago

I'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 ago
Eliding 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 ago
  I would consider a programming language's native string interpolation as a form of templating, but to each his own.
  - ninkendo 5y ago
    I 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.)
- Spivak 5y ago
  While nice this feature doesn't really exist in other programming languages that don't involve the caller unergonomically wrapping the parameter to defer evaluation.
- andrebreves 5y ago
  It's coming to Java (soon I hope): https://openjdk.java.net/jeps/8273943 https://openjdk.java.net/jeps/8273943
acdha 5y ago
I think you’re right but also showing where a more secure design could solve it just like we’ve seen for SQL injection. The problem is that the first parameter can either be a format string or data. If the signature was different so it only used instances of a FormatString class to get dynamic behavior, this problem would be avoided, but I’m sure a lot of people would complain about the extra typing. What’d be really cool would be if Java supported something like this Rust lifetime hack to let that be implicit where you could disable dynamic functionality for strings created after startup. https://polyfloyd.net/post/compile-time-prevention-of-sql-injections/ https://polyfloyd.net/post/compile-time-prevention-of-sql-in...
wyager 5y ago
There are several ways to avoid generating log strings that don't involve sketchy ad-hoc templating. Use a language with laziness, put the string generation in a lambda expression, use a language with a good compiler and don't put side effects in your log string expression, etc. As a matter of principle, it should also clearly not be possible for a templating engine to perform any kind of side effect. That's totally crazy.
rixed 5y ago
Apparently, from recent posts on this page, the problem is not solely caused by the fact that log4j perform some substitutions from a preset map, but that it performs substitution recursively, thus interpreting again the data injected into the string, with apparently no way to escape it, which I think very few logging libraries are doing. Imagine, you write SQL commands using the proper parametric form, such as: > exec("select * where id=${1}", user_provided_login); You would be right to expect the DB library to escape the string so that no SQL injection is possible. After all, isn't that the whole point of parameters over a mere > exec('select * where id=' + user_provided_login); ? Well, apparently log4j is doing the equivalent of treating 'user_provided_login' as legit SQL. This is especially problematic because not only will it substitute some '${variables}' again at remote user discretion, but some '${special.forms}' can actually instruct log4j to connect anywhere, download some code, execute it, then print the output. Because that was deamed convenient to someone in the past who complained that feature was missing and submitted a patch which, because of stellar unit tests, passed the code review. The only context I can think of where this behavior regarding recursive substitution is acceptable is text templating, away from possibly adversarial input. I believe it goes opposite to expectations when logging.
- mike22 5y ago
  > You would be right to expect the DB library to escape the string so that no SQL injection is possible. SQL in some (hopefully good number of) cases is much safer than that. Going with MySQL prepared statements here: parameters are not sustituted into the SQL statement string, but rather sent as seperate data packets in the wire protocol.
andrebreves 5y ago
Nowadays I guess the logging methods using lambda to supply the log string fix the performance problem

by dikei 5y ago

ninkendo 5y ago

Eliding 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 ago
I would consider a programming language's native string interpolation as a form of templating, but to each his own.
- ninkendo 5y ago
  I 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.)
Spivak 5y ago
While nice this feature doesn't really exist in other programming languages that don't involve the caller unergonomically wrapping the parameter to defer evaluation.
andrebreves 5y ago
It's coming to Java (soon I hope): https://openjdk.java.net/jeps/8273943 https://openjdk.java.net/jeps/8273943

acdha 5y ago

I think you’re right but also showing where a more secure design could solve it just like we’ve seen for SQL injection. The problem is that the first parameter can either be a format string or data. If the signature was different so it only used instances of a FormatString class to get dynamic behavior, this problem would be avoided, but I’m sure a lot of people would complain about the extra typing. What’d be really cool would be if Java supported something like this Rust lifetime hack to let that be implicit where you could disable dynamic functionality for strings created after startup. https://polyfloyd.net/post/compile-time-prevention-of-sql-injections/ https://polyfloyd.net/post/compile-time-prevention-of-sql-in...

wyager 5y ago

There are several ways to avoid generating log strings that don't involve sketchy ad-hoc templating. Use a language with laziness, put the string generation in a lambda expression, use a language with a good compiler and don't put side effects in your log string expression, etc. As a matter of principle, it should also clearly not be possible for a templating engine to perform any kind of side effect. That's totally crazy.

rixed 5y ago

Apparently, from recent posts on this page, the problem is not solely caused by the fact that log4j perform some substitutions from a preset map, but that it performs substitution recursively, thus interpreting again the data injected into the string, with apparently no way to escape it, which I think very few logging libraries are doing. Imagine, you write SQL commands using the proper parametric form, such as: > exec("select * where id=${1}", user_provided_login); You would be right to expect the DB library to escape the string so that no SQL injection is possible. After all, isn't that the whole point of parameters over a mere > exec('select * where id=' + user_provided_login); ? Well, apparently log4j is doing the equivalent of treating 'user_provided_login' as legit SQL. This is especially problematic because not only will it substitute some '${variables}' again at remote user discretion, but some '${special.forms}' can actually instruct log4j to connect anywhere, download some code, execute it, then print the output. Because that was deamed convenient to someone in the past who complained that feature was missing and submitted a patch which, because of stellar unit tests, passed the code review. The only context I can think of where this behavior regarding recursive substitution is acceptable is text templating, away from possibly adversarial input. I believe it goes opposite to expectations when logging.

mike22 5y ago
> You would be right to expect the DB library to escape the string so that no SQL injection is possible. SQL in some (hopefully good number of) cases is much safer than that. Going with MySQL prepared statements here: parameters are not sustituted into the SQL statement string, but rather sent as seperate data packets in the wire protocol.

andrebreves 5y ago

Nowadays I guess the logging methods using lambda to supply the log string fix the performance problem