Re: Suid mount helpers fail to anticipate RLIMIT_FSIZE

[Dan Rosenberg <dan.j.rosenberg () gmail com>]

I did a survey of some suid helpers I'm aware of.  Here's the existing behavior:

util-linux mount
=============
* Edits /etc/mtab.tmp with custom my_addmntent(), behaves identically
to glibc addmntent() in terms of return code
* Succeeds on partial writes, does not remove temp file on failure
(could result in additional corruption of /etc/mtab through multiple
invocations), does not remove lock file /etc/mtab~ on failure (also an
issue)

fusermount (FUSE)
================
* Does not edit mtab directly, calls into util-linux mount/umount, no
changes needed

mount.cifs (samba)
================
* mount.cifs edits /etc/mtab directly, no cleanup on addmntent() failure
* umount.cifs edits /etc/mtab.tmp but does not check return code of addmntent()

ncpmount (ncpfs)
==============
* ncpmount edits /etc/mtab directly, no cleanup on failure, does not
remove lock file /etc/mtab~ on failure
* ncpumount edits /etc/mtab.tmp but does not check return code of addmntent()

vmware-hgfsmounter (open-vm-tools)
===============================
* edits /etc/mtab directly, no cleanup on failure


Regards,
Dan


On Mon, Mar 14, 2011 at 12:31 PM, Dan Rosenberg
<dan.j.rosenberg () gmail com> wrote:
> I've done some further investigation, and have found one of the
> underlying problems.  addmntent() will return 0 (success) even if the
> write was truncated:
>
>  return (fprintf (stream, "%s %s %s %s %d %d\n",
>                   mntcopy.mnt_fsname,
>                   mntcopy.mnt_dir,
>                   mntcopy.mnt_type,
>                   mntcopy.mnt_opts,
>                   mntcopy.mnt_freq,
>                   mntcopy.mnt_passno)
>          < 0 ? 1 : 0);
>
> Of course, this only matters if the process is catching the SIGXFSZ
> that gets thrown if the resource limit is exceeded, but nearly all
> suid mount helpers block or ignore signals (if they don't, that's an
> additional problem, because the process could be terminated mid-write,
> corrupting /etc/mtab or leaving a stale lockfile, for example).
>
> So, I think the first step is to patch glibc to return success in
> these functions if and only if the *full* contents have been written.
> Then, it will be possible to have proper error handling in these
> helper utilities.  Currently, there's really no way for these programs
> to know whether or not their calls to addmntent() actually succeeded
> besides installing a special signal handler for SIGXFSZ (ugly).
>
> After some further thinking and discussion, I think what needs to be
> done is ensuring that all helpers make mtab edits to a temporary file,
> and have proper error handling that cleans up correctly without
> copying over to the actual /etc/mtab if anything bad happens.
> Currently, some mount helpers edit /etc/mtab directly, and others use
> a temporary file but don't have the proper error handling.
>
> I think this one's going to fall into the hands of package maintainers
> and distros, I don't have time to fix all of these.
>
> -Dan
>
> On Mon, Mar 14, 2011 at 8:32 AM, Dan Rosenberg
> <dan.j.rosenberg () gmail com> wrote:
> > Sigh.  Unfortunately I think this is the truth - I just wish there
> > were an easier way of addressing this besides patching every affected
> > helper individually.  Unless anyone else has any ideas, I'll write up
> > some patches for affected programs later today.
> >
> > -Dan
> >
> > On Mon, Mar 14, 2011 at 8:14 AM, Ludwig Nussel <ludwig.nussel () suse de> wrote:
> > > Dan Rosenberg wrote:
> > > > There are a few possible options   We could patch glibc to try to
> > > > raise the rlimit in addmntent(). [...]
> > >
> > > Citing our glibc maintainer Petr Baudis via Bugzilla:
> > >
> > > | I have been thinking about it and I'm not at all sure the proposed solution
> > > | makes sense. First, this may also concern the obscure interfaces like
> > > | putspent() (not sure if anyone uses these, moreover in security relevant
> > > | contexts). Second, messing with RLIMIT_FSIZE within library routine is just
> > > | evil. The caller may be multi-threaded or just do something else between
> > > | setpwent() and endpwent() too and RLIMIT_FSIZE is just evil. All setuid
> > > | programs must sanitize things like this, on their own terms.
> > >
> > > cu
> > > Ludwig
> > >
> > > --
> > >  (o_   Ludwig Nussel
> > >  //\
> > >  V_/_  http://www.suse.de/
> > > SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg)