Re: [SECURITY] Re: PATCH: rm -rf and readdir bug in coreutils-6.7

[Jim Meyering]

Mikulas Patocka <address@hidden> wrote:
> BTW. looking at the code leads me to another observation --- all those
> security checks against 'rm'-vs-'mv' race break apart if the attacker is

Thank you for bringing this up.  However, note that such systems are not
POSIX-compliant, since (as you must well know) POSIX requires unique inode
numbers.  There are several places in the coreutils where this requirement
is assumed, mainly for security and directory-cycle detection, but also
for the relatively new --preserve-root option.  Relaxing it would not be
trivial.  It's just that I'm not (yet?) convinced doing so is desirable.

> able to create directories with colliding inode number (which can happen
> on 64-bit filesystems that can store more than 2^32 files --- for example
> NFS v3, OCFS or my SPADFS filesystem)

Can you do this in practice?
If so, what are the system and resource requirements?
Does one have to create 2^32 files (or something approaching that number)
in order to get the first inode number collision?
If you can, please answer separately for each file system
type that is susceptible.

Even assuming all of this is feasible, I imagine it would be very
expensive (probabilistically) to get an exploitable collision.
Of course, that is no reason not to avoid the vulnerability.

> Imagine this:
> user creates directory /home/user/foo with i_ino colliding with home
> user creates directory /home/user/foo/boo with i_ino colliding with user
> user creates directory /home/user/foo/boo/bla with a lot of content
> user persuades the root to delete /home/user/foo
> while deleting, he moves /home/user/foo/boo/bla to /home/user/bla
> --- rm will then carelessly remove all home directories.

Haven't you omitted (assumed?) another prerequisite for such an exploit:
a 32-bit ino_t type?  Don't most progressive kernels expose a 64-bit ino_t?
...for some definition of "progressive" :-) -- I presume this isn't
an issue with them.

> Utilities like rm, cp or mv shouldn't probably execute open("..") or
> chdir("..") at all. The best implementation would be walk the whole path
> from initial file again

Surely you don't mean to imply that the _best_ implementation involves
traversing a potentially deep hierarchy *twice*!?

> when finished processing directory and keep cache
> of about 3 to 5 open handles to directories up to initial directory, so
> that walking the whole path can be avoided for most common cases (for
> performance reasons).

That is precisely what is now done by tools (du, chmod, chgrp, chown)
that use gnulib's fts.c.

> Another possibility to fix it (without rewriting the whole algorithm)
> would be to compare not only st_ino and st_dev but also st_uid and st_gid
> in macro SAME_INODE --- this way the user cannot arrange directories to
> delete other user's files. Although I'm still not sure it this is 100% safe.

If necessary, this might be a good short-term fix, but it'd come at a
small cost: currently, only st_dev and st_ino are stored on rm's internal
stack.  It could double the footprint of that stack -- not that it's big.
More importantly, changing SAME_INODE to reference members other than
st_dev and st_ino would affect many tools that store only dev/ino members
rather than a full 'struct stat'.