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

[Andrew Bartlett]

On Thu, 2012-10-25 at 17:07 -0700, Jeremy Allison wrote:
> When we do a become_root()/unbecome_root() pair to temporarily
> raise privilege, this NULLs out the NT token. If we're within
> a become_root()/unbecome_root() pair then return the previous
> token on the stack as our NT token. This is what we should be
> using to check against NT ACLs in the file server. This copes
> with security context changing when removing a file on close
> under the context of another user (when 2 users have a file
> open, one sets delete on close and then the other user has
> to actually do the delete).
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/smbd/proto.h   |    1 +
>  source3/smbd/sec_ctx.c |   25 +++++++++++++++++++++++++
>  source3/smbd/uid.c     |   12 +++++++++++-
>  3 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
> index d218184..c80ef14 100644
> --- a/source3/smbd/proto.h
> +++ b/source3/smbd/proto.h
> @@ -952,6 +952,7 @@ void set_sec_ctx(uid_t uid, gid_t gid, int ngroups, gid_t *groups, const struct
>  void set_root_sec_ctx(void);
>  bool pop_sec_ctx(void);
>  void init_sec_ctx(void);
> +const struct security_token *sec_ctx_active_token(void);
>  
>  /* The following definitions come from smbd/server.c  */
>  
> diff --git a/source3/smbd/sec_ctx.c b/source3/smbd/sec_ctx.c
> index 83674a2..07565bf 100644
> --- a/source3/smbd/sec_ctx.c
> +++ b/source3/smbd/sec_ctx.c
> @@ -466,3 +466,28 @@ void init_sec_ctx(void)
>  	current_user.vuid = UID_FIELD_INVALID;
>  	current_user.nt_user_token = NULL;
>  }
> +
> +/*************************************************************
> + Called when we're inside a become_root() temporary escalation
> + of privileges and the nt_user_token is NULL. Return the last
> + active token on the context stack. We know there is at least
> + one valid non-NULL token on the stack so panic if we underflow.
> +*************************************************************/
> +
> +const struct security_token *sec_ctx_active_token(void)
> +{
> +	int stack_index = sec_ctx_stack_ndx;
> +	struct sec_ctx *ctx_p = &sec_ctx_stack[stack_index];
> +
> +	while (ctx_p->token == NULL) {
> +		stack_index--;
> +		if (stack_index < 0) {
> +			DEBUG(0, ("Security context active token "
> +				"stack underflow!\n"));
> +			smb_panic("Security context active token "
> +				"stack underflow!");
> +		}
> +		ctx_p = &sec_ctx_stack[stack_index];
> +	}
> +	return ctx_p->token;
> +}
> diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
> index eac5d9d..30c7154 100644
> --- a/source3/smbd/uid.c
> +++ b/source3/smbd/uid.c
> @@ -533,9 +533,19 @@ const struct security_unix_token *get_current_utok(connection_struct *conn)
>  	return &current_user.ut;
>  }
>  
> +/****************************************************************************
> + Return the Windows token we are running effectively as on this connection.
> + If this is currently a NULL token as we're inside become_root() - a temporary
> + UNIX security override, then we search up the stack for the previous active
> + token.
> +****************************************************************************/
> +
>  const struct security_token *get_current_nttok(connection_struct *conn)
>  {
> -	return current_user.nt_user_token;
> +	if (current_user.nt_user_token) {
> +		return current_user.nt_user_token;
> +	}
> +	return sec_ctx_active_token();
>  }
>  
>  uint64_t get_current_vuid(connection_struct *conn)

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

Also, if get_current_nttok() really isn't related to the conn argument,
we probably should remove it.

Andrew Bartlett

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