--7AUc2qLy4jB3hD7Z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Jun 23, 2007 at 08:11:18PM +0300, Elad Efrat wrote:
> Jachym Holecek wrote:
> >[Stripping CC somewhat]
>=20
> adding cc back. this is very relevant to all lists, and potentially
> current-users@ and netbsd-users@ as well, as this damages a framework
> in -current and future releases if it stays in the tree.

I expect the interface will get fixed quite soon.

> ># Elad Efrat 2007-06-23:
> >>while the changes to get/setgroups syscall internals and compat calls
> >>will not change the user experience in any way, breaking kauth's opacity
> >>have direct and immediate implications in the form of not allowing much
> >>flexibility when implementing new security models that expand beyond
> >>what is currently allowed by bsd44.
> >
> >Could you provide some specific examples of what was possible before
> >but will be impossible because of David's change?
> >
> >>additionally, it is well worth pointing out that the benefit you
> >>introduced is orthogonal to breaking the interface's opacity, and could
> >>have been introduced either way.
> >
> >I don't quite see how opacity gets harmed -- the group list was a flat
> >array before and it's still a flat array now...

Because it now has to stay a flat array of gid_t's.

One whole point of kauth is that all the types are opaque. Subject to=20
change w/o notice. Something we can do WHATEVER we want with.

Like say start using GUIDs to identify groups.

Sure, doing that would mean a lot of compat code to map back & forth
between gid_t's. And these routines would be in the thick of it. But kauth
abstracts things well enough that we can do it.

ACLs are a place where this matters. Many file systems use GUIDs or SIDs=20
on-disk, so to add proper support, it'd be easiest if we used GUIDs (or=20
SIDs, but Apple went w/ GUIDs, so there's prior art) natively. Doing so=20
has a number of advantages, but that'd be getting farther off topic. :-)

> you're now getting a pointer to an internal buffer where you can change
> it directly without going through the interface.

Agreed.

Ok, so how about some techical discussion, eh?

getting groups:

1) We pass out the pointer to the group array. If we stored groups in some=
=20
way other than gid_t's (say GUIDs), we'd have to malloc memory. We have no=
=20
way to say, "We're done now," and so we would leak memory. Since we don't=
=20
want to do a copy if we're about to do another copy, I can see wanting to=
=20
not copy always. So we can't also just say, "call free()". So I think we=20
either need:

A) kauth_cred_getgrlist_done(kauth_cred_t cred, gid_t *) which will call=20
free if needed.

B) have a different routine that does the copy itself. Pass in a pointer,=
=20
a vmspace identifier, and a count, and it handles getting the gid's there.=
=20
It can either scribble to a kernel memory block (for compat code that has=
=20
to do mapping) or copyout to userland directly.

2) We pass out a ointer to the group array. So right now, rogue code could=
=20
scribble over the group list. I'm don't really think this is an issue as=20
any such rogue code would have to be in the kernel, and could probably=20
already scribble there if it wanted to.


Setting groups:

3) We pass out the memory array, let the caller scribble on it, then=20
process it. This seems like a bad idea. I think what we should do is pass=
=20
in an array and have kauth set the group list to that. I think changing=20
the group set should be atomic. Either it happens or it doesn't. Other=20
users (other threads in the same process for instance) should either see=20
the old group set or the new group set. Nothing else.

The current code, however, isn't atomic. We set the number of groups in
the kauth_cred_setngroups() call. We then let the caller do things. We=20
then kauth_authorize_process() the operation, and if it fails, we error=20
out.

What's really unclear to me is how we are supposed to go back to the=20
original group set at this point. The caller has directly modified the=20
group set, and I see no way to restore the old contents.

4) Since we change the # groups before we finish changing the groups, we=20
can expose concurrent access to group IDs that aren't set yet. Say we=20
raise the count. Before we finish updating the array, other threads can=20
see garbage gids. That's bad.

5) There's nothing that protects against concurrent update. Yes, we have=20
calls to proc_crmod_enter() and proc_cr_mod_leav(), but the caller is=20
scriubbling over the group list when we don't have them held. Think about=
=20
the mess that happens if two threads call into the set routine at once.=20
They both are scribbling over the group array at the same time.

6) In kauth_proc_setgroups(), there are two calls to proc_crmod_leave()=20
that have "ncred" and "cred" passed as different arguements. Is that=20
correct?

Performance:

Earlier in the thread, it was mentioned that we get better performance if
we skip an unneeded malloc()/free() in the system callroutines. I agree. =
=20
However we introduce a number of isses. So.... How often do we call these
routines? I thought a process calls setgroups() only once, and getgroups() =
=20
a hand full of times over its lifetime. I don't think that a malloc()/
free() would really hurt us. And if it does, we can revisit. :-)

Take care,

Bill

--7AUc2qLy4jB3hD7Z
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (NetBSD)

iD8DBQFGfrgyWz+3JHUci9cRAkX1AKCQhdp7LCxh2Qu3H253fdBdmDTbmQCfRMEH
HwKa4y93rNFm1d0lcVvtk+U=
=UhBb
-----END PGP SIGNATURE-----

--7AUc2qLy4jB3hD7Z--