[GitHub] [logging-log4j2] srdo commented on pull request #644: LOG4J2-3259: Limit max recursion depth when interpolating strings.

[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