Patchwork os-posix: set groups properly for -runas

login
register
mail settings
Submitter Stefan Hajnoczi
Date July 9, 2011, 9:22 a.m.
Message ID <1310203327-27069-1-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/103963/
State New
Headers show

Comments

Stefan Hajnoczi - July 9, 2011, 9:22 a.m.
Andrew Griffiths reports that -runas does not set supplementary group
IDs.  This means that gid 0 (root) is not dropped when switching to an
unprivileged user.

Add an initgroups(3) call to use the -runas user's /etc/groups
membership to update the supplementary group IDs.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
Note this needs compile testing on various POSIX host platforms.  Tested on
Linux.  Should work on BSD and Solaris.  initgroups(3) is SVr4/BSD but not in
POSIX.

 os-posix.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Stefan Hajnoczi - July 12, 2011, 3:59 p.m.
On Sat, Jul 9, 2011 at 10:22 AM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> Andrew Griffiths reports that -runas does not set supplementary group
> IDs.  This means that gid 0 (root) is not dropped when switching to an
> unprivileged user.
>
> Add an initgroups(3) call to use the -runas user's /etc/groups
> membership to update the supplementary group IDs.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> Note this needs compile testing on various POSIX host platforms.  Tested on
> Linux.  Should work on BSD and Solaris.  initgroups(3) is SVr4/BSD but not in
> POSIX.
>
>  os-posix.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)

Are you happy with this patch?  Bumping because security-related.

Regarding portability, Linux, BSD, Solaris, and Mac OS X all provide
initgroups(3).  I think we're good.

Stefan
Stefan Hajnoczi - July 12, 2011, 6:10 p.m.
* Stefan Hajnoczi (stefanha@linux.vnet.ibm.com) wrote:
> @@ -199,6 +200,11 @@ static void change_process_uid(void)
>              fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
>              exit(1);
>          }
> +        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> +            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
> +                    user_pwd->pw_name, user_pwd->pw_gid);
> +            exit(1);
> +        }

Does initgroups need access to /etc/group?  How does this combine w/
-chroot?

Added bonus...this will fail when the initial user is not privileged
_and_ is the same user as -runas user (probably not what a user intended,
but would've worked before).  Something like:

[doh@laptop qemu]$ qemu -runas doh
Chris Wright - July 12, 2011, 7:18 p.m.
* Chris Wright (chrisw@sous-sol.org) wrote:
> * Stefan Hajnoczi (stefanha@linux.vnet.ibm.com) wrote:
> > @@ -199,6 +200,11 @@ static void change_process_uid(void)
> >              fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> >              exit(1);
> >          }
> > +        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> > +            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
> > +                    user_pwd->pw_name, user_pwd->pw_gid);
> > +            exit(1);
> > +        }
> 
> Does initgroups need access to /etc/group?  How does this combine w/
> -chroot?

Tested this on Linux, and w/out /etc/group it simply fails to add any
supplementary groups (doesn't fail completely, just fails safely).
Appears similar from solaris manpages.

Given that...

Acked-by: Chris Wright <chrisw@sous-sol.org>
Blue Swirl - July 12, 2011, 9:44 p.m.
Thanks, applied.

On Sat, Jul 9, 2011 at 12:22 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> Andrew Griffiths reports that -runas does not set supplementary group
> IDs.  This means that gid 0 (root) is not dropped when switching to an
> unprivileged user.
>
> Add an initgroups(3) call to use the -runas user's /etc/groups
> membership to update the supplementary group IDs.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> Note this needs compile testing on various POSIX host platforms.  Tested on
> Linux.  Should work on BSD and Solaris.  initgroups(3) is SVr4/BSD but not in
> POSIX.
>
>  os-posix.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index 7dfb278..6f8d488 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -31,6 +31,7 @@
>  /*needed for MAP_POPULATE before including qemu-options.h */
>  #include <sys/mman.h>
>  #include <pwd.h>
> +#include <grp.h>
>  #include <libgen.h>
>
>  /* Needed early for CONFIG_BSD etc. */
> @@ -199,6 +200,11 @@ static void change_process_uid(void)
>             fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
>             exit(1);
>         }
> +        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> +            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
> +                    user_pwd->pw_name, user_pwd->pw_gid);
> +            exit(1);
> +        }
>         if (setuid(user_pwd->pw_uid) < 0) {
>             fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
>             exit(1);
> --
> 1.7.5.4
>
>
>

Patch

diff --git a/os-posix.c b/os-posix.c
index 7dfb278..6f8d488 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -31,6 +31,7 @@ 
 /*needed for MAP_POPULATE before including qemu-options.h */
 #include <sys/mman.h>
 #include <pwd.h>
+#include <grp.h>
 #include <libgen.h>
 
 /* Needed early for CONFIG_BSD etc. */
@@ -199,6 +200,11 @@  static void change_process_uid(void)
             fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
             exit(1);
         }
+        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
+                    user_pwd->pw_name, user_pwd->pw_gid);
+            exit(1);
+        }
         if (setuid(user_pwd->pw_uid) < 0) {
             fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
             exit(1);