[ubuntu-hardened] [PATCH] policycoreutils: preserve mode bits and ownership of /tmp in seunshare

[dave w]

On Thu, Sep 15, 2011 at 4:07 PM, Guido Trentalancia
<guido at trentalancia.com> wrote:
> Hello Dave.
>
> On Thu, 2011-09-15 at 13:39 -0400, dave w wrote:
>> Hi,
>>
>> This patch addresses a flaw in seunshare.c that allows unprivileged
>> users to arbitrarily modify the contents of /tmp.  This bug is further
>> described in CVE 2011-1011
>> (http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-1011):
>
> seunshare should not be installed by default and, even if it still
> needed to be installed by default, its setuid bit should be carefully
> re-evaluated in my opinion.
>

Perhaps, but distros that install seunshare at present will be made
safer with the addition of a patch which eliminates an attack vector
to a privilege escalation.

> In any case, good practice says nothing should ever be allowed to mount
> under /tmp with suid/exec flags (use noexec,nosuid options in fstab).
>
> That said, have you tested the patch already ? Is it effective ?
>

Yes, the patch has been effective and with it applied, unprivileged
users cannot delete files other than their own from /tmp, which is the
expected behavior in a directory with the sticky bit set owned by the
superuser.

> Thanks.
>
> Guido
>
>> The seunshare_mount function in sandbox/seunshare.c in seunshare in certain
>> Red Hat packages of policycoreutils 2.0.83 and earlier in Red Hat
>> Enterprise Linux (RHEL) 6 and earlier, and Fedora 14 and earlier, mounts a
>> new directory on top of /tmp without assigning root ownership and the
>> sticky bit to this new directory, which allows local users to replace or
>> delete arbitrary /tmp files, and consequently cause a denial of service or
>> possibly gain privileges, by running a setuid application that relies on
>> /tmp, as demonstrated by the ksu application
>>
>> This patch preserves the mode bits, and thus permissions, and
>> ownership of the destination directory of the bind mount performed by
>> seunshare.  The permission check in verify_mount() was relaxed for
>> directories who originally had the sticky bit set, as root ownership
>> is required for these to ensure that unprivileged users cannot unlink
>> arbitrary files in the newly bind mounted directory.
>>
>> Thanks,
>> David
>>
>>
>>
>>  policycoreutils/sandbox/seunshare.c |   23 ++++++++++++++++++++++-
>>  1 files changed, 22 insertions(+), 1 deletions(-)
>>
>> diff --git a/policycoreutils/sandbox/seunshare.c
>> b/policycoreutils/sandbox/seunshare.c
>> index f9bf12c..82b3cb9 100644
>> --- a/policycoreutils/sandbox/seunshare.c
>> +++ b/policycoreutils/sandbox/seunshare.c
>> @@ -149,7 +149,9 @@ static int verify_mount(const char *mntdir, struct
>> passwd *pwd) {
>>         fprintf(stderr, _("Invalid mount point %s: %s\n"), mntdir,
>> strerror(errno));
>>         return -1;
>>     }
>> -   if (sb.st_uid != pwd->pw_uid) {
>> +
>> +    /* Owners don't have to match if the sticky bit has been set. */
>> +   if (sb.st_uid != pwd->pw_uid && !(sb.st_mode && S_ISVTX)) {
>>         errno = EPERM;
>>         syslog(LOG_AUTHPRIV | LOG_ALERT, "%s attempted to mount an
>> invalid directory, %s", pwd->pw_name, mntdir);
>>         perror(_("Invalid mount point, reporting to administrator"));
>> @@ -245,8 +247,17 @@ static int verify_shell(const char *shell_name)
>>  }
>>
>>  static int seunshare_mount(const char *src, const char *dst, struct
>> passwd *pwd) {
>> +    struct stat buf;
>> +
>>     if (verbose)
>>         printf("Mount %s on %s\n", src, dst);
>> +
>> +    /* Preserve mode bits and ownership */
>> +    if (stat(dst, &buf) < 0) {
>> +        fprintf(stderr, _("Failed to stat %s: %s\n"), dst, strerror(errno));
>> +        return -1;
>> +    }
>> +
>>     if (mount(dst, dst,  NULL, MS_BIND | MS_REC, NULL) < 0) {
>>         fprintf(stderr, _("Failed to mount %s on %s: %s\n"), dst, dst,
>> strerror(errno));
>>         return -1;
>> @@ -262,6 +273,16 @@ static int seunshare_mount(const char *src, const
>> char *dst, struct passwd *pwd)
>>         return -1;
>>     }
>>
>> +    /* Restore original mode bits and ownership */
>> +    if (chmod(dst, buf.st_mode) < 0) {
>> +        fprintf(stderr, _("Failed to set permissions on %s: %s\n"),
>> dst, strerror(errno));
>> +        return -1;
>> +    }
>> +    if (chown(dst, buf.st_uid, buf.st_gid) < 0) {
>> +        fprintf(stderr, _("Failed to set ownership on %s: %s\n"),
>> dst, strerror(errno));
>> +        return -1;
>> +    }
>> +
>>     if (verify_mount(dst, pwd) < 0)
>>         return -1;
>>  }
>
>
>