Solving the recursive 'wrong uid' problem in Samba4

[simo]

On Tue, 2009-03-03 at 22:18 +1100, Andrew Bartlett wrote:
> I spent part of today looking into the UID wrong problem that hits
> production use of Samba4.
> 
> The basic problem is that when we call event_loop_once inside code that
> has changed Samba's UID, the event loop runs with a different UID.
> 
> Tridge proposed that we modify event_loop_once to optionally run a hook
> to change the UID back to root, run the loop once, and to restore the
> UID.
> 
> I would like to take the design a step further - create a new function:
> 
> int event_loop_until(struct tevent_context *ev, (*until_fn)(void *),
> void *private) {
> 	void *hook_state = NULL;
> 	if (ev->hook) {
> 		ret = ev->hook(&hook_state, true);
> 		if (ret != 0) {
> 			return ret;
> 		}
> 	}
> 	while (until_fn(private)) {
> 		ret = event_loop_once(ev);
> 		if (ret != 0) {
> 			break;
> 		}
> 	}
> 	
> 	if (ev->hook) {
> 		int ev_ret = ev->hook(&hook_state, false);
> 		if (ev_ret != 0) {
> 			return ev_ret;
> 		}
> 	}
> 
> 	return ret;
> }
> 
> This would remove the loop form all the calling libraries that currently
> call event_loop_once, but allow a single place where we can reset the
> UID etc before we start doing events, and put things back where they
> were on the exit from the event loop.

How do you know what's the next thing that is executed when you call
event_loop_once() in there ?
What does exactly until_fn() do ? Does it test to know when it is time
to run the original un-privileged function ?
What happen if another callback sets a different UID while the initial
one is not completed ?

> The hook would be set onto the event context in the Samba server only,
> when it is first created.  It would be a no-op when the current UID is
> already root, so would only really 'do work' in the NTVFS layer. 
> 
> The ultimate aim would be to remove this function, because all the
> semi-async code is gone, but this won't happen for a while.  In the
> meantime it might avoid the serious issue we currently have with nested
> events.  This allows the storage of the nested security contexts to be
> on the program stack, so there is much less risk of getting it mixed
> up. 
> 
> It's late here, and I've probably not been clear, but I wanted to float
> the idea overnight,

It's a hack on top of another hack, if it actually solves anything it
can be used, but are you sure this way will work ?

Wouldn't it be better instead to set the uid in the private data of
specific event contexts, and switch to it when the event callback is
called (inside the callback), while going back to root every time a
function runs event_loop_once ?

Not sure how invasive this would be for the NTVFS but it seem to me it
would give a bit more control over what's going on with uid changes.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>