[PATCH] Fix for reopened bug 9175

Ralph Böhme slow at samba.org
Tue Nov 13 11:28:17 UTC 2018


On Tue, Nov 13, 2018 at 09:12:42AM +0100, Ralph Böhme wrote:
>On Tue, Nov 13, 2018 at 08:17:51AM +0100, Stefan Metzmacher wrote:
>>Hi Ralph,
>>
>>>On Mon, Nov 12, 2018 at 04:51:29PM +0100, Andreas Schneider wrote:
>>>>On Monday, 12 November 2018 12:17:59 CET Ralph Böhme via samba-technical
>>>>wrote:
>>>>>Hi metze,
>>>>>
>>>>>shall we apply the attached patch?
>>>>
>>>>I think this is more correct so +1 from my side.
>>>
>>>thanks.
>>>
>>>metze, what do you think?
>>
>>Can we create a test for this that verifies that
>>smbXcli_conn_is_connected() returns false,
>>when a request fails with SESSION_EXPIRED?
>
>sure. Let me see...

attached.

The second patch also passed CI as part of 
https://gitlab.com/samba-team/devel/samba/pipelines/36191198 

Plus it passes manual make test TESTS=smb2.session

Please review&push if happy. Thanks!

-slow

-- 
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
GPG Key Fingerprint:           FAE2 C608 8A24 2520 51C5
                               59E4 AA1E 9B71 2639 9E46
-------------- next part --------------
From dffeb4c83ede9b866bade84f298925826a58b1a2 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 13 Nov 2018 12:08:10 +0100
Subject: [PATCH 1/2] s4:torture/smb2/session: test
 smbXcli_session_set_disconnect_expired() works

This adds a simple test that verifies that after having set
smbXcli_session_set_disconnect_expired() a session gets disconnected
when it expires.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=9175

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 selftest/knownfail.d/samba3.smb2.session |   2 +
 source4/torture/smb2/session.c           | 110 +++++++++++++++++++++++
 2 files changed, 112 insertions(+)
 create mode 100644 selftest/knownfail.d/samba3.smb2.session

diff --git a/selftest/knownfail.d/samba3.smb2.session b/selftest/knownfail.d/samba3.smb2.session
new file mode 100644
index 00000000000..1ef3605d6b8
--- /dev/null
+++ b/selftest/knownfail.d/samba3.smb2.session
@@ -0,0 +1,2 @@
+^samba3.smb2.session krb5.expire_disconnect\(ad_dc\)
+^samba3.smb2.session krb5.expire_disconnect\(ad_member\)
diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c
index 57a5addcfcc..3917e0c09c4 100644
--- a/source4/torture/smb2/session.c
+++ b/source4/torture/smb2/session.c
@@ -1596,6 +1596,114 @@ static bool test_session_expire2e(struct torture_context *tctx)
 				     true); /* force_encryption */
 }
 
+static bool test_session_expire_disconnect(struct torture_context *tctx)
+{
+	NTSTATUS status;
+	bool ret = false;
+	struct smbcli_options options;
+	const char *host = torture_setting_string(tctx, "host", NULL);
+	const char *share = torture_setting_string(tctx, "share", NULL);
+	struct cli_credentials *credentials = popt_get_cmdline_credentials();
+	struct smb2_tree *tree = NULL;
+	enum credentials_use_kerberos use_kerberos;
+	char fname[256];
+	struct smb2_handle _h1;
+	struct smb2_handle *h1 = NULL;
+	struct smb2_create io1;
+	union smb_fileinfo qfinfo;
+	bool connected;
+
+	use_kerberos = cli_credentials_get_kerberos_state(credentials);
+	if (use_kerberos != CRED_MUST_USE_KERBEROS) {
+		torture_warning(tctx, "smb2.session.expire1 requires -k yes!");
+		torture_skip(tctx, "smb2.session.expire1 requires -k yes!");
+	}
+
+	cli_credentials_invalidate_ccache(credentials, CRED_SPECIFIED);
+
+	lpcfg_set_option(tctx->lp_ctx, "gensec_gssapi:requested_life_time=4");
+	lpcfg_smbcli_options(tctx->lp_ctx, &options);
+	options.signing = SMB_SIGNING_REQUIRED;
+
+	status = smb2_connect(tctx,
+			      host,
+			      lpcfg_smb_ports(tctx->lp_ctx),
+			      share,
+			      lpcfg_resolve_context(tctx->lp_ctx),
+			      credentials,
+			      &tree,
+			      tctx->ev,
+			      &options,
+			      lpcfg_socket_options(tctx->lp_ctx),
+			      lpcfg_gensec_settings(tctx, tctx->lp_ctx)
+			      );
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"smb2_connect failed");
+
+	smbXcli_session_set_disconnect_expired(tree->session->smbXcli);
+
+	/* Add some random component to the file name. */
+	snprintf(fname, sizeof(fname), "session_expire1_%s.dat",
+		 generate_random_str(tctx, 8));
+
+	smb2_util_unlink(tree, fname);
+
+	smb2_oplock_create_share(&io1, fname,
+				 smb2_util_share_access(""),
+				 smb2_util_oplock_level("b"));
+	io1.in.create_options |= NTCREATEX_OPTIONS_DELETE_ON_CLOSE;
+
+	status = smb2_create(tree, tctx, &io1);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"smb2_create failed");
+	_h1 = io1.out.file.handle;
+	h1 = &_h1;
+	CHECK_CREATED(tctx, &io1, CREATED, FILE_ATTRIBUTE_ARCHIVE);
+	torture_assert_int_equal(tctx, io1.out.oplock_level,
+					smb2_util_oplock_level("b"),
+					"oplock_level incorrect");
+
+	/* get the security descriptor */
+
+	ZERO_STRUCT(qfinfo);
+
+	qfinfo.access_information.level = RAW_FILEINFO_ACCESS_INFORMATION;
+	qfinfo.access_information.in.file.handle = _h1;
+
+	torture_comment(tctx, "query info => OK\n");
+
+	ZERO_STRUCT(qfinfo.access_information.out);
+	status = smb2_getinfo_file(tree, tctx, &qfinfo);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
+					"smb2_getinfo_file failed");
+
+	torture_comment(tctx, "sleep 10 seconds\n");
+	smb_msleep(10*1000);
+
+	torture_comment(tctx, "query info => EXPIRED\n");
+	ZERO_STRUCT(qfinfo.access_information.out);
+	status = smb2_getinfo_file(tree, tctx, &qfinfo);
+	torture_assert_ntstatus_equal_goto(tctx, status,
+					   NT_STATUS_NETWORK_SESSION_EXPIRED,
+					   ret, done, "smb2_getinfo_file "
+					   "returned unexpected status");
+
+	connected = smbXcli_conn_is_connected(tree->session->transport->conn);
+	torture_assert_goto(tctx, !connected, ret, done, "connected\n");
+
+	ret = true;
+done:
+	cli_credentials_invalidate_ccache(credentials, CRED_SPECIFIED);
+
+	if (h1 != NULL) {
+		smb2_util_close(tree, *h1);
+	}
+
+	talloc_free(tree);
+	lpcfg_set_option(tctx->lp_ctx, "gensec_gssapi:requested_life_time=0");
+	return ret;
+}
+
 bool test_session_bind1(struct torture_context *tctx, struct smb2_tree *tree1)
 {
 	const char *host = torture_setting_string(tctx, "host", NULL);
@@ -1754,6 +1862,8 @@ struct torture_suite *torture_smb2_session_init(TALLOC_CTX *ctx)
 	torture_suite_add_simple_test(suite, "expire1e", test_session_expire1e);
 	torture_suite_add_simple_test(suite, "expire2s", test_session_expire2s);
 	torture_suite_add_simple_test(suite, "expire2e", test_session_expire2e);
+	torture_suite_add_simple_test(suite, "expire_disconnect",
+				      test_session_expire_disconnect);
 	torture_suite_add_1smb2_test(suite, "bind1", test_session_bind1);
 
 	suite->description = talloc_strdup(suite, "SMB2-SESSION tests");
-- 
2.17.2


From b8d84d6e0584e77d275e9959a568c8d705eff1d3 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 7 Nov 2018 14:00:25 +0100
Subject: [PATCH 2/2] libcli/smb: don't overwrite status code

The original commit c5cd22b5bbce724dcd68fe94320382b3f772cabf from bug
9175 never worked, as the preceeding signing check overwrote the status
variable.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=9175

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 libcli/smb/smbXcli_base.c                | 12 +++++++-----
 selftest/knownfail.d/samba3.smb2.session |  2 --
 2 files changed, 7 insertions(+), 7 deletions(-)
 delete mode 100644 selftest/knownfail.d/samba3.smb2.session

diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
index d0cc33b8b05..40480c83aa0 100644
--- a/libcli/smb/smbXcli_base.c
+++ b/libcli/smb/smbXcli_base.c
@@ -3908,15 +3908,17 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct smbXcli_conn *conn,
 		}
 
 		if (signing_key) {
-			status = smb2_signing_check_pdu(*signing_key,
-							state->conn->protocol,
-							&cur[1], 3);
-			if (!NT_STATUS_IS_OK(status)) {
+			NTSTATUS signing_status;
+
+			signing_status = smb2_signing_check_pdu(*signing_key,
+								state->conn->protocol,
+								&cur[1], 3);
+			if (!NT_STATUS_IS_OK(signing_status)) {
 				/*
 				 * If the signing check fails, we disconnect
 				 * the connection.
 				 */
-				return status;
+				return signing_status;
 			}
 		}
 
diff --git a/selftest/knownfail.d/samba3.smb2.session b/selftest/knownfail.d/samba3.smb2.session
deleted file mode 100644
index 1ef3605d6b8..00000000000
--- a/selftest/knownfail.d/samba3.smb2.session
+++ /dev/null
@@ -1,2 +0,0 @@
-^samba3.smb2.session krb5.expire_disconnect\(ad_dc\)
-^samba3.smb2.session krb5.expire_disconnect\(ad_member\)
-- 
2.17.2



More information about the samba-technical mailing list