Item10647: Preference variable that refers to itself hangs Foswiki

The following code evaluates to an infinite loop

* Set SET = %IF{"'' = '%SET%'" then="UNDEFINED" else="%SET%"}%

-- TemiVarghese - 18 Apr 2011

This has been an issue back to at least 1.0.10. I've been unable to trace down what is happening but it appears that the expansion of the foswikiAlert regarding the depth exceeded also loops. Setting this to urgent. Even though it's been around a long time, the effects are pretty severe.

-- GeorgeClark - 19 Apr 2011

TWiki current svn checkout has the same bug.

-- GeorgeClark - 19 Apr 2011

It seems to be IF that does the looping and not the Set

We need to be careful when fixing.

The IF is today equipped with a loop catcher which checks for the same IF condition looping. Problem here is that each loop through IF has a new condition (the ever growing IF itself) so the loop goes on forever.

When fixing we need to make sure you can still perform a SEARCH with an IF in the format which gets called 200 or 2000 times. That must still work after fixing.

Note that it is only the current topic view that hangs and times out when the Apache script timer expires. You can still open more new topics in another browser window so there is no direct denial of service here. Not more than making a giant search and calling it 2000 times. Eventually the processes dies and all is back to normal without the need of an admin.

What we need is a better catch of a loop that identifies that it is the same IF that gets called too many times.

-- KennethLavrsen - 12 May 2011

Changing status to "Confirmed" as this has already been analysed.

-- CrawfordCurrie - 08 Jun 2011

The problem is fundamental to the way we expand macros. Here's a much simpler testcase:

* Set Z = %Z% %Z%

Expansion of the first %Z% creates %Z% %Z%. The first %Z% in this new statement is itself recursively expanded until we are 16 levels deep. Now the recursion is blocked. We then step up one level (to level 15) and try to expand the rest of the statement - which includes a %Z%, resulting in the same recursion as just described. This will halt eventually - after 16^16 Z's or so.

There's a trade-off here. This is a pretty pathological case, which we could trap, but in so doing we would be forced to disable other cases of correct recovery from recursion, and it would be inefficient too.

As Kenneth says, there is a server timeout that will eventually catch this, but launch enough requests at a server and it could become a DoS.

Marking for feedback to gather other views. Before making any proposal using the words "just" or "simply", please read the code and try it yourself. Here's a simple textcase function to help you:

sub test_ZZZZZZZZZZZZZZZZ {
    my $this = shift;

    Foswiki::Func::setPreferencesValue( "Z", "%Z% %Z%");

    my $m = new Foswiki::Meta($this->{session}, $this->{test_web},
            $this->{test_topic});
    my $text = '%Z%';
    $this->{session}->innerExpandMacros(\$text, $m);
}

-- CrawfordCurrie - 08 Jun 2011

There are plenty of ways to DoS Foswiki if you can modify topic content. I can't think of an elegant solution to this... but I'd hate to slow down normal macro rendering :/

-- PaulHarvey - 08 Jun 2011

Given that this bug has existed for a very, very long time, and a trivial fix hasn't made itself apparent - I've changed target to minor, so it doesn't show up as a blocker for 1.1.4.

-- PaulHarvey - 18 Jun 2011

mmm, ok, I'm not going to think about this very hard, but 'what if'

When a Set X statement is encountered, we enter a context called settingpreference_X, which then changes (ok, slightly toungle in cheek) all the evaluation code to have a much more aggressive recursion test disallowing any expansion of =%X% on the rhs?

or conversely, can someone quickly explain why Set X= recurse recursion would be desireable?

-- SvenDowideit - 20 Aug 2011

We can probably block the most obvious recursion. The danger is to braindeadly put a counter insider e.g IF. It is important that a recursion counter is reset the right way so a SEARCH that contains an IF and gives 200 hits does not generate a false recursion detection. We have to make sure the counter only counts truely recursed conditions.

Conservatively set a counter at say 32000 so the code stops after a few seconds and not when Apache times out so the normal return sets are never hitting the ceiling.

What should be easy to fix is a macro that contains itself and that stops the most obvious attacks. But then you can still attack with two or 3 macros that circle themselves.

-- KennethLavrsen - 22 Aug 2011

I'm not thinking of changing the MACRO code at all. I'm wondering if we would allow the Preference code to have the macro being set to be used in the rhs (in the simple, non-parameterised form) as that can't actually work..

And so the most naive suggestion I have, is that

   * !Set NOT=something%NOT%else

would in the preference code cause the =%!NOT% to be escaped for safety.

whereas

   * !Set NOT=something%NOT{param='asd'}%else

would be left alone, as its a more advanced usage?

-- SvenDowideit - 23 Aug 2011

Nice idea, and it would catch the most obvious case. However it is so easily defeated that I wonder if it's worth bothering with.

• Set A = %Z%
• Set Z = %A% %A%

The definition of Z doesn't involve Z, but this is just as bad a DoS as before.

The only effective "complete" solution would be to block all recursive evaluation of macros. Perhaps this should be possible as a "security option" on a public server.

-- CrawfordCurrie - 29 Sep 2011

i think you should be wrong, but I will have to spend time in the preference and render code to decide how to do it - because yes, my intent was to prevent all recursive evaluation.

I suspect even the trivial case is worth blocking - because non-mallicious users do hit it at times.

-- SvenDowideit - 29 Sep 2011

This has been waiting for Sven for 18 months - clearly it isn't Urgent. Downspeccing to Low.

-- CrawfordCurrie - 22 Mar 2013

No feedback, so I'm dropping this. i really don't think it's worth putting mindshare on.

-- Main.CrawfordCurrie - 20 Jun 2015 - 08:31

Commenting is disabled while not logged in