[RFC] SMB2 Compound related chain handling when generation of FileId has failed
Anubhav Rakshit
anubhav.rakshit at gmail.com
Wed May 13 16:31:38 UTC 2020
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.“
I have written a smbtorture test case to verify the above. I also
propose the following fix. I am attaching code for your reference.
Kindly let me know if the changes look reasonable.
+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.unknown2 = 0;
+ io.in.max_response_size = 64;
+ 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_padding(struct torture_context *tctx,
struct smb2_tree *tree)
{
@@ -1111,6 +1206,8 @@ struct torture_suite *torture_smb2_compound_init(void)
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, "unrelated1",
test_compound_unrelated1);
torture_suite_add_1smb2_test(suite, "invalid1", test_compound_invalid1);
torture_suite_add_1smb2_test(suite, "invalid2", test_compound_invalid2);
diff --git source3/smbd/globals.h source3/smbd/globals.h
index e9bcf45..b85f41a 100644
--- source3/smbd/globals.h
+++ source3/smbd/globals.h
@@ -776,6 +776,8 @@ struct smbd_smb2_request {
/* Should we encrypt? */
bool do_encryption;
bool compound_related;
+ bool create_fail;
+ NTSTATUS compound_create_err;
bool retry_operation;
bool interim_response_sent;
bool has_twrp;
diff --git source3/smbd/smb2_create.c source3/smbd/smb2_create.c
index 0a93dd6..02b1ef6 100644
--- source3/smbd/smb2_create.c
+++ source3/smbd/smb2_create.c
@@ -365,6 +365,8 @@ 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->create_fail = true;
+ smb2req->compound_create_err = status;
if (!NT_STATUS_IS_OK(error)) {
smbd_server_connection_terminate(smb2req->xconn,
nt_errstr(error));
@@ -372,7 +374,8 @@ static void smbd_smb2_request_create_done(struct
tevent_req *tsubreq)
}
return;
}
-
+ smb2req->create_fail = false;
+ 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 source3/smbd/smb2_server.c source3/smbd/smb2_server.c
index 5d9de30..0f771e9 100644
--- source3/smbd/smb2_server.c
+++ source3/smbd/smb2_server.c
@@ -2412,6 +2412,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 (req->compound_related && req->create_fail) {
+ 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);
Thanks,
Anubhav
-------------- next part --------------
A non-text attachment was scrubbed...
Name: compound_create.diff
Type: application/octet-stream
Size: 5975 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20200513/43ea2d90/compound_create.obj>
More information about the samba-technical
mailing list