"J. Bruce Fields" <bfields@xxxxxxxxxxxx> writes:
> On Mon, Mar 04, 2013 at 09:11:16AM -0800, Eric W. Biederman wrote:
>> "J. Bruce Fields" <bfields@xxxxxxxxxxxx> writes:
>>
>> Not strongly. As long as we have the uid ang gid valid checks in
>> there somewhere before we start using these uids and gids much.
>> This does seems to be the case of using out of range uids and gids
>> to mean out of range uids and gids.
>>
>> In the description of my original patch before I split it up, I
>> noted that one of the small dangers might be that I had added
>> some possibly unneceessary uid and gid valid checks. So it seems I had
>> even considered this slight possibility once and noted that there was a
>> slight chance we might have to remove a few of these.
>
> Ah, I should have seen that and checked that, my bad.
I think that text was lost when I split my nfs patch into all of those
patches so I don't expect you ever saw that text.
>> It would be nice to have a comment to say that we expect this to happen
>> so people don't start wondering why there are missing checks on this
>> path.
>
> OK--something like the appended?
It looks good to me.
>> There is also a gid_valid check in the secondary uids. It looks like we
>> should keep that as we don't have any checks for invalid gids in
>> nfsd_setuser. Is that possibly an oversight in nfsd_setuser?
>
> Honestly I don't think anyone's thought that through....
I try and take these moments of that's weird to spark the thinking of
things through. Just in case there is something important was
overlooked.
> But it would seem odd to pass down an "anonymous" supplementary gid--why
> not just leave it off the list instead?
True.
> (BTW, dumb question: is it legal for userspace to use a uid -1? And was
> it illegal before the user namespace patches?)
It is now impossible and thus illegal for userspace to use a uid value
of -1. Previously to use a uid of -1 you had to jump through hoops as
some system calls would allow it (setuid) and some system calls special
cased the value of -1 (setresuid). Which mean using a uid value of -1
was while possible but a bad idea because various things would break in
strange ways.
>> Also looking towards a future in which all of these things don't reside
>> in the initial user namespace. Is mapping any out of range uid to
>> INVALID_UID and then into ex_anon_uid the always the right thing to do?
>>
>> Or is -1 a very special case in this instance. In which case we
>> probably want checks that look like:
>
> svcgssd should pass down either -1 or a valid uid, so I don't think
> we're committed to any particular handling of invalid id's other than
> -1--whatever's easiest.
Sounds good. I won't worry about them in future development then.
> commit d78f5cd062edb400190521e9540b042b4805875b
> Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> Date: Mon Mar 4 08:44:01 2013 -0500
>
> nfsd: fix krb5 handling of anonymous principals
>
> krb5 mounts started failing as of
> 683428fae8c73d7d7da0fa2e0b6beb4d8df4e808 "sunrpc: Update svcgss xdr
> handle to rpsec_contect cache".
>
> The problem is that mounts are usually done with some host principal
> which isn't normally mapped to any user, in which case svcgssd passes
> down uid -1, which the kernel is then expected to map to the
> export-specific anonymous uid or gid.
>
> The new uid_valid/gid_valid checks were therefore causing that downcall
> to fail.
>
> (Note the regression may not have been seen with older userspace that
> tended to map unknown principals to an anonymous id on their own rather
> than leaving it to the kernel.)
>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
Reviewed-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index f7d34e7..c2ab26f 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -447,17 +447,21 @@ static int rsc_parse(struct cache_detail *cd,
> else {
> int N, i;
>
> + /*
> + * NOTE: we don't check uid_valid() or gid_valid(): instead,
> + * -1 id's are later mapped to the (export-specific)
> + * anonymous id by nfsd_setuser.
> + *
> + * (But supplementary gid's get no such special
> + * treatment so are checked for validity here.)
> + */
> /* uid */
> rsci.cred.cr_uid = make_kuid(&init_user_ns, id);
> - if (!uid_valid(rsci.cred.cr_uid))
> - goto out;
>
> /* gid */
> if (get_int(&mesg, &id))
> goto out;
> rsci.cred.cr_gid = make_kgid(&init_user_ns, id);
> - if (!gid_valid(rsci.cred.cr_gid))
> - goto out;
>
> /* number of additional gid's */
> if (get_int(&mesg, &N))
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
