From ded824a1570f676dcacd19761c956114291e939f Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 10 Jan 2018 14:29:01 +0100 Subject: [PATCH 1/6] smbd: Fix a typo Signed-off-by: Volker Lendecke Reviewed-by: Stefan Metzmacher (cherry picked from commit e8636e7ab75f89e89ef054b5d4aa6c07fddcbe2a) --- 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 de2f922c..900029d 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -2203,7 +2203,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 -- 1.9.1 From 31f608f9d5a64a139de8888924462bd593077675 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 10 Jan 2018 15:51:56 +0100 Subject: [PATCH 2/6] torture4: Fix typos Signed-off-by: Volker Lendecke Reviewed-by: Stefan Metzmacher (cherry picked from commit 71cee27962cba53da3249bd3f5ece32a1d10071d) --- 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 26b5583..5c8a372 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) { @@ -2164,7 +2164,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) { -- 1.9.1 From 77b4102f61364ef66e2a663f95690853ea569367 Mon Sep 17 00:00:00 2001 From: Volker Lendecke 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 Reviewed-by: Stefan Metzmacher (cherry picked from commit 03f65a7cdc91091a171269cfebc9916f2f678388) --- 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 900029d..e3e5199 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -2214,11 +2214,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; @@ -2228,10 +2228,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; -- 1.9.1 From 0f870b340fec7d7ad6e0cc4878ee7d20430de43c Mon Sep 17 00:00:00 2001 From: Volker Lendecke 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 Signed-off-by: Volker Lendecke Signed-off-by: Stefan Metzmacher (cherry picked from commit 0b57434151a8334a6e9b9b7542824ce4915421a2) --- 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 1bfa51e..d3f8d30 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 d3b9800..efcf3e9 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -733,6 +733,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 e3e5199..573f5f6 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -2141,6 +2141,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; @@ -2167,6 +2168,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) { /* @@ -2222,6 +2231,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) { @@ -2235,12 +2245,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); @@ -2726,7 +2738,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 { -- 1.9.1 From 7bfc9d910d85c2ee11f6a79d830719c84c6caad6 Mon Sep 17 00:00:00 2001 From: Volker Lendecke 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 Reviewed-by: Stefan Metzmacher (cherry picked from commit cd288a08500b1cc38ef26e5cb8ef754b4da658b6) --- 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 239e5eb..d1e532d 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; @@ -532,6 +534,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; @@ -2896,7 +2909,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 2594f07..336b1cbd 100644 --- a/libcli/smb/smbXcli_base.h +++ b/libcli/smb/smbXcli_base.h @@ -58,6 +58,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, -- 1.9.1 From b272eede7abc47ac37a804d82067662e4f00c5a7 Mon Sep 17 00:00:00 2001 From: Volker Lendecke 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 Reviewed-by: Stefan Metzmacher Autobuild-User(master): Stefan Metzmacher Autobuild-Date(master): Sun Jan 14 14:47:15 CET 2018 on sn-devel-144 (cherry picked from commit 0abe16a5343de9a69bb5cccbad9809b28b642f45) --- 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 5c8a372..109c04f 100644 --- a/source4/torture/smb2/replay.c +++ b/source4/torture/smb2/replay.c @@ -2427,6 +2427,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(void) { struct torture_suite *suite = @@ -2447,6 +2543,7 @@ struct torture_suite *torture_smb2_replay_init(void) 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"); -- 1.9.1