Re: socket credentials [Was: CVS commit: src/sys]

[Christoph Badura <bad%bsd.de@localhost>]

I have a couple of questions regarding this change.

Was it intentional that the hack of setting socket credentials only after
the socket is attached to a file descriptor is left undocumented?

On Tue, Dec 29, 2009 at 04:23:43AM +0000, Elad Efrat wrote:
> Modified Files:
>       src/sys/kern: uipc_socket.c uipc_syscalls.c
>       src/sys/sys: socketvar.h
> 
> Log Message:
> Add credentials to to sockets.

> Modified files:
> 
> Index: src/sys/kern/uipc_socket.c
> diff -u src/sys/kern/uipc_socket.c:1.197 src/sys/kern/uipc_socket.c:1.198
> @@ -582,6 +582,7 @@
>               sofree(so);
>               return error;
>       }
> +     so->so_cred = kauth_cred_dup(l->l_cred);
>       sounlock(so);
>       *aso = so;
>       return 0;

Why are you using kauth_cred_dup() instead of the normal kauth_cred_hold()?
If there is a reason, shouldn't it be documented?

> diff -u src/sys/kern/uipc_syscalls.c:1.138 src/sys/kern/uipc_syscalls.c:1.139
> --- src/sys/kern/uipc_syscalls.c:1.138        Sun Dec 20 09:36:06 2009
> +++ src/sys/kern/uipc_syscalls.c      Tue Dec 29 04:23:43 2009
> @@ -228,9 +229,11 @@
>       fp2->f_ops = &socketops;
>       fp2->f_data = so2;
>       error = soaccept(so2, nam);
> +     so2->so_cred = kauth_cred_dup(so->so_cred);

The same applies here, of course.

>       sounlock(so);
>       if (error) {
>               /* an error occurred, free the file descriptor and mbuf */
> +             kauth_cred_free(so2->so_cred);
>               m_freem(nam);
>               mutex_enter(&fp2->f_lock);
>               fp2->f_count++;

Was it intentional to leave the comment misleading after the change?

And lastly some broader questions:

In how many places do you think we should keep credentials in the socket
structure?  E.g. is there a reason to keep so_egid after the the
introduction of so_cred?  What about using so_uidinfo->ui_uid for
authentication?  Stuff like:


        if (so->so_uidinfo->ui_uid == 0)        /* XXX-kauth */
                new->priv = 1;
        else    
                new->priv = 0;

in netinet6/ipsec.c  or

#ifdef __NetBSD__
/* superuser opened socket? */
#define IPSEC_PRIVILEGED_SO(so) ((so)->so_uidinfo->ui_uid == 0)
#endif  /* __NetBSD__ */

in netipsec/ipsec_osdep.h to pick two places at random.

--chris