5 ms·

Here's the alleged original Jira ticket: https://issues.apache.org/jira/browse/LOG4J2-313 https://issues.apache.org/jira/browse/LOG4J2-313 PS: Don't post snark

Here's the alleged original Jira ticket: https://issues.apache.org/jira/browse/LOG4J2-313 https://issues.apache.org/jira/browse/LOG4J2-313 PS: Don't post snarky comments on an 8-year old Jira ticket, please.

jancsika 5y ago
Thanks, I'll have a look. Also, good call on your PS. :)
technion 5y ago
I have read that ticket and the JNDI documentation multiple times and I still can't picture what it would look like to choose to use this. Can anyone show me a codebase that utilises this feature so we can see why?
- deleted 5y ago
  [deleted]
- AnotherGoodName 5y ago
  After reading through it's a pretty bad feature. It should have been blocked. The very nature of what it does is terrible. Some background: JNDI alone is fine. You want to load code remotely and run it? Fine. You can do that already in other ways. No issue with JNDI here, go ahead and use it on your server in a controlled way that doesn't involve user input. Log4j has a 'routing appender' that can conditionally write to different logs depending on the content being logged. https://logging.apache.org/log4j/2.x/manual/appenders.html#RoutingAppender https://logging.apache.org/log4j/2.x/manual/appenders.html#R... I can see a use to string match and send logs different ways. Now unfortunately this patch flat out uses the 'routing appender' to look for incoming log statements with the pattern ${jndi:logging/context-name} and load that remote JNDI class. This is such a terrible idea that doesn't pass the sniff test. The person who approved this should have simply read the description of what it does. After matching the pattern ${jndi:logging/context-name} it puts that match into a string 'key' and runs ctx.lookup(convertJndiName(key)); It's similar to someone submitting a patch that says "I want to run eval(user_input)". The only difference is that lookup(convertJndiName()) is a little bit obfuscated since it's not called eval(). I guess the review could be mistaken that it's harmless?. Still i think it's a bad smell. I'm worried for this project. It's probably worth going through everything that 'implements StrLookup' and seeing what they do in the 'lookup(final LogEvent event, final String key)' function. Both event and key are user generated content.
- twic 5y ago
  The key thing is that one of the directory services you can use via JNDI is "java", which is a process-local configuration store present by default in enterprise (Java EE) applications. An enterprise web server can run multiple "applications" at once, and each one gets its own configuration in the java directory service. So, if you have defined a configuration variable app-name-for-logging, then you can look up: java:comp/env/app-name-for-logging to get the value. If you are writing a generic logging utility which works across many applications, it might be useful to use this to choose the log file, or just include it in the log output. The easiest implementation of this was just to allow generic JNDI lookups, which includes LDAP. What i can't explain is why anyone would legitimately use this feature to make JNDI lookups. It should have been scoped to only lookups in java:comp/env, not arbitrary ones.
gonzo41 5y ago
What an amazing thing to see, from birth to fiery explosion and then death. That feature may just be the most impactful thing that programmer did ever. And I'm not trying to be snarky, bugs happen, and so do bad features. It's just amazing the blast radius of this thing.
albert_e 5y ago
interesting that this feature request was turned around in one day!
- lnx01 5y ago
  The feature request and feature were written by the same person.
siva7 5y ago
Which bugs the question who is that guy that introduced the patch?
- raverbashing 5y ago
  While I can see why this would be useful (in very strict cases), one can always wonder if there was an ulterior motive to developing this "feature" And of course, it's one thing to parse a jdni entry in a config setting, another to parse every single log message
dt3ft 5y ago
Too late, someone already posted this comment 2 days ago: "nice work ;)"
- abledon 5y ago
  poor woonsan :(
kune 5y ago
The change request talks about configuration files as the Apache documentation. Why did they use it for messages? Was this misunderstood?
- zorr 5y ago
  I haven't validated this assumption but it looks like the patch was rather naive and added the jndi lookup to the Interpolator class and only tested for its usage in the configuration. Being unaware that the interpolator also runs on the full message.

jancsika 5y ago

Thanks, I'll have a look. Also, good call on your PS. :)

technion 5y ago

I have read that ticket and the JNDI documentation multiple times and I still can't picture what it would look like to choose to use this. Can anyone show me a codebase that utilises this feature so we can see why?

deleted 5y ago
[deleted]
AnotherGoodName 5y ago
After reading through it's a pretty bad feature. It should have been blocked. The very nature of what it does is terrible. Some background: JNDI alone is fine. You want to load code remotely and run it? Fine. You can do that already in other ways. No issue with JNDI here, go ahead and use it on your server in a controlled way that doesn't involve user input. Log4j has a 'routing appender' that can conditionally write to different logs depending on the content being logged. https://logging.apache.org/log4j/2.x/manual/appenders.html#RoutingAppender https://logging.apache.org/log4j/2.x/manual/appenders.html#R... I can see a use to string match and send logs different ways. Now unfortunately this patch flat out uses the 'routing appender' to look for incoming log statements with the pattern ${jndi:logging/context-name} and load that remote JNDI class. This is such a terrible idea that doesn't pass the sniff test. The person who approved this should have simply read the description of what it does. After matching the pattern ${jndi:logging/context-name} it puts that match into a string 'key' and runs ctx.lookup(convertJndiName(key)); It's similar to someone submitting a patch that says "I want to run eval(user_input)". The only difference is that lookup(convertJndiName()) is a little bit obfuscated since it's not called eval(). I guess the review could be mistaken that it's harmless?. Still i think it's a bad smell. I'm worried for this project. It's probably worth going through everything that 'implements StrLookup' and seeing what they do in the 'lookup(final LogEvent event, final String key)' function. Both event and key are user generated content.
twic 5y ago
The key thing is that one of the directory services you can use via JNDI is "java", which is a process-local configuration store present by default in enterprise (Java EE) applications. An enterprise web server can run multiple "applications" at once, and each one gets its own configuration in the java directory service. So, if you have defined a configuration variable app-name-for-logging, then you can look up: java:comp/env/app-name-for-logging to get the value. If you are writing a generic logging utility which works across many applications, it might be useful to use this to choose the log file, or just include it in the log output. The easiest implementation of this was just to allow generic JNDI lookups, which includes LDAP. What i can't explain is why anyone would legitimately use this feature to make JNDI lookups. It should have been scoped to only lookups in java:comp/env, not arbitrary ones.

gonzo41 5y ago

What an amazing thing to see, from birth to fiery explosion and then death. That feature may just be the most impactful thing that programmer did ever. And I'm not trying to be snarky, bugs happen, and so do bad features. It's just amazing the blast radius of this thing.

albert_e 5y ago

interesting that this feature request was turned around in one day!

lnx01 5y ago
The feature request and feature were written by the same person.

siva7 5y ago

Which bugs the question who is that guy that introduced the patch?

raverbashing 5y ago
While I can see why this would be useful (in very strict cases), one can always wonder if there was an ulterior motive to developing this "feature" And of course, it's one thing to parse a jdni entry in a config setting, another to parse every single log message

dt3ft 5y ago

Too late, someone already posted this comment 2 days ago: "nice work ;)"

abledon 5y ago
poor woonsan :(

kune 5y ago

The change request talks about configuration files as the Apache documentation. Why did they use it for messages? Was this misunderstood?

zorr 5y ago
I haven't validated this assumption but it looks like the patch was rather naive and added the jndi lookup to the Interpolator class and only tested for its usage in the configuration. Being unaware that the interpolator also runs on the full message.