[PATCH] Fix bug 13215
Volker Lendecke
Volker.Lendecke at SerNet.DE
Fri Jan 12 10:52:54 UTC 2018
Hi!
Review appreciated!
Thanks, Volker
--
Besuchen Sie die verinice.XP 2018 in Berlin,
Anwenderkonferenz für Informationssicherheit
vom 21.-23.03.2018 im Sofitel Kurfürstendamm
Info & Anmeldung hier: http://veriniceXP.org
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 123d3230978e5be5ba2c4ad66e701ecc7edf0975 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 10 Jan 2018 14:29:01 +0100
Subject: [PATCH 1/6] smbd: Fix a typo
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/smb2_server.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 5290c05cb22..565f9996245 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2220,7 +2220,7 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts(
* a 16 bit overflow of the client-submitted sequence
* number:
*
- * If the stored channel squence number is more than
+ * If the stored channel sequence number is more than
* 0x7FFF larger than the one from the request, then
* the client-provided sequence number has likely
* overflown. We treat this case as valid instead
--
2.11.0
From b5b3eb56a141dfaf5bddeca9556b5838147cf650 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 10 Jan 2018 15:51:56 +0100
Subject: [PATCH 2/6] torture4: Fix typos
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source4/torture/smb2/replay.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/source4/torture/smb2/replay.c b/source4/torture/smb2/replay.c
index f15fc849125..889ecd73c72 100644
--- a/source4/torture/smb2/replay.c
+++ b/source4/torture/smb2/replay.c
@@ -473,7 +473,7 @@ done:
}
/**
- * Test Durablity V2 Create Replay Detection on Single Channel.
+ * Test Durability V2 Create Replay Detection on Single Channel.
*/
static bool test_replay_dhv2_oplock1(struct torture_context *tctx,
struct smb2_tree *tree)
@@ -560,7 +560,7 @@ done:
}
/**
- * Test Durablity V2 Create Replay Detection on Single Channel.
+ * Test Durability V2 Create Replay Detection on Single Channel.
* Hand in a different oplock level in the replay.
* Server responds with the handed in oplock level and
* corresponding durable status, but does not change the
@@ -697,7 +697,7 @@ done:
}
/**
- * Test Durablity V2 Create Replay Detection on Single Channel.
+ * Test Durability V2 Create Replay Detection on Single Channel.
* Replay with a different share mode. The share mode of
* the opened file is not changed by this.
*/
@@ -823,7 +823,7 @@ done:
}
/**
- * Test Durablity V2 Create Replay Detection on Single Channel.
+ * Test Durability V2 Create Replay Detection on Single Channel.
* Create with an oplock, and replay with a lease.
*/
static bool test_replay_dhv2_oplock_lease(struct torture_context *tctx,
@@ -927,7 +927,7 @@ done:
/**
- * Test durablity v2 create replay detection on single channel.
+ * Test durability v2 create replay detection on single channel.
* Variant with leases instead of oplocks:
* - open a file with a rh lease
* - upgrade to a rwh lease with a second create
@@ -1065,7 +1065,7 @@ done:
}
/**
- * Test durablity v2 create replay detection on single channel.
+ * Test durability v2 create replay detection on single channel.
* Variant with leases instead of oplocks, where the
* replay does not specify the original lease level but
* just a "R" lease. This still gives the upgraded lease
@@ -1216,7 +1216,7 @@ done:
}
/**
- * Test durablity v2 create replay detection on single channel.
+ * Test durability v2 create replay detection on single channel.
* create with a lease, and replay with a different lease key
*/
static bool test_replay_dhv2_lease3(struct torture_context *tctx,
@@ -1349,7 +1349,7 @@ done:
}
/**
- * Test durablity v2 create replay detection on single channel.
+ * Test durability v2 create replay detection on single channel.
* Do the original create with a lease, and do the replay
* with an oplock.
*/
@@ -1758,7 +1758,7 @@ done:
}
/**
- * Test Durablity V2 Create Replay Detection on Multi Channel
+ * Test Durability V2 Create Replay Detection on Multi Channel
*/
static bool test_replay3(struct torture_context *tctx, struct smb2_tree *tree1)
{
@@ -2162,7 +2162,7 @@ done:
}
/**
- * Test Durablity V2 Persistent Create Replay on a Single Channel
+ * Test Durability V2 Persistent Create Replay on a Single Channel
*/
static bool test_replay5(struct torture_context *tctx, struct smb2_tree *tree)
{
--
2.11.0
From 55a94305808fbb967c66c1c10a32a934944552b2 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 10 Jan 2018 14:59:08 +0100
Subject: [PATCH 3/6] smbd: Remove a "!" from an if-condition for easier
readability
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13215
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/smbd/smb2_server.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 565f9996245..69788e9b2f5 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2231,11 +2231,11 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts(
cmp *= -1;
}
- if (!(flags & SMB2_HDR_FLAG_REPLAY_OPERATION)) {
- if (cmp == 0) {
+ if (flags & SMB2_HDR_FLAG_REPLAY_OPERATION) {
+ if (cmp == 0 && op->pre_request_count == 0) {
op->request_count += 1;
req->request_counters_updated = true;
- } else if (cmp > 0) {
+ } else if (cmp > 0 && op->pre_request_count == 0) {
op->pre_request_count += op->request_count;
op->request_count = 1;
op->global->channel_sequence = channel_sequence;
@@ -2245,10 +2245,10 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts(
return NT_STATUS_FILE_NOT_AVAILABLE;
}
} else {
- if (cmp == 0 && op->pre_request_count == 0) {
+ if (cmp == 0) {
op->request_count += 1;
req->request_counters_updated = true;
- } else if (cmp > 0 && op->pre_request_count == 0) {
+ } else if (cmp > 0) {
op->pre_request_count += op->request_count;
op->request_count = 1;
op->global->channel_sequence = channel_sequence;
--
2.11.0
From 7561d2893bba8d98c526709c38dd68ba5955b9ca Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 11 Jan 2018 15:34:45 +0100
Subject: [PATCH 4/6] smbd: Fix channel sequence number checks for long-running
requests
When the client's supplied csn overflows and hits a pending, long-running
request's csn, we panic. Fix this by counting the overflows in
smbXsrv_open_global0->channel_generation
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13215
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/librpc/idl/smbXsrv.idl | 3 ++-
source3/smbd/globals.h | 1 +
source3/smbd/smb2_server.c | 15 ++++++++++++++-
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/source3/librpc/idl/smbXsrv.idl b/source3/librpc/idl/smbXsrv.idl
index 1bfa51ea912..d3f8d30d1e3 100644
--- a/source3/librpc/idl/smbXsrv.idl
+++ b/source3/librpc/idl/smbXsrv.idl
@@ -430,7 +430,8 @@ interface smbXsrv
uint32 durable_timeout_msec;
boolean8 durable;
DATA_BLOB backend_cookie;
- hyper channel_sequence;
+ uint16 channel_sequence;
+ hyper channel_generation;
} smbXsrv_open_global0;
typedef union {
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index 78f1260909d..69db07a490b 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -744,6 +744,7 @@ struct smbd_smb2_request {
* adapted again in reply.
*/
bool request_counters_updated;
+ uint64_t channel_generation;
/*
* The sub request for async backend calls.
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 69788e9b2f5..a731880e98e 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2158,6 +2158,7 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts(
struct smbXsrv_connection *xconn = req->xconn;
const uint8_t *inhdr;
uint16_t channel_sequence;
+ uint8_t generation_wrap = 0;
uint32_t flags;
int cmp;
struct smbXsrv_open *op;
@@ -2184,6 +2185,14 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts(
channel_sequence = SVAL(inhdr, SMB2_HDR_CHANNEL_SEQUENCE);
cmp = channel_sequence - op->global->channel_sequence;
+ if (cmp < 0) {
+ /*
+ * csn wrap. We need to watch out for long-running
+ * requests that are still sitting on a previously
+ * used csn. SMB2_OP_NOTIFY can take VERY long.
+ */
+ generation_wrap += 1;
+ }
if (abs(cmp) > INT16_MAX) {
/*
@@ -2239,6 +2248,7 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts(
op->pre_request_count += op->request_count;
op->request_count = 1;
op->global->channel_sequence = channel_sequence;
+ op->global->channel_generation += generation_wrap;
update_open = true;
req->request_counters_updated = true;
} else if (modify_call) {
@@ -2252,12 +2262,14 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts(
op->pre_request_count += op->request_count;
op->request_count = 1;
op->global->channel_sequence = channel_sequence;
+ op->global->channel_generation += generation_wrap;
update_open = true;
req->request_counters_updated = true;
} else if (modify_call) {
return NT_STATUS_FILE_NOT_AVAILABLE;
}
}
+ req->channel_generation = op->global->channel_generation;
if (update_open) {
status = smbXsrv_open_update(op);
@@ -2744,7 +2756,8 @@ static void smbd_smb2_request_reply_update_counts(struct smbd_smb2_request *req)
inhdr = SMBD_SMB2_IN_HDR_PTR(req);
channel_sequence = SVAL(inhdr, SMB2_HDR_CHANNEL_SEQUENCE);
- if (op->global->channel_sequence == channel_sequence) {
+ if ((op->global->channel_sequence == channel_sequence) &&
+ (op->global->channel_generation == req->channel_generation)) {
SMB_ASSERT(op->request_count > 0);
op->request_count -= 1;
} else {
--
2.11.0
From a98969b716fdaad45094c67d1cce88e0ff16f455 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 11 Jan 2018 11:25:49 +0100
Subject: [PATCH 5/6] smbXcli: Add "force_channel_sequence"
This enables use of the channel sequence number even for
non-multi-channel servers. This makes our client invalid, but we need to
protect against broken clients with tests.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13215
Signed-off-by: Volker Lendecke <vl at samba.org>
---
libcli/smb/smbXcli_base.c | 15 ++++++++++++++-
libcli/smb/smbXcli_base.h | 4 ++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
index 6335ce0c121..e9fdc1dc32a 100644
--- a/libcli/smb/smbXcli_base.c
+++ b/libcli/smb/smbXcli_base.c
@@ -138,6 +138,8 @@ struct smbXcli_conn {
uint8_t io_priority;
+ bool force_channel_sequence;
+
uint8_t preauth_sha512[64];
} smb2;
@@ -549,6 +551,17 @@ const struct GUID *smbXcli_conn_server_guid(struct smbXcli_conn *conn)
return &conn->smb1.server.guid;
}
+bool smbXcli_conn_get_force_channel_sequence(struct smbXcli_conn *conn)
+{
+ return conn->smb2.force_channel_sequence;
+}
+
+void smbXcli_conn_set_force_channel_sequence(struct smbXcli_conn *conn,
+ bool v)
+{
+ conn->smb2.force_channel_sequence = v;
+}
+
struct smbXcli_conn_samba_suicide_state {
struct smbXcli_conn *conn;
struct iovec iov;
@@ -2899,7 +2912,7 @@ struct tevent_req *smb2cli_req_create(TALLOC_CTX *mem_ctx,
uint32_t flags = 0;
uint32_t tid = 0;
uint64_t uid = 0;
- bool use_channel_sequence = false;
+ bool use_channel_sequence = conn->smb2.force_channel_sequence;
uint16_t channel_sequence = 0;
bool use_replay_flag = false;
diff --git a/libcli/smb/smbXcli_base.h b/libcli/smb/smbXcli_base.h
index d0ee04f4bb0..20ef26e3353 100644
--- a/libcli/smb/smbXcli_base.h
+++ b/libcli/smb/smbXcli_base.h
@@ -59,6 +59,10 @@ uint16_t smbXcli_conn_max_requests(struct smbXcli_conn *conn);
NTTIME smbXcli_conn_server_system_time(struct smbXcli_conn *conn);
const DATA_BLOB *smbXcli_conn_server_gss_blob(struct smbXcli_conn *conn);
const struct GUID *smbXcli_conn_server_guid(struct smbXcli_conn *conn);
+bool smbXcli_conn_get_force_channel_sequence(struct smbXcli_conn *conn);
+void smbXcli_conn_set_force_channel_sequence(struct smbXcli_conn *conn,
+ bool v);
+
struct tevent_req *smbXcli_conn_samba_suicide_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
--
2.11.0
From 93b5702ce2e6d4add85d16d3caa0fd7c9a2d4196 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 11 Jan 2018 11:55:39 +0100
Subject: [PATCH 6/6] torture: Add test for channel sequence number handling
We run into an assert when the csn wraps
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13215
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source4/torture/smb2/replay.c | 97 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/source4/torture/smb2/replay.c b/source4/torture/smb2/replay.c
index 889ecd73c72..2ef40445aee 100644
--- a/source4/torture/smb2/replay.c
+++ b/source4/torture/smb2/replay.c
@@ -2425,6 +2425,102 @@ done:
return ret;
}
+static bool test_replay7(struct torture_context *tctx, struct smb2_tree *tree)
+{
+ TALLOC_CTX *mem_ctx = talloc_new(tctx);
+ struct smb2_transport *transport = tree->session->transport;
+ NTSTATUS status;
+ struct smb2_handle _dh;
+ struct smb2_handle *dh = NULL;
+ struct smb2_notify notify;
+ struct smb2_request *req;
+ union smb_fileinfo qfinfo;
+ bool ret = false;
+
+ if (smbXcli_conn_protocol(transport->conn) < PROTOCOL_SMB3_00) {
+ torture_skip(tctx, "SMB 3.X Dialect family required for "
+ "replay tests\n");
+ }
+
+ torture_comment(tctx, "Notify across increment/decrement of csn\n");
+
+ smbXcli_conn_set_force_channel_sequence(transport->conn, true);
+
+ status = torture_smb2_testdir(tree, BASEDIR, &_dh);
+ CHECK_STATUS(status, NT_STATUS_OK);
+ dh = &_dh;
+
+ notify.in.recursive = 0x0000;
+ notify.in.buffer_size = 0xffff;
+ notify.in.file.handle = _dh;
+ notify.in.completion_filter = FILE_NOTIFY_CHANGE_FILE_NAME;
+ notify.in.unknown = 0x00000000;
+
+ /*
+ * This posts a long-running request with csn==0 to "dh". Now
+ * op->request_count==1 in smb2_server.c.
+ */
+ smb2cli_session_reset_channel_sequence(tree->session->smbXcli, 0);
+ req = smb2_notify_send(tree, ¬ify);
+
+ qfinfo = (union smb_fileinfo) {
+ .generic.level = RAW_FILEINFO_POSITION_INFORMATION,
+ .generic.in.file.handle = _dh
+ };
+
+ /*
+ * This sequence of 2 dummy requests moves
+ * op->request_count==1 to op->pre_request_count. The numbers
+ * used avoid int16 overflow.
+ */
+
+ smb2cli_session_reset_channel_sequence(tree->session->smbXcli, 30000);
+ status = smb2_getinfo_file(tree, mem_ctx, &qfinfo);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ smb2cli_session_reset_channel_sequence(tree->session->smbXcli, 60000);
+ status = smb2_getinfo_file(tree, mem_ctx, &qfinfo);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ /*
+ * This final request turns the op->global->channel_sequence
+ * to the same as we had when sending the notify above. The
+ * notify's request count has in the meantime moved to
+ * op->pre_request_count.
+ */
+
+ smb2cli_session_reset_channel_sequence(tree->session->smbXcli, 0);
+ status = smb2_getinfo_file(tree, mem_ctx, &qfinfo);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ /*
+ * At this point op->request_count==0.
+ *
+ * The next cancel makes us reply to the notify. Because the
+ * csn we currently use is the same as we used when sending
+ * the notify, smbd thinks it must decrement op->request_count
+ * and not op->pre_request_count.
+ */
+
+ status = smb2_cancel(req);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ status = smb2_notify_recv(req, mem_ctx, ¬ify);
+ CHECK_STATUS(status, NT_STATUS_CANCELLED);
+
+ ret = true;
+
+done:
+ if (dh != NULL) {
+ smb2_util_close(tree, _dh);
+ }
+ smb2_deltree(tree, BASEDIR);
+ talloc_free(tree);
+ talloc_free(mem_ctx);
+
+ return ret;
+}
+
struct torture_suite *torture_smb2_replay_init(TALLOC_CTX *ctx)
{
struct torture_suite *suite =
@@ -2445,6 +2541,7 @@ struct torture_suite *torture_smb2_replay_init(TALLOC_CTX *ctx)
torture_suite_add_1smb2_test(suite, "replay4", test_replay4);
torture_suite_add_1smb2_test(suite, "replay5", test_replay5);
torture_suite_add_1smb2_test(suite, "replay6", test_replay6);
+ torture_suite_add_1smb2_test(suite, "replay7", test_replay7);
suite->description = talloc_strdup(suite, "SMB2 REPLAY tests");
--
2.11.0
More information about the samba-technical
mailing list