| Message ID | 1508506702-17704-7-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 20, 2017 at 02:38:21PM +0100, Ian Jackson wrote: > +static bool os_parse_runas_uid_gid(const char *optarg) > +{ > + unsigned long lv; > + char *ep; > + uid_t got_uid; > + gid_t got_gid; > + int rc; > + > + errno = 0; > + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */ Should strtoul base be 10? If that matter. > + got_uid = lv; /* overflow here is ID in C99 */ > + if (errno || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) { > + return false; > + } > + > + lv = 0; > + rc = qemu_strtoul(ep + 1, 0, 0, &lv); > + got_gid = lv; /* overflow here is ID in C99 */ > + if (rc || got_gid != lv || got_gid == (gid_t)-1) { > + return false; > + } > + > + user_uid = got_uid; > + user_gid = got_gid; > + return true; > +} > + > /* > * Parse OS specific command line options. > * return 0 if option handled, -1 otherwise > @@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg) > #endif > case QEMU_OPTION_runas: > user_pwd = getpwnam(optarg); > - if (!user_pwd) { > - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg); > + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) { > + fprintf(stderr, > + "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n", The error message have not been update, I think it should be <uid>:<gid> > + optarg); > exit(1); > } > break; With the error message fix: Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Anthony PERARD writes ("Re: [PATCH v5.1 7/8] os-posix: Provide new -runas <uid>:<gid> facility"):
> On Fri, Oct 20, 2017 at 02:38:21PM +0100, Ian Jackson wrote:
> > +static bool os_parse_runas_uid_gid(const char *optarg)
...
> > + errno = 0;
> > + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */
>
> Should strtoul base be 10? If that matter.
If someone wants to write uids in hex then I don't see a reason to
stop them...
> > - if (!user_pwd) {
> > - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> > + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> > + fprintf(stderr,
> > + "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n",
>
> The error message have not been update, I think it should be <uid>:<gid>
Oops.
> With the error message fix:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
Ian.
CC'ing the maintainers (scripts/get_maintainer.pl is your friend) On Fri, 20 Oct 2017, Ian Jackson wrote: > This allows the caller to specify a uid and gid to use, even if there > is no corresponding password entry. This will be useful in certain > Xen configurations. > > We don't support just -runas <uid> because: (i) deprivileging without > calling setgroups would be ineffective (ii) given only a uid we don't > know what gid we ought to use (since uids may eppear in multiple > passwd file entries with different gids). > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > --- > v5: Use : rather than . to separate uid from gid > v4: Changed to reuse option -runas > v3: Error messages fixed. Thanks to Peter Maydell and Ross Lagerwall. > v2: Coding style fixes. > > squash! os-posix: Provide new -runas <uid>.<gid> facility > --- > os-posix.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++---------- > qemu-options.hx | 3 ++- > 2 files changed, 55 insertions(+), 12 deletions(-) > > diff --git a/os-posix.c b/os-posix.c > index 92e9d85..f95b7bf 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -43,6 +43,8 @@ > #endif > > static struct passwd *user_pwd; > +static uid_t user_uid = (uid_t)-1; > +static gid_t user_gid = (gid_t)-1; > static const char *chroot_dir; > static int daemonize; > static int daemon_pipe; > @@ -128,6 +130,34 @@ void os_set_proc_name(const char *s) > #endif > } > > + > +static bool os_parse_runas_uid_gid(const char *optarg) > +{ > + unsigned long lv; > + char *ep; > + uid_t got_uid; > + gid_t got_gid; > + int rc; > + > + errno = 0; > + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */ > + got_uid = lv; /* overflow here is ID in C99 */ > + if (errno || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) { > + return false; > + } > + > + lv = 0; > + rc = qemu_strtoul(ep + 1, 0, 0, &lv); > + got_gid = lv; /* overflow here is ID in C99 */ > + if (rc || got_gid != lv || got_gid == (gid_t)-1) { > + return false; > + } > + > + user_uid = got_uid; > + user_gid = got_gid; > + return true; > +} > + > /* > * Parse OS specific command line options. > * return 0 if option handled, -1 otherwise > @@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg) > #endif > case QEMU_OPTION_runas: > user_pwd = getpwnam(optarg); > - if (!user_pwd) { > - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg); > + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) { > + fprintf(stderr, > + "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n", > + optarg); > exit(1); > } > break; > @@ -166,18 +198,28 @@ void os_parse_cmd_args(int index, const char *optarg) > > static void change_process_uid(void) > { > - if (user_pwd) { > - if (setgid(user_pwd->pw_gid) < 0) { > - fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); > + if (user_pwd || user_uid != (uid_t)-1) { > + gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid; > + uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid; > + if (setgid(intended_gid) < 0) { > + fprintf(stderr, "Failed to setgid(%d)\n", intended_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 (user_pwd) { > + 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); > + } > + } else { > + if (setgroups(1, &user_gid) < 0) { > + fprintf(stderr, "Failed to setgroups(1, [%d])", > + user_gid); > + exit(1); > + } > } > - if (setuid(user_pwd->pw_uid) < 0) { > - fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid); > + if (setuid(intended_uid) < 0) { > + fprintf(stderr, "Failed to setuid(%d)\n", intended_uid); > exit(1); > } > if (setuid(0) != -1) { > diff --git a/qemu-options.hx b/qemu-options.hx > index 9f6e2ad..f707197e 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3958,7 +3958,8 @@ ETEXI > > #ifndef _WIN32 > DEF("runas", HAS_ARG, QEMU_OPTION_runas, \ > - "-runas user change to user id user just before starting the VM\n", > + "-runas user change to user id user just before starting the VM\n" \ > + " user can be numeric uid:gid instead\n", > QEMU_ARCH_ALL) > #endif > STEXI > -- > 2.1.4 >
Stefano Stabellini writes ("Re: [PATCH v5.1 7/8] os-posix: Provide new -runas <uid>:<gid> facility"):
> CC'ing the maintainers (scripts/get_maintainer.pl is your friend)
I don't know what your scripts/get_maintainer.pl does, but mine
says:
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches! Use common sense.
Anthony PERARD <anthony.perard@citrix.com> (commit_signer:1/2=50%)
Paolo Bonzini <pbonzini@redhat.com> (commit_signer:1/2=50%,commit_signer:11/57=19%)
Ian Jackson <Ian.Jackson@eu.citrix.com> (commit_signer:1/2=50%)
Michael Tokarev <mjt@tls.msk.ru> (commit_signer:12/57=21%)
Eric Blake <eblake@redhat.com> (commit_signer:10/57=18%)
Thomas Huth <thuth@redhat.com> (commit_signer:8/57=14%)
Markus Armbruster <armbru@redhat.com> (commit_signer:8/57=14%)
qemu-devel@nongnu.org (open list:POSIX)
I have added Paolo, Markus and Daniel Berrange to the CCs of my patch
on the basis that they have commented already...
Thanks,
Ian.
diff --git a/os-posix.c b/os-posix.c index 92e9d85..f95b7bf 100644 --- a/os-posix.c +++ b/os-posix.c @@ -43,6 +43,8 @@ #endif static struct passwd *user_pwd; +static uid_t user_uid = (uid_t)-1; +static gid_t user_gid = (gid_t)-1; static const char *chroot_dir; static int daemonize; static int daemon_pipe; @@ -128,6 +130,34 @@ void os_set_proc_name(const char *s) #endif } + +static bool os_parse_runas_uid_gid(const char *optarg) +{ + unsigned long lv; + char *ep; + uid_t got_uid; + gid_t got_gid; + int rc; + + errno = 0; + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */ + got_uid = lv; /* overflow here is ID in C99 */ + if (errno || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) { + return false; + } + + lv = 0; + rc = qemu_strtoul(ep + 1, 0, 0, &lv); + got_gid = lv; /* overflow here is ID in C99 */ + if (rc || got_gid != lv || got_gid == (gid_t)-1) { + return false; + } + + user_uid = got_uid; + user_gid = got_gid; + return true; +} + /* * Parse OS specific command line options. * return 0 if option handled, -1 otherwise @@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg) #endif case QEMU_OPTION_runas: user_pwd = getpwnam(optarg); - if (!user_pwd) { - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg); + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) { + fprintf(stderr, + "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n", + optarg); exit(1); } break; @@ -166,18 +198,28 @@ void os_parse_cmd_args(int index, const char *optarg) static void change_process_uid(void) { - if (user_pwd) { - if (setgid(user_pwd->pw_gid) < 0) { - fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); + if (user_pwd || user_uid != (uid_t)-1) { + gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid; + uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid; + if (setgid(intended_gid) < 0) { + fprintf(stderr, "Failed to setgid(%d)\n", intended_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 (user_pwd) { + 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); + } + } else { + if (setgroups(1, &user_gid) < 0) { + fprintf(stderr, "Failed to setgroups(1, [%d])", + user_gid); + exit(1); + } } - if (setuid(user_pwd->pw_uid) < 0) { - fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid); + if (setuid(intended_uid) < 0) { + fprintf(stderr, "Failed to setuid(%d)\n", intended_uid); exit(1); } if (setuid(0) != -1) { diff --git a/qemu-options.hx b/qemu-options.hx index 9f6e2ad..f707197e 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3958,7 +3958,8 @@ ETEXI #ifndef _WIN32 DEF("runas", HAS_ARG, QEMU_OPTION_runas, \ - "-runas user change to user id user just before starting the VM\n", + "-runas user change to user id user just before starting the VM\n" \ + " user can be numeric uid:gid instead\n", QEMU_ARCH_ALL) #endif STEXI
This allows the caller to specify a uid and gid to use, even if there is no corresponding password entry. This will be useful in certain Xen configurations. We don't support just -runas <uid> because: (i) deprivileging without calling setgroups would be ineffective (ii) given only a uid we don't know what gid we ought to use (since uids may eppear in multiple passwd file entries with different gids). Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> --- v5: Use : rather than . to separate uid from gid v4: Changed to reuse option -runas v3: Error messages fixed. Thanks to Peter Maydell and Ross Lagerwall. v2: Coding style fixes. squash! os-posix: Provide new -runas <uid>.<gid> facility --- os-posix.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++---------- qemu-options.hx | 3 ++- 2 files changed, 55 insertions(+), 12 deletions(-)