Solving the recursive 'wrong uid' problem in Samba4

[Andrew Bartlett]

On Tue, 2009-03-03 at 08:56 -0500, simo wrote:
> 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 ?

You don't.  The idea is simply to restore to a similar state to what
event_loop_wait() started with.

> What does exactly until_fn() do ? Does it test to know when it is time
> to run the original un-privileged function ?

Yeah - things like (from the ldap code):

	while (req->state < LDAP_REQUEST_DONE) {
		if (tevent_loop_once(req->conn->event.event_ctx) != 0) {
			req->status = NT_STATUS_UNEXPECTED_NETWORK_ERROR;
			break;
		}
	}
	return req->status;
would become

bool ldap_done_yet(void *ptr) {
	ldap_request *req = ptr;
	return req->state < LDAP_REQUEST_DONE;
}

if (tevent_loop_until(req->conn->event.event_ctx, ldap_done_yet, req) !=
0) {
	return req->status
}

> What happen if another callback sets a different UID while the initial
> one is not completed ?

That's fine, the state always gets reset, loosing what the new callback
did.  As long as the recursion occurs via this function the correct UID
to reset to is kept on the program stack, and can't be lost.

> > 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 ?

No, but I wanted to see if anybody else could see before I tried to code
it :-)

> 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.

The problem here is that we don't know what UID we want to be for an
packet until we unpack it.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Red Hat Inc.                  http://redhat.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.samba.org/archive/samba-technical/attachments/20090304/04406187/attachment.bin