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