LOG4J2-3259 Limit max recursion depth when interpolating strings

[Volkan Yazıcı <vo...@yazi.ci>]

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.

Re: LOG4J2-3259 Limit max recursion depth when interpolating strings

[Stig Rohde Døssing <st...@gmail.com>]

 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
<pa...@cerberusftp.com.invalid>:

> 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 <ra...@dslextreme.com>
> 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ı <vo...@yazi.ci> 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.
> >
> >
>

Re: LOG4J2-3259 Limit max recursion depth when interpolating strings

[Patrick Mills <pa...@cerberusftp.com.INVALID>]

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 <ra...@dslextreme.com>
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ı <vo...@yazi.ci> 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.
>
>

Re: LOG4J2-3259 Limit max recursion depth when interpolating strings

[Tim Perry <ti...@gmail.com>]

I think Ralph is right: there should (could) be a limit on how often the
user is informed but the user does need to be informed.

On Mon, Jan 3, 2022 at 8:32 AM Ralph Goers <ra...@dslextreme.com>
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ı <vo...@yazi.ci> 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.
>
>

Re: LOG4J2-3259 Limit max recursion depth when interpolating strings

[Ralph Goers <ra...@dslextreme.com>]

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ı <vo...@yazi.ci> 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.

Re: LOG4J2-3259 Limit max recursion depth when interpolating strings

[Gary Gregory <ga...@gmail.com>]

I was thinking about this the other day in the following terms: How would a
user or dev know that something went wrong?

Let's say I load a config and I see the logging is incorrect in some way.
Under the covers, it's because recursion hit a limit. So how can a user or
dev know they need to enable recursion or raise its limit?

At the lowest level, when replace() is called and the recurse limit is hit,
what would be the best outcome? I claim, from worst to best:
- A String that is partially replaced where replacement stopped when the
recursion limit was hit.
- The original String, no replacement took place.
- null, indicating something is clearly broken.

Aside from the above, I do not see how we can help user and devs _without_
logging to the status logger.

I am thinking about this from a support perspective.

WDYT?

Gary (AFK for the rest of the work day)

On Mon, Jan 3, 2022 at 7:41 AM Volkan Yazıcı <vo...@yazi.ci> 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.
>