[PATCH] SMB2 Compound related chain handling when generation of FileId has failed
Jeremy Allison
jra at samba.org
Mon May 18 22:31:34 UTC 2020
On Mon, May 18, 2020 at 06:06:14PM +0200, Ralph Boehme via samba-technical wrote:
> Am 5/18/20 um 6:00 PM schrieb anubhav rakshit:
> >
> >
> > On Mon, May 18, 2020 at 8:37 PM Ralph Boehme <slow at samba.org
> > <mailto:slow at samba.org>> wrote:
> >
> > Am 5/18/20 um 5:02 PM schrieb anubhav rakshit:
> > > Hi Ralph,
> > > I am attaching additional test case that would verify
> > > Create(RO)->Read->Write->Read->Close chain. As expected we see Write
> > > failing with ACCESS DENIED.
> >
> > yes, that's as expected. WHat about the read?
> >
> > Both the Reads succeed.
>
> cool, slow: 1, metze: 0: :)))
>
> > Also, CI fails with the new code:
> >
> > https://gitlab.com/samba-team/devel/samba/-/jobs/557045164
> >
> > I have no clue to offer atm why this fails in an unrelated test. Sorry!
> >
> > Let me triage the failure.
Ah, I think I've found the problem.
In smbd_smb2_request_dispatch() when
file_fsp_smb2() fails to find a file
handle you're unconditionally checking
and returning req->compound_create_err
if it's not NULL. You should only check
and return that if it's in a compound
request (req->compound_related == true).
I have an updated patchset that fixes
this (attached). I'm running in gitlab-ci
now.
Before we proceed, have you filled in
the Samba copyright policy form here:
https://www.samba.org/samba/devel/copyright-policy.html
If not, email it in - that way we know we've
got the rights to integrate the code !
Cheers,
Jeremy.
-------------- next part --------------
From cd94878c8bcca38012d4d27099c032e923ef779e 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.
We already pass samba3.smb2.compound.related5, but mark related4 as knownfail.
Signed-off-by: Anubhav Rakshit <anubhav.rakshit at gmail.com>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail | 1 +
source4/torture/smb2/compound.c | 168 ++++++++++++++++++++++++++++++++
2 files changed, 169 insertions(+)
diff --git a/selftest/knownfail b/selftest/knownfail
index 38e8597deda..10dfd3850cf 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -211,6 +211,7 @@
^samba3.smb2.session.*reauth5 # some special anonymous checks?
^samba3.smb2.compound.interim2 # wrong return code (STATUS_CANCELLED)
^samba3.smb2.compound.aio.interim2 # wrong return code (STATUS_CANCELLED)
+^samba3.smb2.compound.related4
^samba3.smb2.replay.channel-sequence
^samba3.smb2.replay.replay3
^samba3.smb2.replay.replay4
diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c
index fd657a4a16e..ad1c94df04c 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,168 @@ 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);
+ if (sd == NULL) {
+ ret = false;
+ goto done;
+ }
+
+ 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);
+ if (req[0] == NULL) {
+ ret = false;
+ goto done;
+ }
+ 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);
+ if (req[1] == NULL) {
+ ret = false;
+ goto done;
+ }
+
+ ZERO_STRUCT(cl);
+ cl.in.file.handle = hd;
+
+ req[2] = smb2_close_send(tree, &cl);
+ if (req[2] == NULL) {
+ ret = false;
+ goto done;
+ }
+
+ set.set_secdesc.in.file.handle = hd;
+
+ req[3] = smb2_setinfo_file_send(tree, &set);
+ if (req[3] == NULL) {
+ ret = false;
+ goto done;
+ }
+
+ 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);
+ if (req[0] == NULL) {
+ goto done;
+ }
+
+ smb2_transport_compound_set_related(tree->session->transport, true);
+
+ ZERO_STRUCT(cl);
+ cl.in.file.handle = hd;
+
+ req[1] = smb2_close_send(tree, &cl);
+ if (req[1] == NULL) {
+ goto done;
+ }
+
+ 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 +1600,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 5bb03576d527b0c774ed3a14b57ee88c290baaec 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>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail | 1 -
source3/smbd/globals.h | 1 +
source3/smbd/smb2_create.c | 2 ++
source3/smbd/smb2_server.c | 5 +++++
4 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 10dfd3850cf..38e8597deda 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -211,7 +211,6 @@
^samba3.smb2.session.*reauth5 # some special anonymous checks?
^samba3.smb2.compound.interim2 # wrong return code (STATUS_CANCELLED)
^samba3.smb2.compound.aio.interim2 # wrong return code (STATUS_CANCELLED)
-^samba3.smb2.compound.related4
^samba3.smb2.replay.channel-sequence
^samba3.smb2.replay.replay3
^samba3.smb2.replay.replay4
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index d3b6ac2ffe6..76eb4a9fa87 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -708,6 +708,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 c11cc66a710..dd370b7e047 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2792,6 +2792,11 @@ 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 &&
+ !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