[PATCH] SMB2 Compound related chain handling when generation of FileId has failed
Anubhav Rakshit
anubhav.rakshit at gmail.com
Fri May 15 21:29:21 UTC 2020
Hi,
Please review the following patches.They consist of:
1. Smbtorture test case to verify the expected behaviour in case of
Create failure in a compound related chain.
2. Implement the behaviour in Samba Fileserver code.
I have attached the patches.
The changes are also staged in github.
https://github.com/anubhavrakshit/samba/commit/a0e7d6196b259038342569d371ff67ed30c9b6b8
https://github.com/anubhavrakshit/samba/commit/4637b6108f188c1a2df7cce94165b621294942a1
Thanks,
Anubhav
-------------- next part --------------
From a0e7d6196b259038342569d371ff67ed30c9b6b8 Mon Sep 17 00:00:00 2001
From: Anubhav Rakshit <anubhav.rakshit at gmail.com>
Date: Sat, 16 May 2020 00:02:18 +0530
Subject: [PATCH 1/2] Add couple of compound related test cases to verify that
server should return NTSTATUS of the failed Create for succeeding requests.
Signed-off-by: Anubhav Rakshit <anubhav.rakshit at gmail.com>
---
source4/torture/smb2/compound.c | 138 ++++++++++++++++++++++++++++++++
1 file changed, 138 insertions(+)
diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c
index fd657a4a16e..9ab370d6792 100644
--- a/source4/torture/smb2/compound.c
+++ b/source4/torture/smb2/compound.c
@@ -24,6 +24,8 @@
#include "libcli/smb2/smb2_calls.h"
#include "torture/torture.h"
#include "torture/smb2/proto.h"
+#include "libcli/security/security.h"
+#include "librpc/gen_ndr/ndr_security.h"
#include "../libcli/smb/smbXcli_base.h"
#define CHECK_STATUS(status, correct) do { \
@@ -441,6 +443,138 @@ done:
return ret;
}
+static bool test_compound_related4(struct torture_context *tctx,
+ struct smb2_tree *tree) {
+ struct smb2_handle hd;
+ struct smb2_create cr;
+ union smb_setfileinfo set;
+ struct smb2_ioctl io;
+ struct smb2_close cl;
+ struct security_descriptor *sd;
+ NTSTATUS status;
+ const char *fname = "compound_related4.dat";
+ struct smb2_request *req[4];
+ bool ret = true;
+
+ smb2_util_unlink(tree, fname);
+
+ ZERO_STRUCT(cr);
+ cr.level = RAW_OPEN_SMB2;
+ cr.in.create_flags = 0;
+ cr.in.desired_access =
+ SEC_STD_READ_CONTROL | SEC_STD_WRITE_DAC | SEC_STD_WRITE_OWNER;
+ cr.in.create_options = 0;
+ cr.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+ cr.in.share_access = NTCREATEX_SHARE_ACCESS_DELETE |
+ NTCREATEX_SHARE_ACCESS_READ |
+ NTCREATEX_SHARE_ACCESS_WRITE;
+ cr.in.alloc_size = 0;
+ cr.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
+ cr.in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS;
+ cr.in.security_flags = 0;
+ cr.in.fname = fname;
+
+ status = smb2_create(tree, tctx, &cr);
+ CHECK_STATUS(status, NT_STATUS_OK);
+ hd = cr.out.file.handle;
+ torture_comment(tctx, "set a sec desc allowing no write by CREATOR_OWNER\n");
+ sd = security_descriptor_dacl_create(tctx,
+ 0, NULL, NULL,
+ SID_CREATOR_OWNER,
+ SEC_ACE_TYPE_ACCESS_ALLOWED,
+ SEC_RIGHTS_FILE_READ | SEC_STD_ALL,
+ 0,
+ NULL);
+
+ set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
+ set.set_secdesc.in.file.handle = hd;
+ set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
+ set.set_secdesc.in.sd = sd;
+
+ status = smb2_setinfo_file(tree, &set);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ torture_comment(tctx, "try open for write\n");
+ cr.in.desired_access = SEC_FILE_WRITE_DATA;
+ smb2_transport_compound_start(tree->session->transport, 4);
+ req[0] = smb2_create_send(tree, &cr);
+ hd.data[0] = UINT64_MAX;
+ hd.data[1] = UINT64_MAX;
+
+ smb2_transport_compound_set_related(tree->session->transport, true);
+ ZERO_STRUCT(io);
+ io.in.function = FSCTL_CREATE_OR_GET_OBJECT_ID;
+ io.in.file.handle = hd;
+ io.in.flags = 1;
+
+ req[1] = smb2_ioctl_send(tree, &io);
+
+ ZERO_STRUCT(cl);
+ cl.in.file.handle = hd;
+
+ req[2] = smb2_close_send(tree, &cl);
+
+ set.set_secdesc.in.file.handle = hd;
+
+ req[3] = smb2_setinfo_file_send(tree, &set);
+
+ status = smb2_create_recv(req[0], tree, &cr);
+ CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
+ status = smb2_ioctl_recv(req[1], tree, &io);
+ CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
+ status = smb2_close_recv(req[2], &cl);
+ CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
+ status = smb2_setinfo_recv(req[3]);
+ CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
+done:
+ smb2_util_unlink(tree, fname);
+ smb2_tdis(tree);
+ smb2_logoff(tree->session);
+ return ret;
+}
+
+static bool test_compound_related5(struct torture_context *tctx,
+ struct smb2_tree *tree)
+{
+ struct smb2_handle hd;
+ struct smb2_ioctl io;
+ struct smb2_close cl;
+ struct smb2_request *req[2];
+ NTSTATUS status;
+ bool ret = false;
+
+ smb2_transport_compound_start(tree->session->transport, 2);
+
+ hd.data[0] = UINT64_MAX;
+ hd.data[1] = UINT64_MAX;
+
+
+ ZERO_STRUCT(io);
+ io.in.function = FSCTL_CREATE_OR_GET_OBJECT_ID;
+ io.in.file.handle = hd;
+ io.in.flags = 1;
+
+ req[0] = smb2_ioctl_send(tree, &io);
+
+ smb2_transport_compound_set_related(tree->session->transport, true);
+
+ ZERO_STRUCT(cl);
+ cl.in.file.handle = hd;
+
+ req[1] = smb2_close_send(tree, &cl);
+
+ status = smb2_ioctl_recv(req[0], tree, &io);
+ CHECK_STATUS(status, NT_STATUS_FILE_CLOSED);
+ status = smb2_close_recv(req[1], &cl);
+ CHECK_STATUS(status, NT_STATUS_FILE_CLOSED);
+
+ ret = true;
+done:
+ smb2_tdis(tree);
+ smb2_logoff(tree->session);
+ return ret;
+}
+
static bool test_compound_padding(struct torture_context *tctx,
struct smb2_tree *tree)
{
@@ -1436,6 +1570,10 @@ struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx)
torture_suite_add_1smb2_test(suite, "related2", test_compound_related2);
torture_suite_add_1smb2_test(suite, "related3",
test_compound_related3);
+ torture_suite_add_1smb2_test(suite, "related4",
+ test_compound_related4);
+ torture_suite_add_1smb2_test(suite, "related5",
+ test_compound_related5);
torture_suite_add_1smb2_test(suite, "unrelated1", test_compound_unrelated1);
torture_suite_add_1smb2_test(suite, "invalid1", test_compound_invalid1);
torture_suite_add_1smb2_test(suite, "invalid2", test_compound_invalid2);
--
2.20.1
From 4637b6108f188c1a2df7cce94165b621294942a1 Mon Sep 17 00:00:00 2001
From: Anubhav Rakshit <anubhav.rakshit at gmail.com>
Date: Sat, 16 May 2020 02:09:16 +0530
Subject: [PATCH 2/2] SMB2 Compound related chain handling when generation of
FileId has failed
Issue:
We have a scenario where an application sends a Compound Related chain
consisting of:
SMB2_CREATE
SMB2_IOCTL
SMB2_SET_INFO
SMB2_CLOSE
SMB2_CREATE failed with NT_STATUS_ACCESS_DENIED and subsequent
requests all fail. In Samba they return NT_STATUS_FILE_CLOSED.
When I tried the same against a Win2k12 server, I noticed that all the
failed requests of the chain would return NT_STATUS_ACCESS_DENIED.
I believe this behaviour is also mentioned in the [MS-SMB2] Specs
3.3.5.2.7.2: Handling Compounded Related Requests
"When the current operation requires a FileId and the previous
operation either contains or generates a FileId, if the previous
operation fails with an error, the server SHOULD<223> fail the current
operation with the same error code returned by the previous
operation."
Fix:
Save NTATUS of a failed Create request. When we process subsequent
requests of the chain we check if the previous Create has failed. In
case of a Create failure we returned the saved NTSTATUS.
Signed-off-by: Anubhav Rakshit <anubhav.rakshit at gmail.com>
---
source3/smbd/globals.h | 1 +
source3/smbd/smb2_create.c | 2 ++
source3/smbd/smb2_server.c | 4 ++++
3 files changed, 7 insertions(+)
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index 79086b3c81c..1cd52d69947 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -702,6 +702,7 @@ struct smbd_smb2_request {
bool do_encryption;
struct tevent_timer *async_te;
bool compound_related;
+ NTSTATUS compound_create_err;
/*
* Give the implementation of an SMB2 req a way to tell the SMB2 request
diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index f3fa6fba646..a971ae7dfea 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -332,6 +332,7 @@ static void smbd_smb2_request_create_done(struct tevent_req *tsubreq)
&out_context_blobs);
if (!NT_STATUS_IS_OK(status)) {
error = smbd_smb2_request_error(smb2req, status);
+ smb2req->compound_create_err = status;
if (!NT_STATUS_IS_OK(error)) {
smbd_server_connection_terminate(smb2req->xconn,
nt_errstr(error));
@@ -340,6 +341,7 @@ static void smbd_smb2_request_create_done(struct tevent_req *tsubreq)
return;
}
+ smb2req->compound_create_err = status;
status = smb2_create_blob_push(smb2req, &out_context_buffer, out_context_blobs);
if (!NT_STATUS_IS_OK(status)) {
error = smbd_smb2_request_error(smb2req, status);
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 718f0941532..234bdfb3ebc 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2622,6 +2622,10 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
fsp = file_fsp_smb2(req, file_id_persistent, file_id_volatile);
if (fsp == NULL) {
+ if (!NT_STATUS_IS_OK(req->compound_create_err)) {
+ return smbd_smb2_request_error(req,
+ req->compound_create_err);
+ }
if (!call->allow_invalid_fileid) {
return smbd_smb2_request_error(req,
NT_STATUS_FILE_CLOSED);
--
2.20.1
More information about the samba-technical
mailing list