Bug#286905: perl-modules: File::Path::rmtree makes setuid
Hi. As this Bug is now lying around for more than one month I decided to
look into a fix. It is not a very beautiful one, it is only partially
tested and it only works for systems which can fork, so please look over it
before applying it.
The idea is to fork off a process, change into the directory, make sure the
directory has the same inode which was originally used, and then change
permissions. fchmod would have been much easier.
A diff follows:
-- snip --
--- Path.pm 2005-02-08 13:23:10.0 +0100
+++ Path.pm.new 2005-02-08 17:13:04.0 +0100
@@ -189,7 +189,7 @@
} else {
$root =~ s#/\z##;
}
- (undef, undef, my $rp) = lstat $root or next;
+ (undef, my $ino, my $rp) = lstat $root or next;
$rp &= 0; # don't forget setuid, setgid, sticky bits
if ( -d _ ) {
# notabene: 0700 is for making readable in the first place,
@@ -239,9 +239,25 @@
}
else {
carp "Can't remove directory $root: $!";
- chmod($rp, ($Is_VMS ? VMS::Filespec::fileify($root) : $root))
+ # this avoids a race condition which would occur if someone
+ # replaced $root while we were working on it. I would have
+ # preferred to use fchmod, but this seems not to be
+ # supported in perl. -- AW
+ my($fork)=fork;
+ if ($fork<0) {carp("and cannot fork: $!");}
+ elsif ($fork>0) { # parent
+ waitpid($fork,0);
+ } else { # child
+ chdir($root) or carp("and cannot change to $root: $!");
+ (undef,my($rino))=lstat(".");
+ $rino==$ino or carp(
+ "and someone replaced $root while I was working on it\n");
+ $rino==$ino
+ and chmod($rp,($Is_VMS ? VMS::Filespec::fileify(".") : "."))
or carp("and can't restore permissions to "
. sprintf("0%o",$rp) . "\n");
+ exit(0);
+ }
}
}
else {
-- snip --
I hope this helps.
cu
AW
--
*tueteKlammernUeberVariableAuskipp* Dereferenzier Dich, Du
+Miststueck!
signature.asc
Description: Digital signature
Bug#286905: perl-modules: File::Path::rmtree makes setuid
On Wed, Jan 12, 2005 at 05:02:41PM -0500, Aaron Sherman wrote: >> [p5p:] If anyone had a cleaner (and cross-platform) fix, I'd love to >> hear of it. > >Well, certainly relying on rm (and you assumed a "-v" option which, >AFAIK implies GNU rm specifically) is right out. I'm sure others will >say the same. Sure, it was proposed as a quick hack for the Debian package, where it is safe to assume /bin/rm is from GNU coreutils. If it weren't for the requirement to retain the current API (returning the number of deletions, and verbose output) then a thin wrapper around system 'rm', '-rf', @paths would suffix for POSIX systems. >Quick fix? Reduce the race by making any changes just before and just >after an operation, not in preparation for a whole directory. Now you >still have a problem, but a smaller one. A race is a race, no matter how small the window. --bod -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#286905: perl-modules: File::Path::rmtree makes setuid
On Wed, 2005-01-12 at 07:45, Brendan O'Dea wrote:
> >Example of attack: suppose we know that root uses rmtree to clean up
> >/tmp directories.
[...]
> >Root would have recorded the permissions of /tmp/psz/sh, but would
> >"restore" it to /bin/sh.
I'll discuss this one, below with my reply to Brendan.
As a general response: the solution to this and any number of other
vulnerabilities is a system that presents filesystem multi-stage
transactions. That's not something Perl can rely on, but it's certainly
something Perl should support when it does exist (perhaps Reiser has
already gone there?)
Ok, now back to reality:
> >Following on from the "File::Path::rmtree makes setuid" issue, I notice
> >that rmtree may be tricked into removing arbitrary files.
> >
> >Example of attack: suppose we know that root uses rmtree to clean up
> >/tmp directories. Attacker prepares things:
You cited a general solution, and it's a good one. You have to chdir
into the directory and then readdir, restoring state as you go back up.
You also need to sanity check using Cwd and File::Spec before and after
the chdir to make sure that you didn't get hijacked.
> [p5p:] If anyone had a cleaner (and cross-platform) fix, I'd love to
> hear of it.
Well, certainly relying on rm (and you assumed a "-v" option which,
AFAIK implies GNU rm specifically) is right out. I'm sure others will
say the same.
Also, I don't see the value in changing permissions.
I understand WHY you want to do this, but it's far better to tell the
programmer calling rmtree that they might have to fix permissions before
calling rmtree. Otherwise, rmtree is doing exactly what your permissions
told it to, and you might have done that for a reason. Is it any worse
to have a developer say, "I made that directory read-only to all, but
rmtree nuked it!" vs. "I made that directory read-only to all, and
rmtree didn't nuke it!"? I can make that argument either way with just
as much sincerity and "oh my gods, you can't do this to me!" pleading as
I'm sure you've heard on this in the past... solution: punt.
Quick fix? Reduce the race by making any changes just before and just
after an operation, not in preparation for a whole directory. Now you
still have a problem, but a smaller one.
Further reduce the problem by using device/inode information from stat
on platforms where it is available. You can't make this atomic, but when
you need to do:
make_change();
if (! do_something()) {
revert_change();
}
checking just before revert_change() will reduce the race substantially.
Also, consider writing sanity-checking wrappers that handle all of this,
so that future additions don't have to boiler-plate your security code.
--
â 781-324-3772
â [EMAIL PROTECTED]
â http://www.ajs.com/~ajs
Bug#286905: perl-modules: File::Path::rmtree makes setuid
Brendan O'Dea <[EMAIL PROTECTED]> wrote: > both of these issues obviously stem from the same root cause--a race > between generating a list of files, then manipulating that list. The first issue "also" relies on Path.pm trying to be clever: # notabene: 0777 is for making readable in the first place, # it's also intended to change it to writable in case we have # to recurse in which case we are better than rm -rf for # subtrees with strange permissions > I don't really see that this is fixable outside of rewriting rmtree to > recursively chdir+readdir+unlink. > Given that there are possible pitfalls even with this approach (cf. > CVE-2002-0435) ... That pitfall is known and easily avoided by double-checking inodes. >... I'm considering punting the problem to fileutils, > replacing rmtree entirely with the attached subroutine. > [p5p:] If anyone had a cleaner (and cross-platform) fix, I'd love to > hear of it. I am not sure that all platforms have fileutils: no -v option on rm. (Tru64 doesn't.) Rafael Garcia-Suarez <[EMAIL PROTECTED]> wrote: > How does this relate to the Debian patch 22_fix_file_path > for CAN-2004-0452 ? ... CAN-2004-0452 exploited the "chmod 0777", the fix changed the mode to 0700 (and 0666 to 0600) but did not avoid the race. Cheers, Paul Szabo - [EMAIL PROTECTED] http://www.maths.usyd.edu.au:8000/u/psz/ School of Mathematics and Statistics University of Sydney 2006 Australia -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#286905: perl-modules: File::Path::rmtree makes setuid
How does this relate to the Debian patch 22_fix_file_path for CAN-2004-0452 ? which I'm pasting below. That said, an implementation of rmtree() that uses /bin/rm isn't suitable for inclusion in perl itself. http://security.debian.org/pool/updates/main/p/perl/perl_5.6.1-8.8.diff.gz [Adapted from Chip Turner's 5.8.0 patch] Fix for CAN-2004-0452. Change chmod's to make files writable/executable by the current user only and not by the entire world. chmod's necessary in the first place but at least this makes them less dangerous. If, for some reason the rm process dies halfway through, at worst some files and dirs were revoked from others, not made available. --- lib/File/Path.pm2001-03-21 04:40:22.0 +1100 +++ lib/File/Path.pm2004-12-22 23:46:54.0 +1100 @@ -174,7 +174,7 @@ # it's also intended to change it to writable in case we have # to recurse in which case we are better than rm -rf for # subtrees with strange permissions - chmod(0777, ($Is_VMS ? VMS::Filespec::fileify($root) : $root)) + chmod(0700, ($Is_VMS ? VMS::Filespec::fileify($root) : $root)) or carp "Can't make directory $root read+writeable: $!" unless $safe; @@ -202,7 +202,7 @@ print "skipped $root\n" if $verbose; next; } - chmod 0777, $root + chmod 0700, $root or carp "Can't make directory $root writeable: $!" if $force_writeable; print "rmdir $root\n" if $verbose; @@ -224,7 +224,7 @@ print "skipped $root\n" if $verbose; next; } - chmod 0666, $root + chmod 0600, $root or carp "Can't make file $root writeable: $!" if $force_writeable; print "unlink $root\n" if $verbose; -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
Bug#286905: perl-modules: File::Path::rmtree makes setuid
On Thu, Dec 23, 2004 at 09:10:31AM +1100, Paul Szabo wrote:
>Package: perl-modules
>Version: 5.6.1-8.7
>Severity: critical
>File: /usr/share/perl/5.6.1/File/Path.pm
>Tags: security
>Justification: root security hole
>Example of attack: suppose we know that root uses rmtree to clean up
>/tmp directories. Attacker prepares things:
>
> mkdir -p /tmp/psz/sh
> perl -e 'open F, ">/tmp/psz/sh/$_" foreach (1..1000)'
> chmod 4777 /tmp/psz/sh
>
>While root is busy working on /tmp/psz/sh (and this can be made as slow
>as we like), attacker does:
>
> mv /tmp/psz/sh /tmp/psz/dummy
> ln -s /bin/sh /tmp/psz/sh
>
>Root would have recorded the permissions of /tmp/psz/sh, but would
>"restore" it to /bin/sh.
>Following on from the "File::Path::rmtree makes setuid" issue, I notice
>that rmtree may be tricked into removing arbitrary files.
>
>Example of attack: suppose we know that root uses rmtree to clean up
>/tmp directories. Attacker prepares things:
>
> mkdir /tmp/psz
> perl -e 'open F, ">/tmp/psz/$_" foreach (1..1000)'
> touch /tmp/psz/passwd
>
>While root is busy working on /tmp/psz (and this can be made as slow as
>we like), attacker does:
>
> mv /tmp/psz /tmp/dummy
> ln -s /etc /tmp/psz
>
>Root will then remove /etc/passwd.
Thanks Paul,
both of these issues obviously stem from the same root cause--a race
between generating a list of files, then manipulating that list.
I don't really see that this is fixable outside of rewriting rmtree to
recursively chdir+readdir+unlink.
Given that there are possible pitfalls even with this approach (cf.
CVE-2002-0435) I'm considering punting the problem to fileutils,
replacing rmtree entirely with the attached subroutine.
[p5p:] If anyone had a cleaner (and cross-platform) fix, I'd love to
hear of it.
--bod
sub rmtree
{
my ($p, $verbose) = @_;
$p = [] unless defined $p and length $p;
$p = [ $p ] unless ref $p;
my @paths = grep defined && length, @$p;
unless (@paths)
{
carp "No root path(s) specified\n";
return 0;
}
local *RM;
my $pid = open RM, '-|';
unless (defined $pid)
{
carp "Can't fork ($!)\n";
return 0;
}
unless ($pid)
{
# need to parse output, ensure it's not localised
delete $ENV{$_} for grep /^(LC_|LANG(UAGE)?$)/, keys %ENV;
exec '/bin/rm', '-rvf', @paths
or croak "Can't exec /bin/rm ($!)\n";
}
my $count = 0;
while ()
{
if ($verbose)
{
chomp;
s/'$//;
if (s/^removed directory: `//)
{
print "rmdir $_\n";
}
elsif (s/^removed `//)
{
print "unlink $_\n";
}
$count++;
}
}
close RM;
$count;
}
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
