session setup bugfix and torture test

Stefan (metze) Metzmacher metze at samba.org
Fri Sep 20 11:41:38 CEST 2013


Hi Michael,

I think this changes are a good start, but I try to
fix this more broadly similar to this change

commit e6a58d370403e818bc2cfb8389751b78adcc14fd
Author: Stefan Metzmacher <metze at samba.org>
Date:   Tue Jul 9 16:38:59 2013 +0200

    s4:rpc_server: make sure we don't terminate a connection with
pending requests (bug #9820)

    Sadly we may have nested event loops, which won't work correctly with
    broken connections, that's why we have to do this...

    Signed-off-by: Stefan Metzmacher <metze at samba.org>
    Reviewed-by: Andrew Bartlett <abartlet at samba.org>

    Autobuild-User(master): Stefan Metzmacher <metze at samba.org>
    Autobuild-Date(master): Wed Jul 10 08:47:38 CEST 2013 on sn-devel-104

The goal should be that we move any session, tcon and open to a deleted list
on the connection and just mark them as deleted like this

  session->status = NT_STATUS_USER_SESSION_DELETED;
  tcon->status = NT_STATUS_NETWORK_NAME_DELETED;
  open->status = NT_STATUS_FILE_CLOSED;

And do through all pending request which match the closed object
and do tevent_req_cancel(smb2req->subreq);

Before we process any new incoming request, we would scan the deleted lists
and clean the objects up if they aren't used anymore.

Am 20.09.2013 08:13, schrieb Michael Adam:
> --- a/source3/smbd/smb2_sesssetup.c
> +++ b/source3/smbd/smb2_sesssetup.c
> @@ -445,23 +445,38 @@ struct smbd_smb2_session_setup_state {
>  
>  static int pp_self_ref_destructor(struct smbd_smb2_session_setup_state **pp_state)
>  {
> -	(*pp_state)->session = NULL;
>  	/*
>  	 * To make things clearer, ensure the pp_self_ref
>  	 * pointer is nulled out. We're never going to
>  	 * access this again.
>  	 */
>  	(*pp_state)->pp_self_ref = NULL;
> +
> +	if ((*pp_state)->session == NULL) {
> +		return 0;
> +	}
> +
> +	(*pp_state)->session = NULL;
> +
>  	return 0;
>  }

I think we should remove this hunk, it doesn't change anything.

> 
> +	substitute_user = generate_random_str_list(tctx, 8,
> +		"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789");
> +	ok = cli_credentials_set_username(broken_creds, substitute_user,
> +					  CRED_SPECIFIED);
> +	CHECK_VAL(ok, true);
> +

Maybe we should just set a wrong password here?

> +	status = smb2_session_setup_spnego(tree->session,
> +					   broken_creds,
> +					   0 /* previous_session_id */);
> +	CHECK_STATUS(status, NT_STATUS_LOGON_FAILURE);
> +
> +	torture_comment(tctx, "did failed reauth\n");
> +	/*
> +	 * now verify that the invalid session reauth has closed our session
> +	 */
> +
> +	if (encrypted) {
> +		expected = NT_STATUS_CONNECTION_DISCONNECTED;
> +	} else if (using_kerberos) {
> +		expected = NT_STATUS_INVALID_NETWORK_RESPONSE;

Is this correct? didn't we get that from smb2_session_setup_spnego()
I can't see why the create should fail with that status.

> +	} else {
> +		expected = NT_STATUS_USER_SESSION_DELETED;
> +	}
> +
> +	smb2_oplock_create_share(&io1, fname,
> +				 smb2_util_share_access(""),
> +				 smb2_util_oplock_level("b"));
> +
> +	status = smb2_create(tree, mem_ctx, &io1);
> +	CHECK_STATUS(status, expected);

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20130920/40bf6b9a/attachment.pgp>


More information about the samba-technical mailing list