OSSP CVS Repository

ossp - Check-in [5522]
Not logged in
[Honeypot]  [Browse]  [Home]  [Login]  [Reports
[Search]  [Ticket]  [Timeline
  [Patchset]  [Tagging/Branching

Check-in Number: 5522
Date: 2006-Jul-25 15:00:52 (local)
2006-Jul-25 13:00:52 (UTC)
User:rse
Branch:
Comment: Fix "arbitrary shell command execution" security bug caused by missing shell command argument escaping for user supplied arguments.

Submitted by: Brian Caswell <bmc@shmoo.com>, Sourcefire

Tickets:
Inspections:
Files:
ossp-pkg/shiela/ChangeLog      1.58 -> 1.59     4 inserted, 0 deleted
ossp-pkg/shiela/THANKS      1.6 -> 1.7     1 inserted, 0 deleted
ossp-pkg/shiela/shiela.pl      1.71 -> 1.72     47 inserted, 19 deleted

ossp-pkg/shiela/ChangeLog 1.58 -> 1.59

--- ChangeLog    2006/07/20 08:18:01     1.58
+++ ChangeLog    2006/07/25 13:00:52     1.59
@@ -11,6 +11,10 @@
 
   Changes between 1.1.6 and 1.1.7 (03-Oct-2005 to 20-Jul-2006):
 
+   *) Fix "arbitrary shell command execution" security bug caused by
+      missing shell command argument escaping for user supplied arguments.
+      [Brian Caswell <bmc@shmoo.com>, Ralf S. Engelschall] (CVE-2006-3633)
+
    *) Upgraded build environment to GNU shtool 2.0.6 and
       GNU autoconf 2.60
       [Ralf S. Engelschall]


ossp-pkg/shiela/THANKS 1.6 -> 1.7

--- THANKS       2004/06/27 07:44:03     1.6
+++ THANKS       2006/07/25 13:00:52     1.7
@@ -13,6 +13,7 @@
   stuff, bugfixes, hints etc. (in alphabetical order):
 
   o Denis Barbier               <barbier@imacs.polytechnique.fr>
+  o Brian Caswell               <bmc@shmoo.com>
   o Markus Sander               <msander@de.cw.com>
   o Michael Schloh v. Bennewitz <michael.schloh@cw.com>
   o Christoph Schug             <chris@schug.net>


ossp-pkg/shiela/shiela.pl 1.71 -> 1.72

--- shiela.pl    2006/07/20 08:17:11     1.71
+++ shiela.pl    2006/07/25 13:00:52     1.72
@@ -207,7 +207,8 @@
     $RT->{mimeboundary} = $randtag;
 
     #   determine CVS version and capabilities
-    my $v = `$RT->{cvs} --version 2>/dev/null`;
+    my $cmd = sprintf("%s --version 2>/dev/null", &qsa($RT->{cvs}));
+    my $v = `$cmd`;
     $RT->{cvsvers} = '?';
     $RT->{cvsvers} = $1 if ($v =~ m|Concurrent\s+Versions\s+System\s+\(CVS\)\s+([\d.p]+)\s+|s);
     $RT->{cvsrse} = 0;
@@ -580,7 +581,7 @@
     STDERR->flush; # because of fork() behind open2()!
     $cvs->{rfd} = new IO::Handle;
     $cvs->{wfd} = new IO::Handle;
-    $cvs->{pid} = IPC::Open2::open2($cvs->{rfd}, $cvs->{wfd}, "$program -f -Q -n server")
+    $cvs->{pid} = IPC::Open2::open2($cvs->{rfd}, $cvs->{wfd}, sprintf("%s -f -Q -n server", &main::qsa($program)))
         or die "cannot spawn CVS server process `$program server'";
     print STDERR "cvs server: spawned (pid $cvs->{pid})\n" if ($trace);
     bless ($cvs, $class);
@@ -739,8 +740,9 @@
     bless ($sm, $class);
     $sm->{trace} = $trace;
     $sm->{fd} = new IO::Handle;
-    open($sm->{fd}, "|$RT->{sendmail} -oi -oem $toaddr");
-    print "sendmail: spawned \"$RT->{sendmail} -oi -oem $toaddr\"\n" if ($sm->{trace});
+    my $cmd = sprintf("%s -oi -oem %s", &main::qsa($RT->{sendmail}), &main::qsa($toaddr));
+    open($sm->{fd}, "|$cmd");
+    print "sendmail: spawned \"$cmd\"\n" if ($sm->{trace});
     $sm->{header} =
         "From: \"".$RT->{username}."\" <".$RT->{usermail}.">\n" .
         "To: $toaddr\n" .
@@ -843,6 +845,32 @@
     }
 }
 
+#   quote shell argument
+sub qsa {
+    my ($arg) = @_;
+
+    #   remove NUL characters at all because
+    #   - sh:   removes silenty      (strange)
+    #   - bash: removes silenty      (strange)
+    #   - ksh:  complains and aborts (problem)
+    #   - zsh:  keeps as-is          (ok)
+    #   all(!) other characters in the range 0x00-0xff are safe to be
+    #   passed through the shell when single quoted as explicit tests
+    #   with all(!) characters under sh, bash, ksh and zsh showed.
+    $arg =~ s/\x00//sg;
+    
+    #   single quote argument by
+    #   1. escape "single quote" character by
+    #      - temporarily ending single quotation
+    #      - double quoting "single quote" character
+    #      - restarting single quotation
+    #   2. embedding remaining string into single quotes
+    $arg =~ s/'/'"'"'/sg;
+    $arg = "'$arg'";
+    
+    return $arg;
+}   
+
 ##  _________________________________________________________________
 ##
 ##  History database support.
@@ -1248,7 +1276,7 @@
     #   annotate the files with the branch they stay on
     my $cvsstat = '';
     if (not $RT->{useserver}) {
-        my $io = new IO::File "$RT->{cvs} -f -Q -n status ".join(' ', @cvsfiles)."|"
+        my $io = new IO::File sprintf("%s -f -Q -n status %s|", &qsa($RT->{cvs}), join(' ', map { &qsa($_) } @cvsfiles))
             or die "unable to open CVS command pipe for reading";
         $cvsstat .= $_ while (<$io>);
         $io->close;
@@ -1493,7 +1521,7 @@
                     print STDERR "cvs import: Ignoring this operation - don't expect log messages!\n";
                     exit(0);
                 }
-                my $io = new IO::File "$RT->{cvs} -f -Q -n log -r$It '$Is'|"
+                my $io = new IO::File sprintf("%s -f -Q -n log -r%s %s|", &qsa($RT->{cvs}), &qsa($It), &qsa($Is))
                     or die "unable to open CVS command pipe for reading";
                 $rcslog = $_ while (<$io>);
                 $io->close;
@@ -1615,7 +1643,7 @@
         if ($Io eq 'R' and $Iv eq 'NONE') {
             my $rcslog ='';
             if (not $RT->{useserver}) {
-                my $io = new IO::File "$RT->{cvs} -f -Q -n log '$Is'|"
+                my $io = new IO::File sprintf("%s -f -Q -n log %s|", &qsa($RT->{cvs}), &qsa($Is))
                     or die "unable to open CVS command pipe for reading";
                 $rcslog .= $_ while (<$io>);
                 $io->close;
@@ -1638,7 +1666,7 @@
         my $rcslog = '';
         if ($Io eq 'A' or $Io eq 'M' or $Io eq 'R') {
             if (not $RT->{useserver}) {
-                my $io = new IO::File "$RT->{cvs} -f -Q -n log -r$Iv '$Is'|"
+                my $io = new IO::File sprintf("%s -f -Q -n log -r%s %s|", &qsa($RT->{cvs}), &qsa($Iv), &qsa($Is))
                     or die "unable to open CVS command pipe for reading";
                 $rcslog .= $_ while (<$io>);
                 $io->close;
@@ -1709,7 +1737,7 @@
                 or die "unable to open temporary file $RT->{tmpfile}.all for writing";
             my $l = 0;
             if (not $RT->{useserver}) {
-                my $cvs = new IO::File "$RT->{cvs} -f -Q -n update -p -r$Iv '$Is'|"
+                my $cvs = new IO::File sprintf("%s -f -Q -n update -p -r%s %s|", &qsa($RT->{cvs}), &qsa($Iv), &qsa($Is))
                     or die "unable to open CVS command pipe for reading";
                 while (<$cvs>) {
                     $io->print($_);
@@ -1749,9 +1777,9 @@
                     my $io = new IO::File ">$RT->{tmpfile}.null"
                         or die "unable to open temporary file $RT->{tmpfile}.null for writing";
                     $io->close;
-                    system("$RT->{xdelta} delta $RT->{tmpfile}.null " .
-                           "$RT->{tmpfile}.all $RT->{tmpfile}.xdelta >/dev/null 2>&1");
-                    $io = new IO::File "$RT->{uuencode} $RT->{tmpfile}.xdelta $Is.xdelta |"
+                    system(sprintf("%s delta %s.null %s.all %s.xdelta >/dev/null 2>&1",
+                           &qsa($RT->{xdelta}), &qsa($RT->{tmpfile}), &qsa($RT->{tmpfile}), &qsa($RT->{tmpfile})));
+                    $io = new IO::File sprintf("%s %s.xdelta %s.xdelta|", &qsa($RT->{uuencode}), &qsa($RT->{tmpfile}), &qsa($Is))
                         or die "unable to open uuencode command pipe for reading";
                     $cvsdiff .= $_ while (<$io>);
                     $io->close;
@@ -1771,7 +1799,7 @@
                         ("=" x 76) . "\n" .
                         "\$ cvs diff -u -r0 -r$Iv $Is\n";
                     my $diff = '';
-                    my $io = new IO::File "$RT->{diff} -u /dev/null $RT->{tmpfile}.all|"
+                    my $io = new IO::File sprintf("%s -u /dev/null %s.all|", &qsa($RT->{diff}), &qsa($RT->{tmpfile}))
                         or die "unable to open CVS command pipe for reading";
                     $diff .= $_ while (<$io>);
                     $io->close;
@@ -1800,7 +1828,7 @@
                     my $io = new IO::File ">$RT->{tmpfile}.old"
                         or die "unable to open temporary file $RT->{tmpfile}.old for writing";
                     if (not $RT->{useserver}) {
-                        my $cvs = new IO::File "$RT->{cvs} -f -Q -n update -p -r$IV '$Is'|"
+                        my $cvs = new IO::File sprintf("%s -f -Q -n update -p -r%s %s|", &qsa($RT->{cvs}), &qsa($IV), &qsa($Is))
                             or die "unable to open CVS command pipe for reading";
                         $io->print($_) while (<$cvs>);
                         $cvs->close;
@@ -1821,7 +1849,7 @@
                     $io = new IO::File ">$RT->{tmpfile}.new"
                         or die "unable to open temporary file $RT->{tmpfile}.new for writing";
                     if (not $RT->{useserver}) {
-                        my $cvs = new IO::File "$RT->{cvs} -f -Q -n update -p -r$Iv '$Is'|"
+                        my $cvs = new IO::File sprintf("%s -f -Q -n update -p -r%s %s|", &qsa($RT->{cvs}), &qsa($Iv), &qsa($Is))
                             or die "unable to open CVS command pipe for reading";
                         $io->print($_) while (<$cvs>);
                         $cvs->close;
@@ -1848,9 +1876,9 @@
                         "Index: $cvsdir/$Is\n" .
                         ("=" x 76) . "\n";
                     unlink("$RT->{tmpfile}.xdelta");
-                    system("$RT->{xdelta} delta $RT->{tmpfile}.old " .
-                           "$RT->{tmpfile}.new $RT->{tmpfile}.xdelta >/dev/null 2>&1");
-                    $io = new IO::File "$RT->{uuencode} $RT->{tmpfile}.xdelta $Is.xdelta |"
+                    system(sprintf("%s delta %s.old %s.new %s.xdelta >/dev/null 2>&1",
+                        &qsa($RT->{xdelta}), &qsa($RT->{tmpfile}), &qsa($RT->{tmpfile})));
+                    $io = new IO::File sprintf("%s %s.xdelta %s.xdelta|", &qsa($RT->{uuencode}), &qsa($RT->{tmpfile}), &qsa($Is))
                         or die "unable to open uuencode command pipe for reading";
                     $cvsdiff .= $_ while (<$io>);
                     $io->close;
@@ -1867,7 +1895,7 @@
                 #   generate textual change patch script
                 my $d = '';
                 if (not $RT->{useserver}) {
-                    my $io = new IO::File "$RT->{cvs} -f -Q -n diff -u -r$IV -r$Iv '$Is'|"
+                    my $io = new IO::File sprintf("%s -f -Q -n diff -u -r%s -r%s %s|", &qsa($RT->{cvs}), &qsa($IV), &qsa($Iv), &qsa($Is))
                         or die "unable to open CVS command pipe for reading";
                     $d .= $_ while (<$io>);
                     $io->close;

CVSTrac 2.0.1