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

[Jeremy Allison]

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:

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.

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.

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

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

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

Jeremy.