Bug #20439 for Perl-Critic: Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions

Thu Jul 13 02:41:00 2006 Guest - Ticket created

Subject:

Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions

Ideally, if a file/module has a version, it should be completely static. $VERSION = '1.23'; and so on. Worse than this is using something based on CVS of SVN versions. $VERSION = "somestring" =~ /....(....)/; This ties the code to a single repository, if any author ever tries to check in a copy of your code, all the versions go haywire. Even worse AGAIN are people that use versions that are not taint-safe. For example. use version; $VERSION = qv(1.2.3); Or worse again use Some::Module; $VERSION = $Some::Module::VERSION; These are hideously evil, and cause all sorts of problems. So I propose two policies. Firstly, ProhibitTaintedVersion, which requires the $VERSION declaration line contains a single statement, and that this statement doesn't have an use calls, variables, or anything else that might be untaint-safe. This needs to be at the strongest possible setting, as it creates all sorts of problems. And a second ProhibitDynamicVersion, which requires that versions are simple $VERSION = '...' things, that will always be the same on every installation, and in every repository, for every author. This is bad, but not so urgent... one or two notches back from the maximum would be fine. Adam K

Thu Jul 13 10:57:54 2006 chris [...] chrisdolan.net - Correspondence added

Subject:	Re: [rt.cpan.org #20439] Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions
Date:	Thu, 13 Jul 2006 09:56:11 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Chris Dolan <chris [...] chrisdolan.net>

On Jul 13, 2006, at 1:41 AM, Guest via RT wrote: Show quoted text

> ... > So I propose two policies. > > Firstly, ProhibitTaintedVersion, which requires the $VERSION > declaration > line contains a single statement, and that this statement doesn't > have > an use calls, variables, or anything else that might be untaint-safe. > > This needs to be at the strongest possible setting, as it creates all > sorts of problems. > > And a second ProhibitDynamicVersion, which requires that versions are > simple $VERSION = '...' things, that will always be the same on every > installation, and in every repository, for every author. > > This is bad, but not so urgent... one or two notches back from the > maximum would be fine. > > Adam K

Adam, Thanks. I've added variations of these to the TODO list as "VariablesAndExpressions::RequireConstantVersion" and "VariablesAndExpressions::ProhibitComplexVersion" (although one could argue that they belong in the P::C::P::Modules:: namespace). I agree with your distaste for dynamic $VERSION but I disagree with your recommendations for severity. Our plan is to reserve highest severity for non-controversial policies (i.e. ones that everyone agrees should be enforced). I think your proposed policies are not going to be appreciated by all, so I've recommended lower severities in the TODO list. Chris -- Chris Dolan, Software Developer, http://www.chrisdolan.net/ Public key: http://www.chrisdolan.net/public.key vCard: http://www.chrisdolan.net/ChrisDolan.vcf

Thu Jul 13 10:57:56 2006 The RT System itself - Status changed from 'new' to 'open'

Thu Jul 13 11:12:08 2006 adam [...] phase-n.com - Correspondence added

Subject:	Re: [rt.cpan.org #20439] Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions
Date:	Fri, 14 Jul 2006 01:08:49 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Adam Kennedy <adam [...] phase-n.com>

Chris: There's two seperate issues. I'm willing to let you do whatever you want for severity with the regex/non-strict on, but the case where anything trying to find the version has to execute it as code is extremely dangerous. For starters, it means you can Denial of Service any system that uses the interpreter to find the version. while ( 1 ) { $_++ } our $VERSION = '0.01'; or use Nuclear::Weapon; our $VERSION = fire('1.2.3'); This includes things like PAUSE. Or slightly less bad, the version becomes unstable. There are real world examples on #perl from the last month or so where a module is reporting the wrong version because it loaded some other module that had been updated, but it hadn't. This, obviously produced a large dose of confusion. Realising this, POE is switching from repository/regex to static on the next release (I believe from Rocco) and SVK is moving from dynamic $VERSION = $Other::Module::VERSION to static versions, because they've had similar problems with modules reporting wrongly. It's your software and your call, but for the case where the version line loads in or runs arbitrary code, this is seriously nasty stuff, and a ton of people that try it end up being bitten. Adam K Chris Dolan via RT wrote: Show quoted text

> <URL: http://rt.cpan.org/Ticket/Display.html?id=20439 > > > On Jul 13, 2006, at 1:41 AM, Guest via RT wrote: >

>> ... >> So I propose two policies. >> >> Firstly, ProhibitTaintedVersion, which requires the $VERSION >> declaration >> line contains a single statement, and that this statement doesn't >> have >> an use calls, variables, or anything else that might be untaint-safe. >> >> This needs to be at the strongest possible setting, as it creates all >> sorts of problems. >> >> And a second ProhibitDynamicVersion, which requires that versions are >> simple $VERSION = '...' things, that will always be the same on every >> installation, and in every repository, for every author. >> >> This is bad, but not so urgent... one or two notches back from the >> maximum would be fine. >> >> Adam K

> > Adam, > > Thanks. I've added variations of these to the TODO list as > "VariablesAndExpressions::RequireConstantVersion" and > "VariablesAndExpressions::ProhibitComplexVersion" (although one could > argue that they belong in the P::C::P::Modules:: namespace). I agree > with your distaste for dynamic $VERSION but I disagree with your > recommendations for severity. Our plan is to reserve highest > severity for non-controversial policies (i.e. ones that everyone > agrees should be enforced). I think your proposed policies are not > going to be appreciated by all, so I've recommended lower severities > in the TODO list. > > Chris > > -- > Chris Dolan, Software Developer, http://www.chrisdolan.net/ > Public key: http://www.chrisdolan.net/public.key > vCard: http://www.chrisdolan.net/ChrisDolan.vcf > > >

Thu Jul 13 12:03:47 2006 chris [...] chrisdolan.net - Correspondence added

Subject:	Re: [rt.cpan.org #20439] Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions
Date:	Thu, 13 Jul 2006 11:02:17 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Chris Dolan <chris [...] chrisdolan.net>

Adam, OK, I might be coming around to agree with you on severity. For the record, I totally agree with you on the danger and inappropriateness of non-constant version numbers. I've advocated similarly to what you are saying in the past. What I'm was reacting to is pissing off our user base. We've tried to use $SEVERITY_HIGHEST only for policies that we're sure nobody will want to turn off. My intuition says that the "($VERSION) = q $REVISION: 38$ =~ /(\d+)/" idiom is so ingrained that people will balk at a policy to outlaw that. But maybe I'm wrong? If any other P::C developers have input, it would be welcome. Chris On Jul 13, 2006, at 10:12 AM, adam@phase-n.com via RT wrote: Show quoted text

> > Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=20439 > > > Chris: There's two seperate issues. I'm willing to let you do whatever > you want for severity with the regex/non-strict on, but the case where > anything trying to find the version has to execute it as code is > extremely dangerous. > > For starters, it means you can Denial of Service any system that uses > the interpreter to find the version. > > while ( 1 ) { $_++ } our $VERSION = '0.01'; > > or > > use Nuclear::Weapon; our $VERSION = fire('1.2.3'); > > This includes things like PAUSE. > > Or slightly less bad, the version becomes unstable. > > There are real world examples on #perl from the last month or so > where a > module is reporting the wrong version because it loaded some other > module that had been updated, but it hadn't. > > This, obviously produced a large dose of confusion. > > Realising this, POE is switching from repository/regex to static on > the > next release (I believe from Rocco) and SVK is moving from dynamic > $VERSION = $Other::Module::VERSION to static versions, because they've > had similar problems with modules reporting wrongly. > > It's your software and your call, but for the case where the version > line loads in or runs arbitrary code, this is seriously nasty > stuff, and > a ton of people that try it end up being bitten. > > Adam K > > Chris Dolan via RT wrote:

>> <URL: http://rt.cpan.org/Ticket/Display.html?id=20439 > >> >> On Jul 13, 2006, at 1:41 AM, Guest via RT wrote: >>

>>> ... >>> So I propose two policies. >>> >>> Firstly, ProhibitTaintedVersion, which requires the $VERSION >>> declaration >>> line contains a single statement, and that this statement doesn't >>> have >>> an use calls, variables, or anything else that might be untaint- >>> safe. >>> >>> This needs to be at the strongest possible setting, as it creates >>> all >>> sorts of problems. >>> >>> And a second ProhibitDynamicVersion, which requires that versions >>> are >>> simple $VERSION = '...' things, that will always be the same on >>> every >>> installation, and in every repository, for every author. >>> >>> This is bad, but not so urgent... one or two notches back from the >>> maximum would be fine. >>> >>> Adam K

>> >> Adam, >> >> Thanks. I've added variations of these to the TODO list as >> "VariablesAndExpressions::RequireConstantVersion" and >> "VariablesAndExpressions::ProhibitComplexVersion" (although one could >> argue that they belong in the P::C::P::Modules:: namespace). I agree >> with your distaste for dynamic $VERSION but I disagree with your >> recommendations for severity. Our plan is to reserve highest >> severity for non-controversial policies (i.e. ones that everyone >> agrees should be enforced). I think your proposed policies are not >> going to be appreciated by all, so I've recommended lower severities >> in the TODO list. >> >> Chris >> >> -- >> Chris Dolan, Software Developer, http://www.chrisdolan.net/ >> Public key: http://www.chrisdolan.net/public.key >> vCard: http://www.chrisdolan.net/ChrisDolan.vcf >> >> >>

> >

-- Chris Dolan, Software Developer, http://www.chrisdolan.net/ Public key: http://www.chrisdolan.net/public.key vCard: http://www.chrisdolan.net/ChrisDolan.vcf

Thu Jul 13 12:16:53 2006 adam [...] phase-n.com - Correspondence added

Subject:	Re: [rt.cpan.org #20439] Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions
Date:	Fri, 14 Jul 2006 02:15:17 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Adam Kennedy <adam [...] phase-n.com>

Ah, ok. Sorry. There's two cases here. ($VERSION) = q$REVISION: 38$ =~ /(\d+)/ That is NOT a severity 5. It's possibly a 4, and certainly no less than 3. As you see, it IS taint-safe, and it doesn't call out of the file. It is a concern from a DoS point of view but it's ONLY a concern over time and the security concerns are mostly theoretical (we haven't seen examples of this in the wild, ever). And I agree that current convention in this regard currently means that the user-base isn't going to like it, and so all up it doesn't warrant a 5. use Module; $VERSION = $Module::VERSION. THAT is a severity 5. Anything that has to call out to another file or load in other modules is the really severe problem. It breaks all sorts of things, from the PAUSE indexer to various tools. It's pure evil. The only controvertial one is going to be that this also includes. use version; $VERSION = qv(1.2.3); Calling that a 5 in direct conflict with PBP is tricky politically. I'd be willing to accept that you make a special case of version, and downrate it to 2 or 3. But ANYTHING other than version.pm is absolutely a 5. Adam K Chris Dolan via RT wrote: Show quoted text

> <URL: http://rt.cpan.org/Ticket/Display.html?id=20439 > > > Adam, > > OK, I might be coming around to agree with you on severity. > > For the record, I totally agree with you on the danger and > inappropriateness of non-constant version numbers. I've advocated > similarly to what you are saying in the past. > > What I'm was reacting to is pissing off our user base. We've tried > to use $SEVERITY_HIGHEST only for policies that we're sure nobody > will want to turn off. My intuition says that the "($VERSION) = q > $REVISION: 38$ =~ /(\d+)/" idiom is so ingrained that people will > balk at a policy to outlaw that. But maybe I'm wrong? > > If any other P::C developers have input, it would be welcome. > > Chris > > On Jul 13, 2006, at 10:12 AM, adam@phase-n.com via RT wrote: >

>> Queue: Perl-Critic >> Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=20439 > >> >> Chris: There's two seperate issues. I'm willing to let you do whatever >> you want for severity with the regex/non-strict on, but the case where >> anything trying to find the version has to execute it as code is >> extremely dangerous. >> >> For starters, it means you can Denial of Service any system that uses >> the interpreter to find the version. >> >> while ( 1 ) { $_++ } our $VERSION = '0.01'; >> >> or >> >> use Nuclear::Weapon; our $VERSION = fire('1.2.3'); >> >> This includes things like PAUSE. >> >> Or slightly less bad, the version becomes unstable. >> >> There are real world examples on #perl from the last month or so >> where a >> module is reporting the wrong version because it loaded some other >> module that had been updated, but it hadn't. >> >> This, obviously produced a large dose of confusion. >> >> Realising this, POE is switching from repository/regex to static on >> the >> next release (I believe from Rocco) and SVK is moving from dynamic >> $VERSION = $Other::Module::VERSION to static versions, because they've >> had similar problems with modules reporting wrongly. >> >> It's your software and your call, but for the case where the version >> line loads in or runs arbitrary code, this is seriously nasty >> stuff, and >> a ton of people that try it end up being bitten. >> >> Adam K >> >> Chris Dolan via RT wrote:

>>> <URL: http://rt.cpan.org/Ticket/Display.html?id=20439 > >>> >>> On Jul 13, 2006, at 1:41 AM, Guest via RT wrote: >>>

>>>> ... >>>> So I propose two policies. >>>> >>>> Firstly, ProhibitTaintedVersion, which requires the $VERSION >>>> declaration >>>> line contains a single statement, and that this statement doesn't >>>> have >>>> an use calls, variables, or anything else that might be untaint- >>>> safe. >>>> >>>> This needs to be at the strongest possible setting, as it creates >>>> all >>>> sorts of problems. >>>> >>>> And a second ProhibitDynamicVersion, which requires that versions >>>> are >>>> simple $VERSION = '...' things, that will always be the same on >>>> every >>>> installation, and in every repository, for every author. >>>> >>>> This is bad, but not so urgent... one or two notches back from the >>>> maximum would be fine. >>>> >>>> Adam K

>>> Adam, >>> >>> Thanks. I've added variations of these to the TODO list as >>> "VariablesAndExpressions::RequireConstantVersion" and >>> "VariablesAndExpressions::ProhibitComplexVersion" (although one could >>> argue that they belong in the P::C::P::Modules:: namespace). I agree >>> with your distaste for dynamic $VERSION but I disagree with your >>> recommendations for severity. Our plan is to reserve highest >>> severity for non-controversial policies (i.e. ones that everyone >>> agrees should be enforced). I think your proposed policies are not >>> going to be appreciated by all, so I've recommended lower severities >>> in the TODO list. >>> >>> Chris >>> >>> -- >>> Chris Dolan, Software Developer, http://www.chrisdolan.net/ >>> Public key: http://www.chrisdolan.net/public.key >>> vCard: http://www.chrisdolan.net/ChrisDolan.vcf >>> >>> >>>

> > -- > Chris Dolan, Software Developer, http://www.chrisdolan.net/ > Public key: http://www.chrisdolan.net/public.key > vCard: http://www.chrisdolan.net/ChrisDolan.vcf > > >

Fri Jul 14 16:54:57 2006 thaljef [...] cpan.org - Correspondence added

Maybe we could distribute this separately in a policy bundle that is aimed at CPAN authors. Or since you mentioned taintedness and denial-of-service, it could fit into the Perl-Critic-Security bundle that I've been thinking about. -Jeff

Fri Jul 14 21:11:42 2006 adam [...] phase-n.com - Correspondence added

Subject:	Re: [rt.cpan.org #20439] Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions
Date:	Sat, 15 Jul 2006 11:11:00 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Adam Kennedy <adam [...] phase-n.com>

Sounds appropriate. I'd actually been pondering the ideas of profiles as well, there's going to be a variety of things that are useful for things like security and backcompatibility. This would fit in well to both, and anything for CPAN authors would necesarily contain both. Adam K Jeffrey Thalhammer via RT wrote: Show quoted text

> <URL: http://rt.cpan.org/Ticket/Display.html?id=20439 > > > Maybe we could distribute this separately in a policy bundle that is > aimed at CPAN authors. Or since you mentioned taintedness and > denial-of-service, it could fit into the Perl-Critic-Security bundle > that I've been thinking about. > > -Jeff

Wed Mar 25 22:06:32 2009 wyant [...] cpan.org - Correspondence added

Proposed implementation committed (to core Perl::Critic) as SVN revision 3247. I think it is fairly clear what ValuesAndExpressions::RequireConstantVersion should do, but when ForbidTaintedVersion transmogrified into ForbidComplexVersion things got a little vague. At the moment ForbidComplexVersion implements more or less what Adam K. asked for in ForbidTaintedVersion - but with an explicit check for fully-qualified symbol names, since the wild appears to contain tainted versions that do not include the source modules on the same line. Tom Wyant

Thu Mar 26 11:15:38 2009 EDAVIS [...] cpan.org - Correspondence added

Let's be clear: the denial-of-service vulnerability is in the code that executes the VERSION line. The interesting VERSION lines mentioned are exploits for that vulnerability; they are not the bug itself. (In fact, it's worse than DoS; it is an arbitrary code execution bug. I have written such dodgy code myself, see <http://membled.com/work/perl/pmq/>, mitigated slightly by the fact that it scans only modules you've already installed in your search path.) If all code conformed to a policy 'VERSION should be a simple static expression' then there would be no need to eval() a line of code and the world would be a more pleasant place for indexers and query tools.

Tue Aug 25 23:41:52 2009 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #20439] Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions
Date:	Tue, 25 Aug 2009 22:41:31 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

ValuesAndExpressions::ProhibitComplexVersion and ValuesAndExpressions::RequireConstantVersion were released in v1.104.

Tue Aug 25 23:45:19 2009 ELLIOTJS [...] cpan.org - Status changed from 'open' to 'resolved'

Tue Aug 25 23:45:20 2009 ELLIOTJS [...] cpan.org - Fixed in 1.104 added

Bug #20439 for Perl-Critic: Policy Idea: Prohibiting Dynamic or (worse) Tainted Versions

Preferred bug tracker