5 ms·

What benign purpose does this feature serve and why does it have to be implemented by parsing the input string? Does the input string get modified before being

by spuz 5y ago

What benign purpose does this feature serve and why does it have to be implemented by parsing the input string? Does the input string get modified before being written into the log? I'll ask again because the information presented so far both in this thread on GitHub and on Twitter has been very lacking: is it necessary to return the input string back to the attacker in the response to their request in order for them to exploit this bug as you are doing in your example code?

BeefWellington 5y ago
Best guess: Resolving some kind of entity name to a username for some weird auditing requirement few people have ever heard of. It's the kind of feature I've seen in some software in the past. That's just a guess though.
- QuercusMax 5y ago
  That sounds like exactly the kind of ridiculous feature that would lead to a Java RCE
- Thorrez 5y ago
  The question isn't about the purpose of the feature. The question is why it's implemented with string parsing. In C, it's unsafe to do printf(string_variable); because variable will get parsed as a format string. The way to solve the vulnerability is printf("%s", string_variable); Is that the same in Java logging libraries? Is it well-known that the logged value will be parsed? What's the safe way to log a value in Java exactly, knowing that nothing will parse that value?
  - didibus 5y ago
    The RCE isn't in the parsing. What gets parsed is a string that tells the server to make a request to another server. If you use a weird protocol for that, like jndi:ldap, you can then return a class which will be automatically loaded. So the code injection happens as the response to the remote request. The part the logger plays is that you can initiate that remote request by having the logger log some special string.
    - ec109685 5y ago
      They are arguing that it’s weird that a user supplied string is ever “parsed” versus printf type behavior.
    - Thorrez 5y ago
      I think there are 2 problems: 1. Attacker-controlled data is being parsed as code (format code, not Java code). I'm not sure to what degree this is the logging library's fault vs programmer error passing attacker-controlled data as a format string. I know in Go, the standard libraries take care to help programmers avoid this problem by making sure to have the character "f" in the function name to indicate the function parameter is a format string. log.Print() takes data, log.Printf() takes a format string, log.Fatal() takes data, log.Fatalf() takes a format string. 2. The format string syntax contains significantly exploitable features if an attacker can control it. This is the same as in C, because in C, printf() contains exploitable features. This is not the case in Go, because the worst the attacker can do in Go is cause the formatting to be strange or the .String() or .Error() methods to be called on the other inputs. Note that even without 2, 1 is still a correctness problem. If I want to log attacker-controlled data, I want it to display accurately. If the attacker's User-Agent header contains weird characters, I want those to be logged exactly, not inadvertently transformed into something strange by my library.
  - vbezhenar 5y ago
    It's not the same in Java logging libraries. You can write `logger.info("a={}")` and it won't cause any issues. At least that was the case before I read about this misfeature. I still think that log4j did absolutely wrong thing, because there are billions of lines like `logger.info("a=" + a)` and nobody's going to rewrite those lines. Breaking trust in logging library doing sane thing is absolutely terrible approach. I'm going to stick to logback everywhere I can, hopefully they did not do those stupid things. Safe way should be something like `logger.info("a={}", a)`. Of course nobody's preventing log4j to parse any argument in any way they like. And they actually do. So with log4j there's no safe way it seems.
  - isbvhodnvemrwvn 5y ago
    You can try to use context in the logging messages, e.g. the ID of the entity you are currently processing (from the HTTP request), but which might for whatever reason not be available in the code (e.g. you don't want to drill it several methods deep) As a possible solution you can set MDC variables at the higher level, they are bound to the current thread, and reference them in the format string, and unset them after processing the entity. It's not a great solution due to temporal coupling, and you typically print it outside of an individual logging statements (e.g. add it to all logging statements), but it definitely beats drilling dozens of methods with the diagnostic identifiers.
hinkley 5y ago
Get yer pointin' finger ready: https://github.com/apache/logging-log4j2/blame/master/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/JndiLookup.java https://github.com/apache/logging-log4j2/blame/master/log4j-...
- im3w1l 5y ago
  People should really have known better than to use (unexpected!) in-band signalling. Even 8 years ago.
  - hinkley 5y ago
    I’m still arguing with people about using the built in url APIs instead of string concatenation. A person is smart, but people are dumb.
twic 5y ago
JNDI is an interface to many kinds of naming and directory services. One common-ish use case in enterprise software is to use it as a sort of internal directory inside an application. In particular, the prefix "java:comp/env" identifies a namespace which contains configuration for the current component (servlet or EJB). So it might be rather useful to do a lookup in that when writing log messages. For example, you could have some common utility method shared by multiple components that looks up the current component name and includes it in the log output, so you can tell which component was making a request to the utility method. The easiest way to support this would just be to allow JNDI lookups from log strings. Unfortunately, that enables all sorts of lookups! IMHO, the real bug here is that the LDAP JNDI provider will load class files from arbitrary untrusted sources. That is an obviously terrible idea, regardless of whether JNDI is being used from a logging string or somewhere else.
immibis 5y ago
To my knowledge JNDI is a monitoring interface allowing you to get various system values like the number of threads, heap size, process ID, etc. Somehow it's capable of being connected to LDAP. It sounds like a classic X-to-Y-to-Z problem. Someone connected X to Y thinking Y was pretty safe even with untrusted input, not knowing it would ever proxy to Z (probably, everything you can do with JNDI on a local machine is safe). Someone else connected Y to Z thinking that Y is only passed trusted values so the new proxy feature could only be triggered deliberately. And here we are. This class of bug should have a name but probably doesn't. And another person didn't document well that log messages must be trusted input...
- mtnygard 5y ago
  Too many acronyms. :-) JNDI was the "Java Naming and Directory Interface", part of the suite of CORBA-like Java-only distributed object tooling. My guess is that the "lookups" feature in log4j assumed that all URL protocols were "http:" or "https:" and didn't account for either the full set of built-in protocol handlers or the fact that an application can register additional protocol handlers.

by spuz 5y ago

BeefWellington 5y ago

Best guess: Resolving some kind of entity name to a username for some weird auditing requirement few people have ever heard of. It's the kind of feature I've seen in some software in the past. That's just a guess though.

QuercusMax 5y ago
That sounds like exactly the kind of ridiculous feature that would lead to a Java RCE
Thorrez 5y ago
The question isn't about the purpose of the feature. The question is why it's implemented with string parsing. In C, it's unsafe to do printf(string_variable); because variable will get parsed as a format string. The way to solve the vulnerability is printf("%s", string_variable); Is that the same in Java logging libraries? Is it well-known that the logged value will be parsed? What's the safe way to log a value in Java exactly, knowing that nothing will parse that value?
- didibus 5y ago
  The RCE isn't in the parsing. What gets parsed is a string that tells the server to make a request to another server. If you use a weird protocol for that, like jndi:ldap, you can then return a class which will be automatically loaded. So the code injection happens as the response to the remote request. The part the logger plays is that you can initiate that remote request by having the logger log some special string.
  - ec109685 5y ago
    They are arguing that it’s weird that a user supplied string is ever “parsed” versus printf type behavior.
  - Thorrez 5y ago
    I think there are 2 problems: 1. Attacker-controlled data is being parsed as code (format code, not Java code). I'm not sure to what degree this is the logging library's fault vs programmer error passing attacker-controlled data as a format string. I know in Go, the standard libraries take care to help programmers avoid this problem by making sure to have the character "f" in the function name to indicate the function parameter is a format string. log.Print() takes data, log.Printf() takes a format string, log.Fatal() takes data, log.Fatalf() takes a format string. 2. The format string syntax contains significantly exploitable features if an attacker can control it. This is the same as in C, because in C, printf() contains exploitable features. This is not the case in Go, because the worst the attacker can do in Go is cause the formatting to be strange or the .String() or .Error() methods to be called on the other inputs. Note that even without 2, 1 is still a correctness problem. If I want to log attacker-controlled data, I want it to display accurately. If the attacker's User-Agent header contains weird characters, I want those to be logged exactly, not inadvertently transformed into something strange by my library.
- vbezhenar 5y ago
  It's not the same in Java logging libraries. You can write `logger.info("a={}")` and it won't cause any issues. At least that was the case before I read about this misfeature. I still think that log4j did absolutely wrong thing, because there are billions of lines like `logger.info("a=" + a)` and nobody's going to rewrite those lines. Breaking trust in logging library doing sane thing is absolutely terrible approach. I'm going to stick to logback everywhere I can, hopefully they did not do those stupid things. Safe way should be something like `logger.info("a={}", a)`. Of course nobody's preventing log4j to parse any argument in any way they like. And they actually do. So with log4j there's no safe way it seems.
- isbvhodnvemrwvn 5y ago
  You can try to use context in the logging messages, e.g. the ID of the entity you are currently processing (from the HTTP request), but which might for whatever reason not be available in the code (e.g. you don't want to drill it several methods deep) As a possible solution you can set MDC variables at the higher level, they are bound to the current thread, and reference them in the format string, and unset them after processing the entity. It's not a great solution due to temporal coupling, and you typically print it outside of an individual logging statements (e.g. add it to all logging statements), but it definitely beats drilling dozens of methods with the diagnostic identifiers.

hinkley 5y ago

Get yer pointin' finger ready: https://github.com/apache/logging-log4j2/blame/master/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/JndiLookup.java https://github.com/apache/logging-log4j2/blame/master/log4j-...

im3w1l 5y ago
People should really have known better than to use (unexpected!) in-band signalling. Even 8 years ago.
- hinkley 5y ago
  I’m still arguing with people about using the built in url APIs instead of string concatenation. A person is smart, but people are dumb.

twic 5y ago

JNDI is an interface to many kinds of naming and directory services. One common-ish use case in enterprise software is to use it as a sort of internal directory inside an application. In particular, the prefix "java:comp/env" identifies a namespace which contains configuration for the current component (servlet or EJB). So it might be rather useful to do a lookup in that when writing log messages. For example, you could have some common utility method shared by multiple components that looks up the current component name and includes it in the log output, so you can tell which component was making a request to the utility method. The easiest way to support this would just be to allow JNDI lookups from log strings. Unfortunately, that enables all sorts of lookups! IMHO, the real bug here is that the LDAP JNDI provider will load class files from arbitrary untrusted sources. That is an obviously terrible idea, regardless of whether JNDI is being used from a logging string or somewhere else.

immibis 5y ago

To my knowledge JNDI is a monitoring interface allowing you to get various system values like the number of threads, heap size, process ID, etc. Somehow it's capable of being connected to LDAP. It sounds like a classic X-to-Y-to-Z problem. Someone connected X to Y thinking Y was pretty safe even with untrusted input, not knowing it would ever proxy to Z (probably, everything you can do with JNDI on a local machine is safe). Someone else connected Y to Z thinking that Y is only passed trusted values so the new proxy feature could only be triggered deliberately. And here we are. This class of bug should have a name but probably doesn't. And another person didn't document well that log messages must be trusted input...

mtnygard 5y ago
Too many acronyms. :-) JNDI was the "Java Naming and Directory Interface", part of the suite of CORBA-like Java-only distributed object tooling. My guess is that the "lookups" feature in log4j assumed that all URL protocols were "http:" or "https:" and didn't account for either the full set of built-in protocol handlers or the fact that an application can register additional protocol handlers.