oss-security - Re: [Pkg-xfce-devel] Bug#639151: Bug#639151: Bug#639151: Local privilege escalation

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110826171430.GA1883@openwall.com>
Date: Fri, 26 Aug 2011 21:14:30 +0400
From: Solar Designer <solar@...nwall.com>
To: Yves-Alexis Perez <corsac@...ian.org>
Cc: oss-security@...ts.openwall.com, Sebastian Krahmer <krahmer@...e.de>,
	639151@...s.debian.org, Moritz Muehlenhoff <jmm@...ian.org>,
	robert.ancell@...onical.com
Subject: Re: [Pkg-xfce-devel] Bug#639151: Bug#639151: Bug#639151: Local privilege escalation

Hi,

I haven't been watching this discussion closely, but here are some
comments that might be of help:

On Fri, Aug 26, 2011 at 11:07:20AM +0200, Yves-Alexis Perez wrote:
> Would something like:
> 
> diff --git a/src/dmrc.c b/src/dmrc.c
> index bff1da8..9f38faf 100644
> --- a/src/dmrc.c
> +++ b/src/dmrc.c
> @@ -80,11 +80,25 @@ dmrc_save (GKeyFile *dmrc_file, const gchar *username)
>      /* Update the users .dmrc */
>      if (user)
>      {
> +      /* write the file as the user itself */
> +      pid_t pid;
> +      pid = fork();
> +
> +      if (pid == 0)
> +      {
> +        if (setuid (user_get_uid(user)) < 0)
> +        {
> +          g_warning("Error changing uid for %s: %s", username, g_strerror(errno));
> +          _exit(EXIT_FAILURE);
> +        }

You also need to switch gid and groups, and you do not have to fork() if
you only switch euid/egid/groups or fsuid/fsgid/groups.  The latter may
be less portable, but at least on Linux it affects only the current
thread in a multi-threaded process.  Probably this difference is
irrelevant in your case, though.

Here's an example:

http://git.altlinux.org/people/ldv/packages/?p=pam.git;a=commitdiff;h=pam_modutil_priv

A tricky part is what to do when you have partially switched credentials
and one of the syscalls fails (e.g., you've switched the gid but not yet
the uid).  The code referenced above (Linux-PAM commit) tries to restore
the old credentials, but ignores possible failure to do so.  A better
action might be to terminate the current process on failure to restore
old credentials.

>          path = g_build_filename (user_get_home_directory (user), ".dmrc", NULL);
>          g_file_set_contents (path, data, length, NULL);
> -        if (getuid () == 0 && chown (path, user_get_uid (user), user_get_gid (user)) < 0)
> -            g_warning ("Error setting ownership on %s: %s", path, strerror (errno));
>          g_free (path);
> +        _exit(EXIT_SUCCESS);
> +
> +      }
> +      if (pid > 0)
> +        wait(NULL);
>      }

You're lucky that you don't seem to need to pass the result of
g_file_set_contents() back to the parent process.  If you were reading
rather than writing a file, you'd have difficulty using the fork()
approach.  However, in your case fork() may actually be fine (but you do
need to drop gid and groups as well).

I hope this helps.

Alexander

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

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.