Class::CGI temporary showstopper

Ovid on 2006-04-23T18:27:28

You know, I was all proud of how Class::CGI was nice and easy to use and made untainting a breeze (due to its "once and only once" philosophy), but curiosly, I had never run any tests under taint mode. Today I just did. It blew up, badly. The following snippet illustrates the problem:

#!/usr/bin/perl -T

use Module::Load::Conditional qw(check_install);
check_install( module => 'Data::Dumper' );

Which blows up with:

Insecure dependency in eval while running with -T switch at /usr/local/lib/perl5/site_perl/5.8.7/Module/Load/Conditional.pm line 215, <GEN0> line 12.

Seems Module::Load::Conditional reads lines in from your modules and evals "VERSION" lines to get the version. This is a showstopper for Class::CGI. Working on a bug report and a fix now. All of the code appears to assume that VERSION information is all on one line, so while that is not always correct, if I use that as my baseline, I can at least hope that I won't make things worse.

Note that the same security hole appears to be present in Module::InstalledVersion and ExtUtils::MM_Unix.


I'm sure I mentioned this before...

Alias on 2006-04-24T02:04:28

Dude, EVERYTHING does it that way, because that, SOMEHOW, became the required way to do things.

I mentioned at OSCON there was an obviously DOS attack against PAUSE right?

You just found it.

Re:I'm sure I mentioned this before...

Ovid on 2006-04-24T03:13:14

I remember your comments and I remember you wouldn't elaborate, which seemed a reasonable thing to do. The fact that it's still not been fixed is discouraging. Right now, I have some code which finds -- without eval -- the correct version number of about 98% of what I have installed (440 modules), but some of the edge cases it misses are pretty important ones.

What's worse is that while some failures are due to me learning the rather convoluted evolution of version numbers, some are due to authors being lazy and doing things like this:

$VERSION = sprintf("%d.%s", map {s/_//g; $_} q$Name: release-0_60-public $ =~ /-(\d+)_([\d_]+)/)

That might look like a "good" lazy to some folks, but as you pointed out, if anyone ever moves to a different source control system, this could be problematic for them. Heck, even if the author assumes that will never happen, they'll still have problems if they lose their repository (hard disk crashes with no backup or old backups are not unknown).