Thanks for raising this discussion Volkan. I have a few points I'd like to make sure have been considered:
The intent is that the limit should be set sufficiently high that no one is realistically going to want to exceed it. I'm happy to make it adjustable if you think people will need it, I just figured it wasn't worth doing until there's a known use case. Staying quiet is going to be an issue in case the limit is hit. It will be very hard for users to understand why some log lines are not being properly interpolated if there are no errors in the logging explaining why. I don't think hiding the error is helpful for users. I'm also having trouble understanding why using the StatusLogger is problematic in this case, when it is already being used in StrSubstitutor for other errors (see https://github.com/apache/logging-log4j2/blob/6a21ada4da85445e7b701d1d106fe44607829910/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java#L461). If the recursion limit is hit, there is likely an error in the user's configuration, so I'm not sure why hiding it is preferable. Throttling how often the error is logged might be fine, but I think swallowing the error would be a mistake, and would just confuse users. Regarding "I'd consider any recursion beyond what the configuration author expects to be an incredibly serious problem", I think the concern is valid, but I don't think it's an argument against reducing the potential blast radius on the recursion feature. If anything, it is an argument in favor of disabling recursion by default (i.e. allowing only depth 0), and allowing users to opt-in to higher depths via the log4j config. Would that be a preferable solution? If recursion can't be disabled by default due to backwards compatibility concerns, I think it at least makes sense to try to limit the potential damage for future code. Even if there are no loopholes to circumvent the recent vulnerability fix in this area, leaving the substitutor capable of unbounded recursion I feel is leaving a landmine for future log4j developers, especially when there is no known use case for more than a recursion depth higher than e.g. 10. Den man. 3. jan. 2022 kl. 19.16 skrev Patrick Mills <[email protected]>: > I would think an option to silence would be ok if off by default. I > wouldn't want it, but some may. > > As for the return, empty would be better than partial as that may open bad > values - not sure that could be abused, but if an attacker figured out the > limit, they may be able to craft something bad that remains once the limit > is reached. > > Agree, that anytime the limit is reached, it really is a misconfiguration. > > On Mon, Jan 3, 2022 at 11:32 AM Ralph Goers <[email protected]> > wrote: > > > I am in favor of limiting the recursion depth to a configurable with a > > default number. > > I fully expect virtually all normal usage would fall within the limits of > > whatever we > > would pick as the default. > > > > But should that limit be hit we can’t just return crap silently. It > almost > > certainly means > > something bad is going on that the user needs to be informed of. We can > > certainly use > > something like ErrorHandler to limit the frequency the user is informed. > > > > Ralph > > > > > > > On Jan 3, 2022, at 5:41 AM, Volkan Yazıcı <[email protected]> wrote: > > > > > > There is a PR <https://github.com/apache/logging-log4j2/pull/644> for > > this > > > issue, shall we bring this to a conclusion? > > > > > > I am in favor of this PR if the following changes were implemented: > > > - limit is read from a property > > > - limit is documented > > > - stay quiet (i.e., not StatusLogger exception logging) if the limit > has > > > been exceeded > > > > > > My rationale for the "stay quiet" feature is that, if the runtime > > exhibits > > > a behavior not anticipated by the configuration, rather than nuking the > > > StatusLogger, simply practice the configuration: recurse no more. > > > > > > This said, I am struggling to not get drawn into Carter's following > > > remark: *"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"*. > > > > > > Note that I am not strongly opinionated about the feature per se. I > just > > > want to bring the discussion to a conclusion. If we decide to reject > the > > PR > > > (preferably, with a good argument), that is fine by me too. > > > > >
