[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