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/19 21:04:23 UTC
[GitHub] [logging-log4j2] srdo opened a new pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
srdo opened a new pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644
I've hard coded the limit to 10. If you feel the limit needs to be configurable (via system property or the log4j config?), let me know and I'll adjust.
I couldn't find a good way to keep the call in line 987 to the protected method rather than the private method, while also keeping track of the recursion count. I'm not sure if that call is relied on by subclasses, I don't think the Javadoc promises a call to that method for each variable resolved.
--
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 a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775600190
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
+ maxRecursionDepth, buf.toString());
+ return 0;
Review comment:
I don't think zero is the correct result, should there be additional test coverage showing that?
edit: I think I'm incorrect here, otherwise the `isRecursiveEvaluationAllowed` check below would be wrong
--
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] Gamebuster19901 commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001075553
> For the `StatusLogger`, removing the log will mean a user hitting this limit won't know about it, which makes it harder for them to fix. Their logs will just be silently incorrect.
Since I was incorrect, and StatusLogger does not do string substitutuion, I have changed my mind. Logging the error should be fine.
--
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] quaff commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
quaff commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997665780
@srdo , I think your PR should be closed, `isRecursiveEvaluationAllowed()` is introduced by https://github.com/apache/logging-log4j2/commit/806023265f8c905b2dd1d81fd2458f64b2ea0b5e since version 2.17.0.
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997462128
@garydgregory Thanks, will close this and move it to another branch. I assume you cherry-pick/merge forward on this project then?
--
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] srdo edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001000089
> Should we return the input, or the pieces that we could successfully replace
Neither string is likely to be the output the author wanted I think. If this case is hit, the person monitoring the logs should become aware that there is a bug to fix, due to the status logger. I'm not sure if it makes a great difference which of the wrong log outputs they get back?
A minor benefit of returning the original string is that it might be easier to grep for?
The note about try-catching makes sense, I could add a try-catch to `substitute` so it is not possible to forget to handle the exception?
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001000089
> Should we return the input, or the pieces that we could successfully replace
Neither string is likely to be the output the author wanted I think. If this case is hit, the person monitoring the logs should become aware that there is a bug to fix, due to the status logger. I'm not sure if it makes a great difference which of the wrong log outputs they get back.
A minor benefit of returning the original string is that it might be easier to grep for?
The note about try-catching makes sense, I could add a try-catch to `substitute` so it is not possible to forget to handle the exception?
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001072449
@vy Could you elaborate on why you prefer this?
For the system property, can we come up with a case where people will actually need/want >10 (or some other limit) levels of recursion? If we don't think anyone will use it, why is it a good idea to add another configuration knob?
For the `StatusLogger`, removing the log will mean a user hitting this limit won't know about it, which makes it harder for them to fix. Their logs will just be silently incorrect.
--
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] Gamebuster19901 edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001072402
> I am in favor of reading the cap from a property and returning the lookup as is, i.e., no error logging via `StatusLogger`.
~~I agree with no error logging from StatusLogger,~~ but I believe there is no reason to make the cap a property.
EDIT: Since StatusLogger doesn't do string substitution, logging the error should be fine.
--
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 a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775600190
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
+ maxRecursionDepth, buf.toString());
+ return 0;
Review comment:
I don't think zero is the correct result, should there be additional test coverage showing that?
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1000926102
> I tend to agree with @carterkozak. According to my experience, these kind of measures have the potential to shadow the underlying problem. If the configuration has something dumb, let it blow, so that people can see and fix it.
>
> OTOH, what if somebody tomorrow comes up with a code path that circumvents the _"recursion is only allowed for configurations"_ condition? I guess everybody who down voted this PR will get crucified. 🤷♂️
This PR is better than knowing about a runaway train and letting it crash in traffic. Esp since we allow for recursion in the first place.
--
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] srdo edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001011990
The error messages are a little harder to understand now, since the logged error string will be whatever was being handled by `substitute` when the limit was exceeded, instead of the original string. On the other hand, we don't risk calling `substitute` without an appropriate try-catch anymore.
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001010713
Updated to avoid throwing a potentially unhandled exception. Also moved the max depth constant into the substitute method, and fixed a conflict with the base branch.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001070245
I am in favor of reading the cap from a property and returning the lookup as is, i.e., no error logging via `StatusLogger`.
--
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 a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775016870
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
Shouldn't this rather be bound to a property? `log4j2.substitution.maxRecursionDepth`
--
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] quaff commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
quaff commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997687041
> @quaff I don't think that fixes the same thing? This fix is trying to limit recursion depth in all cases, the fix you are linking disables recursion in some cases.
Limiting max recursion depth doesn't need anymore, since v2.17.0 it only allow recursion lookup from configuration which is controlled by developer, It's safe enough since attacker cannot change your configuration file and restart your application, even if endless recursion introduced by accident, you will know at the startup time.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997897168
> > @quaff I don't think that fixes the same thing? This fix is trying to limit recursion depth in all cases, the fix you are linking disables recursion in some cases.
>
> Limiting max recursion depth doesn't need anymore, since v2.17.0 it only allow recursion lookup from configuration which is controlled by developer, It's safe enough since attacker cannot change your configuration file and restart your application, even if endless recursion introduced by accident, you will know at the startup time.
We do want a limit when the feature is enabled. I will take a look later today.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-998413709
I'm not sure I entirely understand what we're protecting against -- I'd consider any recursion beyond what the configuration author expects to be an incredibly serious problem, while the difference in danger between `EXPECTED + 1` and `EXPECTED + 2` levels of recursion are not entirely clear.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775170010
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
Review comment:
It was my impression that StatusLogger just logs to stderr without doing anything fancy to the log message. Logging these lines in the tests don't seem to trigger infinite recursion.
It's possible this is a concern in a non-default configuration?
--
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] Gamebuster19901 commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001072402
> I am in favor of reading the cap from a property and returning the lookup as is, i.e., no error logging via `StatusLogger`.
I agree with no error logging from StatusLogger, but I believe there is no reason to make the cap a property.
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775102029
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
I disagree. If a developer needs a depth of more than 10, they should rethink their design. I would remove the `MAX_SUBSTITUITION_RECURSIONS` field altogether and hardcode the value of `10` in `checkRecursionEventNotExceeded`. Fields and system properties could potentially be modified by a malicious actor, and set to something ridiculous like `-1` or `Integer.MAX_VALUE`.
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775102029
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
I disagree. If a developer needs a depth of more than 10, they should rethink their design. I would remove the `MAX_SUBSTITUITION_RECURSIONS` altogether and hardcode the value of `10` in `checkRecursionEventNotExceeded`. Fields and system properties could potentially be modified by a malicious actor, and set to something ridiculous like `-1` or `Integer.MAX_VALUE`.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1000927711
When the recursion limit is hit, it's not clear whether we should return the original string, or avoid recursing further.
Consider `java version: ${java:version}, recursive value: ${[assume >10 layers of recursion]}`
Should we return the input: `java version: ${java:version}, recursive value: ${[assume >10 layers of recursion]}`
Or the pieces that we could successfully replace? `java version: 11.0.11, recursive value: ${[assume >10 layers of recursion]}`
Throwing is a bit different, prior to 2.17 where we added try/catch to StrSubstitutor it would prevent entire log events from being recorded, which probably isn't what we want. If this is called using a path which doesn't handle the exception, the case where depth is greater than the limit but less than the jvm max stack depth is worse off. I don't expect that case to be common.
I'm in favor of defense in depth, I also want to make sure that we're not adding complexity that unintentionally hinders our ability to maintain and secure this component :-)
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775165870
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
Review comment:
Won't this still cause a StackOverflowError? The bad string is passed in as a parameter into StatusLogger.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775170590
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
Review comment:
The `handleFailedReplacement` method in this class also logs the string it failed to do replacement in, so if this is a concern here, it might also be a concern for that method?
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775173387
##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java
##########
@@ -249,4 +249,37 @@ public void testTopLevelLookupsWithoutRecursiveEvaluationAndDefaultValueLookup()
subst.setRecursiveEvaluationAllowed(false);
assertEquals("two", subst.replace("${lower:${ctx:key1:-${ctx:key2}}}"));
}
+
+ @Test
+ public void testRecursionLimit() {
+ final Map<String, String> map = new HashMap<>();
+ for (int i = 0; i < 15; i++) {
+ int next = i + 1;
+ map.put("key" + i, "${key" + next +"}");
+ }
+ map.put("key15", "finalVal");
+ final StrLookup lookup = new Interpolator(new MapLookup(map));
+ final StrSubstitutor subst = new StrSubstitutor(lookup);
+ subst.setRecursiveEvaluationAllowed(true);
+ subst.setEnableSubstitutionInVariables(false);
Review comment:
Perhaps include a test which is the opposite of this. I.E. for when recursive evaluation is disabled, but substitution is allowed, as a sanity check.
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997681581
@quaff I don't think that fixes the same thing? This fix is trying to limit recursion depth in all cases, the fix you are linking disables recursion in some cases.
--
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 edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997464275
> @garydgregory Thanks, will close this and move it to another branch. I assume you cherry-pick/merge forward on this project then?
Yes, just a plain merge from GitHub works. Not sure why you need a new PR though.
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997461040
Jira https://issues.apache.org/jira/browse/LOG4J2-3259
--
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] jvz commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1019582347
I'm leaning toward closing this for a couple reasons:
* Setting a depth limit of something like 10 would still be vulnerable to billion-laughs style attacks.
* The issues that a depth limit would help prevent have already been fixed through other strategies.
* Adding a depth limit to this particular code is really pushing its complexity to a point where nobody can reasonably continue maintaining it.
Do you have a test or demo that could show what a depth limit of 10 (or any arbitrary depth limit) would help solve that the current implementation can't handle?
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1019592372
> Setting a depth limit of something like 10 would still be vulnerable to billion-laughs style attacks
Good point. I guess in order to prevent such an attack, you would need the method to stop recursing once the output string hits a certain configurable size as well?
> The issues that a depth limit would help prevent have already been fixed through other strategies.
I had the impression that what was fixed was the easy access to triggering this code with placeholders from application log strings, so there is no longer a known way for an attacker to get placeholders interpolated by this method short of controlling the logging config. I assume this change is what you are referring to, right?
The reason I wanted to add a recursion limit is that there is not a known good reason to allow unbounded recursion, and the unbounded method is dangerous if there happens to be some way for an attacker to get their own placeholders processed by it. Putting a wall around the potentially dangerous method is good, but then you have to be sure that there's no gap in that wall. I thought adding a limit would reduce the consequences if such a gap was found in the future. Such a gap could either be one that exists now and has been overlooked, or one that is introduced by accident in the future.
> Adding a depth limit to this particular code is really pushing its complexity to a point where nobody can reasonably continue maintaining it.
Okay, but not having a limit also imposes a maintenance burden, since all future developers need to be aware that this method is a potential landmine, and they need to be sure this method never ends up interpolating attacker-controlled placeholders again. I figured adding a limit was a way to reduce the burden on future developers.
> Do you have a test or demo that could show what a depth limit of 10 (or any arbitrary depth limit) would help solve that the current implementation can't handle?
No. I don't know of a way to get attacker-defined placeholders interpolated by this method anymore. The change was intended to make the failure mode less damaging, in case such a way is found in the future.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997464275
> @garydgregory Thanks, will close this and move it to another branch. I assume you cherry-pick/merge forward on this project then?
No, just a plain merge from GitHub works.
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997693611
You are right that 3230 fixes the vulnerability, I'm not opening this PR to fix a known problem. I'm coming at this from the point of view that if there is no reason to allow infinite recursion (which I don't think there is), it should not be possible at all. I think this is safer than allowing infinite recursion in some cases, when it is not needed. This will also make it safer to use the substitutor in future code, since it is not as dangerous if you accidentally allow recursion somewhere an attacker could insert a string.
--
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] srdo closed pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo closed pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001011990
The error messages are a little harder to understand now, since the logged string will be whatever was being handled by `substitute` when the limit was exceeded, instead of the original string. On the other hand, we don't risk the exception escaping from `substitute` anymore.
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775172956
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
Review comment:
It appears I was incorrect, StatusLogger does not have a StrSubstitutor.
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775173387
##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java
##########
@@ -249,4 +249,37 @@ public void testTopLevelLookupsWithoutRecursiveEvaluationAndDefaultValueLookup()
subst.setRecursiveEvaluationAllowed(false);
assertEquals("two", subst.replace("${lower:${ctx:key1:-${ctx:key2}}}"));
}
+
+ @Test
+ public void testRecursionLimit() {
+ final Map<String, String> map = new HashMap<>();
+ for (int i = 0; i < 15; i++) {
+ int next = i + 1;
+ map.put("key" + i, "${key" + next +"}");
+ }
+ map.put("key15", "finalVal");
+ final StrLookup lookup = new Interpolator(new MapLookup(map));
+ final StrSubstitutor subst = new StrSubstitutor(lookup);
+ subst.setRecursiveEvaluationAllowed(true);
+ subst.setEnableSubstitutionInVariables(false);
Review comment:
Perhaps include a test which is the opposite of this. I.E. for when recursive evaluation is disabled, but substitution is allowed.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1000844140
I tend to agree with @carterkozak. According to my experience, these kind of measures have the potential to shadow the underlying problem. If the configuration has something dumb, let it blow, so that people can see and fix it.
OTOH, what if somebody tomorrow comes up with a code path that circumvents the _"recursion is only allowed for configurations"_ condition? I guess everybody who down voted this PR will get crucified. :man_shrugging:
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-998546015
@carterkozak An issue like 3230 would have been much less serious if the substitutor weren't capable of infinite recursion. This is an attempt at harm reduction in case an issue like 3230 crops up again in the future, e.g. in new code. When recursion is limited, if another vector is discovered for an attacker (or unlucky regular user) to insert a string that would cause infinite recursion, the fallout will be that the log line is not interpolated. That type of bug is much less severe than a security 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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1038251524
Sounds good, I'll close this then.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1037900620
@carterkozak Does the PR you merged make this PR obsolete? My understanding is that recursion isn't allowed anywhere now.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1038248519
Yes, this is obsolete. We have much stricter controls over where recursion is and is not allowed.
--
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] srdo edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1038251524
Sounds good, I'll close this then.
For reference, the commit in question is https://github.com/apache/logging-log4j2/commit/17a77f5c27bda545de4b8eda46590401313e0be8
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1038251524
Sounds good, I'll close this then.
--
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] srdo closed pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo closed pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644
--
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 a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775600190
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
+ maxRecursionDepth, buf.toString());
+ return 0;
Review comment:
~~I don't think zero is the correct result, should there be additional test coverage showing that?~~
edit: I think I'm incorrect here, otherwise the `isRecursiveEvaluationAllowed` check below would be wrong
--
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] Gamebuster19901 edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001072402
> I am in favor of reading the cap from a property and returning the lookup as is, i.e., no error logging via `StatusLogger`.
~~I agree with no error logging from StatusLogger,~~ but I believe there is no reason to make the cap a property.
--
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] Gamebuster19901 commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1000924743
> I tend to agree with @carterkozak. According to my experience, these kind of measures have the potential to shadow the underlying problem. If the configuration has something dumb, let it blow, so that people can see and fix it.
As far as I can tell, it is 'blowing' by logging the fact that it could not write the string. It's just blowing safely instead of possibly causing a StackOverflowError if infinite recursion somehow occurs.
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775165870
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -986,12 +986,20 @@ protected boolean substitute(final LogEvent event, final StringBuilder buf, fina
* @param buf the string builder to substitute into, not null
* @param offset the start offset within the builder, must be valid
* @param length the length within the builder to be processed, must be valid
+ * @param recursionDepth the current recursion depth, strings exceeding 10 recursions
+ * will not be fully interpolated, and will cause an error to be logged.
* @param priorVariables the stack keeping track of the replaced variables, may be null
* @return the length change that occurs, unless priorVariables is null when the int
* represents a boolean flag as to whether any change occurred.
*/
private int substitute(final LogEvent event, final StringBuilder buf, final int offset, final int length,
- List<String> priorVariables) {
+ final int recursionDepth, List<String> priorVariables) {
+ int maxRecursionDepth = 10;
+ if (recursionDepth >= maxRecursionDepth) {
+ StatusLogger.getLogger().error("Property interpolation exceeded recursion depth of {}. Replacement failed on '{}'",
Review comment:
Won't this still cause a StackOverflowError? The bad string is passed in as a parameter into StatusLogger.getLogger(). Unless StatusLogger doesn't do string interpolation.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997461971
Hi @srdo
Wrong branch: You want to target `release-2.x`.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775126323
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
The reason I didn't put this in a property is that I don't think people are likely to need to adjust this. A configuration that needs 10 recursions to get to the final value is going to be hard to reason about, and is probably not a good idea even if log4j can technically do it. If you think 10 is too conservative, I'm happy to bump it up to something else.
If you feel strongly that this property is something people will need to adjust I'm also happy to add (and document) a system property for it, I just think it is a waste to do so unless you think people are going to need to modify the limit.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775127430
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
Otherwise, if people are fine with this being a fixed number (at least until some user actually needs configurability), I'll inline the constant as suggested. Let me know what you prefer :)
--
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] srdo edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1001011990
The error messages are a little harder to understand now, since the logged error string will be whatever was being handled by `substitute` when the limit was exceeded, instead of the original string. On the other hand, we don't risk the exception escaping from `substitute` anymore.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775176529
##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java
##########
@@ -249,4 +249,37 @@ public void testTopLevelLookupsWithoutRecursiveEvaluationAndDefaultValueLookup()
subst.setRecursiveEvaluationAllowed(false);
assertEquals("two", subst.replace("${lower:${ctx:key1:-${ctx:key2}}}"));
}
+
+ @Test
+ public void testRecursionLimit() {
+ final Map<String, String> map = new HashMap<>();
+ for (int i = 0; i < 15; i++) {
+ int next = i + 1;
+ map.put("key" + i, "${key" + next +"}");
+ }
+ map.put("key15", "finalVal");
+ final StrLookup lookup = new Interpolator(new MapLookup(map));
+ final StrSubstitutor subst = new StrSubstitutor(lookup);
+ subst.setRecursiveEvaluationAllowed(true);
+ subst.setEnableSubstitutionInVariables(false);
Review comment:
Done. That configuration is probably not very useful though, since it limits you to a single level of substitution in variable names (i.e. you can go `${key${key15}}` -> `${key16}` -> `finalVal`, but `${key${key14}}` will just resolve to `${key${key14}}`)
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1037900620
@carterkozak Does the PR you merged make this PR obsolete? My understanding is that recursion isn't allowed anywhere now.
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1038248519
Yes, this is obsolete. We have much stricter controls over where recursion is and is not allowed.
--
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] srdo closed pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo closed pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644
--
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] Gamebuster19901 commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
Gamebuster19901 commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775102137
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
I disagree. If a developer needs a depth of more than 10, they should rethink their design. I would remove the `MAX_SUBSTITUITION_RECURSIONS` field altogether and hardcode the value of `10` in `checkRecursionEventNotExceeded`. Fields and system properties could potentially be modified by a malicious actor, and set to something ridiculous like `-1` or `Integer.MAX_VALUE`.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775126323
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
The reason I didn't put this in a property is that I don't think people are likely to need to adjust this. A configuration that needs more than 10 recursions to get to the final value is going to be hard to reason about, and is probably not a good idea even if log4j can technically do it. If you think 10 is too conservative, I'm happy to bump it up to something else.
If you feel strongly that this property is something people will need to adjust I'm also happy to add (and document) a system property for it, I just think it is a waste to do so unless you think people are going to need to modify the limit.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775127430
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
Otherwise, if people are fine with this being a fixed number, I'll inline the constant as suggested. Let me know what you prefer :)
--
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 #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775075046
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
##########
@@ -168,6 +168,11 @@
private static final int BUF_SIZE = 256;
+ /**
+ * The maximum number of recursions performed during substitution.
+ */
+ private static final int MAX_SUBSTITUTION_RECURSIONS = 10;
Review comment:
> Shouldn't this rather be bound to a property? `log4j2.substitution.maxRecursionDepth`
I agree.
--
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] jvz commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1000927476
FWIW, I think making a specific finite limit on recursion is easier to configure than relying on the stack size at runtime to run out of space.
--
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] srdo commented on a change in pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#discussion_r775176529
##########
File path: log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java
##########
@@ -249,4 +249,37 @@ public void testTopLevelLookupsWithoutRecursiveEvaluationAndDefaultValueLookup()
subst.setRecursiveEvaluationAllowed(false);
assertEquals("two", subst.replace("${lower:${ctx:key1:-${ctx:key2}}}"));
}
+
+ @Test
+ public void testRecursionLimit() {
+ final Map<String, String> map = new HashMap<>();
+ for (int i = 0; i < 15; i++) {
+ int next = i + 1;
+ map.put("key" + i, "${key" + next +"}");
+ }
+ map.put("key15", "finalVal");
+ final StrLookup lookup = new Interpolator(new MapLookup(map));
+ final StrSubstitutor subst = new StrSubstitutor(lookup);
+ subst.setRecursiveEvaluationAllowed(true);
+ subst.setEnableSubstitutionInVariables(false);
Review comment:
Done. That configuration is probably not very useful though, since it limits you to a single level of substitution in variable names (i.e. you can go `${key${key15}}` -> `${key16}` -> `finalVal`, but `${key${key14}}` will just resolve to `${key${key14}}`)
Added a test that asserts that it works that way.
--
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] srdo edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-1038251524
Sounds good, I'll close this then.
For reference, the commit in question is https://github.com/apache/logging-log4j2/commit/17a77f5c27bda545de4b8eda46590401313e0be8
--
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 edited a comment on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997464275
> @garydgregory Thanks, will close this and move it to another branch. I assume you cherry-pick/merge forward on this project then?
Yes, just a plain merge from GitHub works.
--
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] quaff commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
quaff commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997717857
> You are right that 3230 fixes the vulnerability, I'm not opening this PR to fix a known problem. I'm coming at this from the point of view that if there is no reason to allow infinite recursion (which I don't think there is), it should not be possible at all. I think this is safer than allowing infinite recursion in some cases, when it is not needed. This will also make it safer to use the substitutor in future code, since it is not as dangerous if you accidentally allow recursion somewhere an attacker could insert a string.
I agree that will increase safety.
--
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] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #644:
URL: https://github.com/apache/logging-log4j2/pull/644#issuecomment-997469084
Sorry, I should have asked more clearly. What I meant was "How are you ensuring that this fix will also be applied to master, is it by cherry-pick or by merging 2.x into master periodically" :)
I'll reopen this and force push a 2.x version.
--
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