From 7440a1985ca9365a76dc69de45ae0628b783f964 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 11 Apr 2018 12:14:59 +0200 Subject: [PATCH 1/2] s3:smb2_server: correctly maintain request counters for compound requests If a session expires during a compound request chain, we exit smbd_smb2_request_dispatch() with 'return smbd_smb2_request_error(req, ...)' before calling smbd_smb2_request_dispatch_update_counts(). As req->request_counters_updated was only reset within smbd_smb2_request_dispatch_update_counts(), smbd_smb2_request_reply_update_counts() was called twice on the same request, which triggers SMB_ASSERT(op->request_count > 0); BUG: https://bugzilla.samba.org/show_bug.cgi?id=13215 Signed-off-by: Stefan Metzmacher Reviewed-by: Volker Lendecke --- source3/smbd/smb2_server.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index ee03a8eb0bb..177e5ffc2f2 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -2180,7 +2180,7 @@ static NTSTATUS smbd_smb2_request_dispatch_update_counts( bool update_open = false; NTSTATUS status = NT_STATUS_OK; - req->request_counters_updated = false; + SMB_ASSERT(!req->request_counters_updated); if (xconn->protocol < PROTOCOL_SMB2_22) { return NT_STATUS_OK; @@ -2315,6 +2315,8 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) DO_PROFILE_INC(request); + SMB_ASSERT(!req->request_counters_updated); + /* TODO: verify more things */ flags = IVAL(inhdr, SMB2_HDR_FLAGS); @@ -2755,6 +2757,8 @@ static void smbd_smb2_request_reply_update_counts(struct smbd_smb2_request *req) return; } + req->request_counters_updated = false; + if (xconn->protocol < PROTOCOL_SMB2_22) { return; } -- 2.11.0 From af2cd4a272c030d608bf6dfa8e4a04efaea2d796 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 11 Apr 2018 15:11:10 +0200 Subject: [PATCH 2/2] torture: Test compound request request counters This will send an unfixed smbd into the SMB_ASSERT(op->request_count > 0); in smbd_smb2_request_reply_update_counts BUG: https://bugzilla.samba.org/show_bug.cgi?id=13215 Signed-off-by: Volker Lendecke --- source4/torture/smb2/compound.c | 77 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c index c59230879b2..d2d4d7e41fc 100644 --- a/source4/torture/smb2/compound.c +++ b/source4/torture/smb2/compound.c @@ -1030,6 +1030,81 @@ done: return ret; } +static bool test_compound_invalid4(struct torture_context *tctx, + struct smb2_tree *tree) +{ + struct smb2_create cr; + struct smb2_read rd; + NTSTATUS status; + const char *fname = "compound_invalid4.dat"; + struct smb2_close cl; + bool ret = true; + bool ok; + struct smb2_request *req[2]; + + smb2_transport_credits_ask_num(tree->session->transport, 2); + + smb2_util_unlink(tree, fname); + + ZERO_STRUCT(cr); + cr.in.security_flags = 0x00; + cr.in.oplock_level = 0; + cr.in.impersonation_level = NTCREATEX_IMPERSONATION_IMPERSONATION; + cr.in.create_flags = 0x00000000; + cr.in.reserved = 0x00000000; + cr.in.desired_access = SEC_RIGHTS_FILE_ALL; + cr.in.file_attributes = FILE_ATTRIBUTE_NORMAL; + cr.in.share_access = NTCREATEX_SHARE_ACCESS_READ | + NTCREATEX_SHARE_ACCESS_WRITE | + NTCREATEX_SHARE_ACCESS_DELETE; + cr.in.create_disposition = NTCREATEX_DISP_OPEN_IF; + cr.in.create_options = NTCREATEX_OPTIONS_SEQUENTIAL_ONLY | + NTCREATEX_OPTIONS_ASYNC_ALERT | + NTCREATEX_OPTIONS_NON_DIRECTORY_FILE | + 0x00200000; + cr.in.fname = fname; + + status = smb2_create(tree, tctx, &cr); + CHECK_STATUS(status, NT_STATUS_OK); + + smb2_transport_compound_start(tree->session->transport, 2); + + ZERO_STRUCT(rd); + rd.in.file.handle = cr.out.file.handle; + rd.in.length = 1; + rd.in.offset = 0; + req[0] = smb2_read_send(tree, &rd); + + smb2_transport_compound_set_related(tree->session->transport, true); + + /* + * Send a completely bogus request as second compound + * element. This triggers smbd_smb2_request_error() in in + * smbd_smb2_request_dispatch() before calling + * smbd_smb2_request_dispatch_update_counts(). + */ + + req[1] = smb2_request_init_tree(tree, 0xff, 0x04, false, 0); + smb2_transport_send(req[1]); + + status = smb2_read_recv(req[0], tctx, &rd); + CHECK_STATUS(status, NT_STATUS_END_OF_FILE); + + ok = smb2_request_receive(req[1]); + torture_assert(tctx, ok, "Invalid request failed\n"); + CHECK_STATUS(req[1]->status, NT_STATUS_INVALID_PARAMETER); + + ZERO_STRUCT(cl); + cl.in.file.handle = cr.out.file.handle; + + status = smb2_close(tree, &cl); + CHECK_STATUS(status, NT_STATUS_OK); + + smb2_util_unlink(tree, fname); +done: + return ret; +} + /* Send a compound request where we expect the last request (Create, Notify) * to go asynchronous. This works against a Win7 server and the reply is * sent in two different packets. */ @@ -1297,6 +1372,8 @@ struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx) torture_suite_add_1smb2_test(suite, "invalid1", test_compound_invalid1); torture_suite_add_1smb2_test(suite, "invalid2", test_compound_invalid2); torture_suite_add_1smb2_test(suite, "invalid3", test_compound_invalid3); + torture_suite_add_1smb2_test( + suite, "invalid4", test_compound_invalid4); torture_suite_add_1smb2_test(suite, "interim1", test_compound_interim1); torture_suite_add_1smb2_test(suite, "interim2", test_compound_interim2); torture_suite_add_1smb2_test(suite, "compound-break", test_compound_break); -- 2.11.0