Re: [Xen-devel] [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas : facility
Ian Jackson writes:
> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide
> new -runas : facility"):
>> Ian Jackson writes:
>> > That would defer the getpwnam from argument parsing to os_setup_post.
>> > I think that's undesriable.
>>
>> No argument. But why can't os_parse_cmd_args() call getpwnam() as it
>> does now, then store user_pwd->pw_uid, ->pw_gid and ->pw_name instead of
>> user_pwd? Store a null name when it parses the argument as UID:GID.
>
> Oh, I see. It seems less obvious to me than what I have done, but I
> can do it like that if you like.
I just wanted to toss out the idea. Please use your judgenment and do
it the way you like better.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas : facility
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new
-runas : facility"):
> Ian Jackson writes:
> > That would defer the getpwnam from argument parsing to os_setup_post.
> > I think that's undesriable.
>
> No argument. But why can't os_parse_cmd_args() call getpwnam() as it
> does now, then store user_pwd->pw_uid, ->pw_gid and ->pw_name instead of
> user_pwd? Store a null name when it parses the argument as UID:GID.
Oh, I see. It seems less obvious to me than what I have done, but I
can do it like that if you like.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas : facility
Ian Jackson writes:
> Thanks for the review. Taking your comments out of order slightly:
>
> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide
> new -runas : facility"):
>> [change_process_uid] is the only user of @user_pwd, @user_uid, @user_gid.
>>
>> Have you considered replacing global @user_pwd by @user_uid, @user_gid
>> and @user_name? --runas with numeric uid and gid would leave @user_name
>> null.
>
> That would defer the getpwnam from argument parsing to os_setup_post.
> I think that's undesriable.
No argument. But why can't os_parse_cmd_args() call getpwnam() as it
does now, then store user_pwd->pw_uid, ->pw_gid and ->pw_name instead of
user_pwd? Store a null name when it parses the argument as UID:GID.
>> Ian Jackson writes:
>> > static struct passwd *user_pwd;
>> > +static uid_t user_uid = (uid_t)-1;
>> > +static gid_t user_gid = (gid_t)-1;
>>
>> As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
>> over @user_uid, @user_gid. Awkward.
>
> My patch has the right behaviour: each -runas completely overrides the
> previous one. -runas that sets user_{uid,gid} always clears user_pwd
> on the way. So user_pwd can only be set if the most recent -runas was
> a name, and then we should honour the name.
>
> This is rather obscure. I think you are right that this is confusing.
> It ought to be clearer.
>
> I will
> - add a comment next to these three variables saying they must
> all be set at the same time
> - explicitly (redundantly) clear user_pwd in os_parse_runas_uid_gid
> - explicitly set user_{uid,gid} to -1 when -runas gets a
> success from getpwnam
> - assert in change_process_uid that the combination is legal
Yes, that's better. But perhaps you like my idea above.
[...]
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas : facility
Thanks for the review. Taking your comments out of order slightly:
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new
-runas : facility"):
> [change_process_uid] is the only user of @user_pwd, @user_uid, @user_gid.
>
> Have you considered replacing global @user_pwd by @user_uid, @user_gid
> and @user_name? --runas with numeric uid and gid would leave @user_name
> null.
That would defer the getpwnam from argument parsing to os_setup_post.
I think that's undesriable.
> Ian Jackson writes:
> > static struct passwd *user_pwd;
> > +static uid_t user_uid = (uid_t)-1;
> > +static gid_t user_gid = (gid_t)-1;
>
> As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
> over @user_uid, @user_gid. Awkward.
My patch has the right behaviour: each -runas completely overrides the
previous one. -runas that sets user_{uid,gid} always clears user_pwd
on the way. So user_pwd can only be set if the most recent -runas was
a name, and then we should honour the name.
This is rather obscure. I think you are right that this is confusing.
It ought to be clearer.
I will
- add a comment next to these three variables saying they must
all be set at the same time
- explicitly (redundantly) clear user_pwd in os_parse_runas_uid_gid
- explicitly set user_{uid,gid} to -1 when -runas gets a
success from getpwnam
- assert in change_process_uid that the combination is legal
> > +errno = 0;
> > +rc = qemu_strtoul(optarg, &ep, 0, &lv);
...
> > +lv = 0;
>
> Either zero lv before both qemu_strtoul() or neither one.
This is a hangover from the previous version which used raw strtoul.
The assignments to both lv and errno are redundant.
> > -if (!user_pwd) {
> > -fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> > +if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> > +error_report("User doesn't exist (and is not :)");
>
> The error message no longer includes the offending value. Intentional?
I was in two minds. I will put it back.
> > +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);
>
> error_report(), please. More of the same below.
I was following the existing code in this function. I'll add a
pre-patch to fix this up here, and maybe a post-patch to fix the rest
of this file.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas : facility
Ian Jackson 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.
>
> We don't support just -runas 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
> Reviewed-by: Anthony PERARD
> CC: Paolo Bonzini
> CC: Markus Armbruster
> CC: Daniel P. Berrange
> ---
> v6.1: Fix constness of qemu_strtoul end pointer parameter.
> v6: Use qemu_strtoul for the first strtoul.
> Use error_report rather than fprintf to print usage error message.
> Fix an error message which still referred to . rather than :
> 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.
>
> Signed-off-by: Ian Jackson
> ---
> os-posix.c | 62
> +++--
> qemu-options.hx | 3 ++-
> 2 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index b9c2343..214f8fb 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -42,6 +42,8 @@
> #endif
>
> static struct passwd *user_pwd;
> +static uid_t user_uid = (uid_t)-1;
> +static gid_t user_gid = (gid_t)-1;
As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
over @user_uid, @user_gid. Awkward.
> static const char *chroot_dir;
> static int daemonize;
> static int daemon_pipe;
> @@ -127,6 +129,34 @@ void os_set_proc_name(const char *s)
> #endif
> }
>
> +
> +static bool os_parse_runas_uid_gid(const char *optarg)
> +{
> +unsigned long lv;
> +const char *ep;
> +uid_t got_uid;
> +gid_t got_gid;
> +int rc;
> +
> +errno = 0;
> +rc = qemu_strtoul(optarg, &ep, 0, &lv);
> +got_uid = lv; /* overflow here is ID in C99 */
> +if (rc || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
> +return false;
> +}
> +
> +lv = 0;
Either zero lv before both qemu_strtoul() or neither one.
> +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
> @@ -144,8 +174,8 @@ 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)) {
> +error_report("User doesn't exist (and is not :)");
The error message no longer includes the offending value. Intentional?
Note for later: @user_uid and @user_gid get set only when @user_pwd
remains null.
> exit(1);
> }
> break;
> @@ -165,18 +195,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);
error_report(), please. More of the same below.
> 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) {
This function is the only user of @user_pwd, @user_uid, @user_gid.
Have y
