Item12271: Conditional dependency (ONLYIF option) is too restrictive to be all that useful.

`ONLYIF is highly restricted because it gets used in an eval. As currently coded, it can only be used to compare the Foswiki Plugins API version.

This needs to be opened up just a bit more. It needs to support:

• Plugin API, a simple decimal comparison as it currently supports
• New module version strings "v1.2.3" style.

The "sanitized" comparison eliminates quotes, so string comparisons are not usable.

The current code:

$trigger =~ /^[a-zA-Z:\s<>0-9.()\$]*$/ 

So it's limited to a module name, a less/greater condition, and a simple dotted decimal number.

-- GeorgeClark

If this means ONLYIF in DEPENDENCIES, we need more than that. I've seen (and coded) things like:

ONLYIF ($^O eq 'linux')
Linux::Inotify2,>= 1.22,cpan,Optional,Makes file monitoring more efficient and more timely.

Just who is this code defending against by "sanitizing" this string? I don't see the point in restricting it at all, beyond trapping and reporting syntax errors detected by the eval.

Sure, it's possible to say ONLYIF( `rm -rf /` ) - but DEPENDENCIES files are provided by people who are also providing perl code, so it ought to be able to trust me (I mean, "them"....) There are much easier ways of being malicious when delivering an extension...

And it does need to be able to express the sample dependency in my example.

-- TimotheLitt - 11 Dec 2012

ONLYIF was made restrictive tbecause it is evaled by the extension installer. Of course, if an extension is compromised to the degree that ONLYIF is damaged, then there are other much more evil things the extension could be hacked to do. So you are right, sanitising ONLYIF is a pointless bit of paranoia on my part.

-- CrawfordCurrie - 11 Dec 2012

I thought about a Safe box - but then ran out of ideas on what the restrictions should be. It might be useful to ONLYIF(`complex_environ_script.pl`), or, as I do in a checker,

ONLYIF({no warnings 'exec'; my $sts = system('selinuxenabled'); return !($sts == -1 ||($sts>>8) && !($sts & 127)));})

To spare reading the code, this says install if SELinux is present and enabled...

So I've removed the sanitization completely. Better to have sharp tools than dull fingers.

I did add a note that I'll repeat here: DEPENDENCIES files are as safe (or dangerous) as .pm files, and should have permissions to match. (Even before this change, they could trigger installing 100s of - even malicious - modules.)

I also error-checked the eval -- there are far too many blind evals in Foswki...

-- TimotheLitt - 13 Dec 2012

Adding a unit test in ConfigureTests for some of these new conditions, I've added following DEPENDENCIES.

<<<< DEPENDENCIES >>>>
Foswiki::Plugins::TriggerFoswikiAPI,>=0.1,( $Foswiki::Plugins::VERSION < 3.2 ),perl,Required
Foswiki::Plugins::TriggerOSLinux,>=0.1,( $^O eq 'linux' ),perl,Required
Foswiki::Plugins::TriggerGoodFoswiki,>=0.1,( $Foswiki::VERSION < '1.1.1' ),perl,Required
Foswiki::Plugins::TriggerOldFoswiki,>=0.1,( $Foswiki::VERSION < '3.2.1' ),perl,Required
Foswiki::Plugins::TriggerSyntaxError,>=0.1, {no warnings 'exec'; my $sts = system('selinuxenabled'); return !($sts == -1 ||($sts>>8) && !($sts & 127)));} ,perl,Required
Foswiki::Plugins::TriggerSELinux,>=0.1, {no warnings 'exec'; my $sts = system('selinuxenabled'); return !($sts == -1 ||($sts>>8) && !($sts & 127));} ,perl,Required

-- GeorgeClark - 14 Dec 2012

Commenting is disabled while not logged in