On Sat, Jun 23, 2007 at 08:11:18PM +0300, Elad Efrat wrote: > Jachym Holecek wrote: > >[Stripping CC somewhat] > > 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 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 on-disk, so to add proper support, it'd be easiest if we used GUIDs (or SIDs, but Apple went w/ GUIDs, so there's prior art) natively. Doing so 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 way other than gid_t's (say GUIDs), we'd have to malloc memory. We have no way to say, "We're done now," and so we would leak memory. Since we don't want to do a copy if we're about to do another copy, I can see wanting to not copy always. So we can't also just say, "call free()". So I think we either need: A) kauth_cred_getgrlist_done(kauth_cred_t cred, gid_t *) which will call free if needed. B) have a different routine that does the copy itself. Pass in a pointer, a vmspace identifier, and a count, and it handles getting the gid's there. It can either scribble to a kernel memory block (for compat code that has to do mapping) or copyout to userland directly. 2) We pass out a ointer to the group array. So right now, rogue code could scribble over the group list. I'm don't really think this is an issue as any such rogue code would have to be in the kernel, and could probably already scribble there if it wanted to. Setting groups: 3) We pass out the memory array, let the caller scribble on it, then process it. This seems like a bad idea. I think what we should do is pass in an array and have kauth set the group list to that. I think changing the group set should be atomic. Either it happens or it doesn't. Other users (other threads in the same process for instance) should either see 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 then kauth_authorize_process() the operation, and if it fails, we error out. What's really unclear to me is how we are supposed to go back to the original group set at this point. The caller has directly modified the 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 can expose concurrent access to group IDs that aren't set yet. Say we raise the count. Before we finish updating the array, other threads can see garbage gids. That's bad. 5) There's nothing that protects against concurrent update. Yes, we have calls to proc_crmod_enter() and proc_cr_mod_leav(), but the caller is scriubbling over the group list when we don't have them held. Think about the mess that happens if two threads call into the set routine at once. 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() that have "ncred" and "cred" passed as different arguements. Is that 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. 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() 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
Attachment:
pgpuGKvIRkDNr.pgp
Description: PGP signature