SpreadFirefox Security Breached (again)

Kurt writes "The hugely popular SpreadFirefox project, a Firefox community marketing site, has recently fallen victim to a security breach in their TWiki software. This breach has forced the site to shutdown until October 19th. During this time, they will be performing a rebuild of the SpreadFirefox system, to hopefully curb more security breaches."

Comments

Re: That robustness patch of yours

[fizbin]

I like what you've done by way of replacing backticks/qx with something that calls the multi-arg form of system. However, I do take issue with this statement:

SpreadSheetPlugin uses regular expressions to ensure that strings which are passed to eval are harmless. While I could not discover an exploit, this approach should be considered poor engineering from a security standpoint.

While it is certainly easy to use regular expressions in this manner to produce code that qualifies as poor engineering from a security standpoint, the regular expressions that SpreadSheetPlugin uses are actually simple enough to be easily verifiable, or would be if they reduced their excessive use of backslashes down to something readable.

For instance, I would rewrite the first half of their safeEvalPerl subroutine as:

sub safeEvalPerl
{
    my( $theText ) = @_;
    # Allow only simple math with operators - + * / % ( )
    # (shh... don't tell anyone, we support comparison operators)
    $theText =~ m{^(!<=>-+*/0-9.()%\s]*)$}
        or return "ERROR: unsafe eval attempted";
    $theText = $1; # untainted variable
    if ($theText =~ m{(^|[^0-9.\)\s])\s*[%*]}) {
        # catch attempted hash/glob access
        return "ERROR: unsafe eval attempted";
    }
    return "" unless( $theText );
    local $SIG{__DIE__} = sub { TWiki::Func::writeDebug($_[0]); warn $_[0] };
    my $result = eval $theText;
    # at this point, put in the result of their code here

I will admit that the excessive use of eval elsewhere in that module (why are they using the string form of eval, and not the block form?) gives me the security heebie-jeebies. Every spot I found was good, but I had to check too closely.

In your "For Developers" section I would add these suggestions:

• When you discuss Perl's open function, tell everyone to use the three-argument form when possible. This has been available since at least perl 5.6 and eliminates a whole host of potential security problems.
  
• If you are using eval only to trap die(), and not to do the kind of thing that requires extensive security checking, then use the block form of eval, not the string form. (e.g. the only two places SpreadSheetPlugin should be using the string form of eval are inside safeEvalPerl and when they need to use tr///)
  
• Don't attempt to defang malicious input. If input is not demonstrably benign, reject it outright. (I mean, escape or quotemeta() stuff what needs escaping, but if you find yourself removing "unsafe" characters or replacing them with spaces, that's a good sign that you should be rejecting the input instead)