| Message ID | 1507133891-26013-8-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/04/2017 05:18 PM, 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. > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> snip > @@ -166,17 +187,19 @@ 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) { > + if (user_pwd || user_uid != (uid_t)-1) { > + if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) { > fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); > exit(1); > } > - if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) { > + if ((user_pwd > + ? initgroups(user_pwd->pw_name, user_pwd->pw_gid) > + : setgroups(1, &user_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) { > + if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) { > fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid); > exit(1); > } This last one should be user_uid, not user_gid.
On 4 October 2017 at 17:18, Ian Jackson <ian.jackson@eu.citrix.com> 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. > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > --- > @@ -166,17 +187,19 @@ 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) { > + if (user_pwd || user_uid != (uid_t)-1) { > + if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) { > fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); If you're changing the gid we pass to setgid() I think you should also change the value we tell the user we tried to use in the error message, or it could be rather confusing. > exit(1); > } > - if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) { > + if ((user_pwd > + ? initgroups(user_pwd->pw_name, user_pwd->pw_gid) > + : setgroups(1, &user_gid)) < 0) { > fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n", > user_pwd->pw_name, user_pwd->pw_gid); ...and here we might claim we failed initgroups() when we actually failed setgroups(). > exit(1); > } > - if (setuid(user_pwd->pw_uid) < 0) { > + if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) { > fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid); This error message should be updated too. thanks -- PMM
Peter Maydell writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 4 October 2017 at 17:18, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> > static void change_process_uid(void)
> > {
> > - if (user_pwd) {
> > - if (setgid(user_pwd->pw_gid) < 0) {
> > + if (user_pwd || user_uid != (uid_t)-1) {
> > + if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
> > fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
>
> If you're changing the gid we pass to setgid() I think you should
> also change the value we tell the user we tried to use in the
> error message, or it could be rather confusing.
Thanks for the review. Sorry about the mess here. I have fixed all
these problems.
Ian.
Ross Lagerwall writes ("Re: [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > - if (setuid(user_pwd->pw_uid) < 0) {
> > + if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
> > fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
> > exit(1);
> > }
>
> This last one should be user_uid, not user_gid.
Thanks, fixed.
Ian.
Ian Jackson <ian.jackson@eu.citrix.com> writes: > 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. > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> [...] > diff --git a/qemu-options.hx b/qemu-options.hx > index 9f6e2ad..34a5329 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root privileges, switching > to the specified user. > ETEXI > > +#ifndef _WIN32 > +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \ > + "-runasid uid.gid change to numeric uid and gid just before starting the VM\n", > + QEMU_ARCH_ALL) > +#endif > +STEXI > +@item -runasid @var{uid}.@var{gid} > +@findex -runasid > +Immediately before starting guest execution, drop root privileges, switching > +to the specified uid and gid. > +ETEXI > + > DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env, > "-prom-env variable=value\n" > " set OpenBIOS nvram variables\n", The last thing the QEMU command line needs is more exotic options. Are you sure we need a new one here? Can we make existing -runas serve? Precedence: Coreutils[*]. Pseudo-code: if argument is a decimal number starting with '+': user ID else if argument is a valid user name: user name else if argument is a valid user ID: user ID else: error [*] https://www.gnu.org/software/coreutils/manual/html_node/Disambiguating-names-and-IDs.html
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> The last thing the QEMU command line needs is more exotic options. Are
> you sure we need a new one here? Can we make existing -runas serve?
> Precedence: Coreutils[*]. Pseudo-code:
>
> if argument is a decimal number starting with '+':
> user ID
> else if argument is a valid user name:
> user name
> else if argument is a valid user ID:
> user ID
> else:
> error
I can do this. So -runas <uid>.<gid> then. I don't think it makes
sense to try to -runas <uid> because: you wouldn't have a username
to pass to initgroups: not calling initgroups would be a bear trap;
and otherwise we wouldn't know what gid to use.
Ian.
On Mon, Oct 09, 2017 at 04:05:10PM +0100, Ian Jackson wrote: > Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > > The last thing the QEMU command line needs is more exotic options. Are > > you sure we need a new one here? Can we make existing -runas serve? > > Precedence: Coreutils[*]. Pseudo-code: > > > > if argument is a decimal number starting with '+': > > user ID > > else if argument is a valid user name: > > user name > > else if argument is a valid user ID: > > user ID > > else: > > error > > I can do this. So -runas <uid>.<gid> then. I don't think it makes > sense to try to -runas <uid> because: you wouldn't have a username > to pass to initgroups: not calling initgroups would be a bear trap; > and otherwise we wouldn't know what gid to use. Just use getpwuid() to get the "struct passwd *", then change_process_uid() doesn't need any changes at all AFAICT. Regards, Daniel
Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Just use getpwuid() to get the "struct passwd *", then change_process_uid()
> doesn't need any changes at all AFAICT.
See my comments in the commit message. There may be multiple passwd
entries referring to a particular uid. I think in this area it would
be better to expect the caller to be explicit, rather than taking
getpwuid's notion of the first one.
Ian.
Ian Jackson <ian.jackson@eu.citrix.com> writes: > Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): >> The last thing the QEMU command line needs is more exotic options. Are >> you sure we need a new one here? Can we make existing -runas serve? >> Precedence: Coreutils[*]. Pseudo-code: >> >> if argument is a decimal number starting with '+': >> user ID >> else if argument is a valid user name: >> user name >> else if argument is a valid user ID: >> user ID >> else: >> error > > I can do this. So -runas <uid>.<gid> then. I don't think it makes > sense to try to -runas <uid> because: you wouldn't have a username > to pass to initgroups: not calling initgroups would be a bear trap; > and otherwise we wouldn't know what gid to use. Actually, a numeric UID without group name or ID could be made to work just fine as long as it maps to a user name. The use case may not be worth the bother, though. Using '.' to separate user and group is suboptimal, because POSIX portable user and group names may contain it: 3.426 User Name A string that is used to identify a user; see also User Database. To be portable across systems conforming to IEEE Std 1003.1-2001, the value is composed of characters from the portable filename character set. The hyphen should not be used as the first character of a portable user name. and 3.276 Portable Filename Character Set The set of characters from which portable filenames are constructed. A B C D E F G H I J K L M N O P Q R S T U V W X Y Z a b c d e f g h i j k l m n o p q r s t u v w x y z 0 1 2 3 4 5 6 7 8 9 . _ - http://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap03.html Coreutils uses ':'. Let's follow its lead.
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Actually, a numeric UID without group name or ID could be made to work
> just fine as long as it maps to a user name. The use case may not be
> worth the bother, though.
In libxl's use case, it wouldn't map to a name.
> Using '.' to separate user and group is suboptimal, because POSIX
> portable user and group names may contain it:
...
> Coreutils uses ':'. Let's follow its lead.
I'm happy to change it to use `:'.
Can you confirm that this command line interface is then OK ?
We would like to stablise the corresponding code in libxl...
Ian.
diff --git a/os-posix.c b/os-posix.c index 92e9d85..d63680b 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; @@ -134,6 +136,9 @@ void os_set_proc_name(const char *s) */ void os_parse_cmd_args(int index, const char *optarg) { + unsigned long lv; + char *ep; + int rc; switch (index) { #ifdef CONFIG_SLIRP case QEMU_OPTION_smb: @@ -150,6 +155,22 @@ void os_parse_cmd_args(int index, const char *optarg) exit(1); } break; + case QEMU_OPTION_runasid: + errno = 0; + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */ + user_uid = lv; /* overflow here is ID in C99 */ + if (errno || *ep != '.' || user_uid != lv || user_uid == (uid_t)-1) { + fprintf(stderr, "Could not obtain uid from \"%s\"", optarg); + exit(1); + } + lv = 0; + rc = qemu_strtoul(ep + 1, 0, 0, &lv); + user_gid = lv; /* overflow here is ID in C99 */ + if (rc || user_gid != lv || user_gid == (gid_t)-1) { + fprintf(stderr, "Could not obtain gid from \"%s\"", optarg); + exit(1); + } + break; case QEMU_OPTION_chroot: chroot_dir = optarg; break; @@ -166,17 +187,19 @@ 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) { + if (user_pwd || user_uid != (uid_t)-1) { + if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) { fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); exit(1); } - if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) { + if ((user_pwd + ? initgroups(user_pwd->pw_name, user_pwd->pw_gid) + : setgroups(1, &user_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) { + if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) { fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid); exit(1); } diff --git a/qemu-options.hx b/qemu-options.hx index 9f6e2ad..34a5329 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root privileges, switching to the specified user. ETEXI +#ifndef _WIN32 +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \ + "-runasid uid.gid change to numeric uid and gid just before starting the VM\n", + QEMU_ARCH_ALL) +#endif +STEXI +@item -runasid @var{uid}.@var{gid} +@findex -runasid +Immediately before starting guest execution, drop root privileges, switching +to the specified uid and gid. +ETEXI + DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env, "-prom-env variable=value\n" " set OpenBIOS nvram variables\n",
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. Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> --- v2: Coding style fixes. --- os-posix.c | 31 +++++++++++++++++++++++++++---- qemu-options.hx | 12 ++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-)