[PATCH 1/2] Fix bug #9329 - Directory listing with SeBackup can crash smbd.

[Andrew Bartlett]

On Fri, 2012-10-26 at 08:49 -0700, Jeremy Allison wrote:
> On Fri, Oct 26, 2012 at 12:52:34PM +1100, Andrew Bartlett wrote:
> > 
> > This is annoyingly more complex than the previous patch - what didn't
> > work out about that?  (Just so I can understand this security-critical
> > code better).  
> 
> Unfortunately it had to be for the reasons below.
> 
> The previous patch was correct in all but one case - which I mention
> in the check-in messsage. To expand on that:

I figured as much, just wanted to dig into it a bit mroe. 

> When doing a delete on close, we can have the case where two
> separate users have the file open, and one of them sets the
> delete on close bit and then closes the file. But the file
> must remain undeleted until the second user closes the file.

OK. 

> But the second user may not have permissions to delete the
> file - so inside the close code we take care of this by storing
> the UNIX and Windows tokens with the open file when the first
> close-with-delete comes in, and then using push_sec_ctx()/set_sec_ctx()
> to take on the original users permissions before deleting
> the file, followed by a pop_sec_ctx() when we're done.

OK, can you point me at where we do this in the code?

> This may seem an extreme case, but it actually occurred
> in practice on a customer site.

Delete-on-close is a funny thing indeed.

> Thus this is the one case where using the token associated
> with the conn struct doesn't work - the token on the conn
> struct in this case is from the user doing the close, not
> the token we must adopt (the one on the stack) from the
> user we must impersonate to do the delete - that token
> is the one higher up on the stack (if we've done a become_root()).
> 
> The first patch would have left the delete-on-close problem
> in case if we ever added a become_root()/unbecome_root()
> pair into the delete code (this came to me in the shower
> when I was mentally reviewing the first fix, something didn't
> quite sit right with it :-).

So this doesn't happen right now, but could happen?

> > Also, if get_current_nttok() really isn't related to the conn argument,
> > we probably should remove it.
> 
> I'd prefer to leave it as I still have hopes of moving to
> the conn token in future if we move the delete on close
> case to an asynchronous method (this leaves the code consistent
> with all the other get_current_XXX() calls, which also
> should eventually move to the conn token).
> 
> If we decide to do that change it's a different cleanup patchset IMHO
> than the fix for this specific bug.

I would also prefer to move this to the conn, which is why the change as
proposed (and now pushed) still doesn't sit right with me.  Given that
the token has to be associated with the fsp to change to the correct
user on delete-on-close (do to the push_sec_ctx() and pop_sec_ctx()),
why can't we then just pass it to the few methods that need the NT
token?

Andrew Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org