Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 15 Mar 2011 09:13:00 -0400
From: Dan Rosenberg <dan.j.rosenberg@...il.com>
To: oss-security@...ts.openwall.com
Cc: Ludwig Nussel <ludwig.nussel@...e.de>, Petr Baudis <pasky@...e.cz>
Subject: Re: Suid mount helpers fail to anticipate RLIMIT_FSIZE

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@...il.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@...il.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@...e.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)
>>>
>>
>

Powered by blists - more mailing lists

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ