You are here: Foswiki>Tasks Web>Item12271 (01 Feb 2013, GeorgeClark)Edit Attach

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

pencil
Priority: Normal
Current State: Closed
Released In: 1.1.7
Target Release: patch
Applies To: Engine
Component: Configure
Branches: Release01x01 trunk
Reported By: GeorgeClark
Waiting For:
Last Change By: GeorgeClark
`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
 
Topic revision: r16 - 01 Feb 2013, GeorgeClark - This page was cached on 28 Jul 2016 - 10:43.

The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License