From ac5336c2865b26df01c6e4f52453f71ce6e22965 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 1 Aug 2023 13:00:54 +0200 Subject: [PATCH 1/3] smbd: reuse "truncating" variable in open_file() Signed-off-by: Ralph Boehme --- source3/smbd/open.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 87719eec06e6..d5d8ff1285c2 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -1390,7 +1390,7 @@ static NTSTATUS open_file(struct smb_request *req, * as we always opened files read-write in that release. JRA. */ - if ((accmode == O_RDONLY) && ((flags & O_TRUNC) == O_TRUNC)) { + if ((accmode == O_RDONLY) && truncating) { DEBUG(10,("open_file: truncate requested on read-only open " "for file %s\n", smb_fname_str_dbg(smb_fname))); local_flags = (flags & ~O_ACCMODE)|O_RDWR; -- 2.41.0 From 8c342637deeed2965521dd612670890f90f7a42b Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 1 Aug 2023 12:30:00 +0200 Subject: [PATCH 2/3] CVE-XXX: smbtorture: test overwrite dispositions on read-only file BUG: https://bugzilla.samba.org/show_bug.cgi?id=15439 --- selftest/knownfail.d/samba3.smb2.acls | 1 + source4/torture/smb2/acls.c | 143 ++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 selftest/knownfail.d/samba3.smb2.acls diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls new file mode 100644 index 000000000000..18df260c0e50 --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2.acls @@ -0,0 +1 @@ +^samba3.smb2.acls.OVERWRITE_READ_ONLY_FILE diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c index 2f15ad27e48b..017bdf294875 100644 --- a/source4/torture/smb2/acls.c +++ b/source4/torture/smb2/acls.c @@ -3023,6 +3023,148 @@ static bool test_mxac_not_granted(struct torture_context *tctx, return ret; } +static bool test_overwrite_read_only_file(struct torture_context *tctx, + struct smb2_tree *tree) +{ + NTSTATUS status; + struct smb2_create c; + const char *fname = BASEDIR "\\test_overwrite_read_only_file.txt"; + struct smb2_handle handle = {{0}}; + union smb_fileinfo q; + union smb_setfileinfo set; + struct security_descriptor *sd = NULL, *sd_orig = NULL; + const char *owner_sid = NULL; + int i; + bool ret = true; + + struct tcase { + int disposition; + const char *disposition_string; + NTSTATUS expected_status; + } tcases[] = { +#define TCASE(d, s) { \ + .disposition = d, \ + .disposition_string = #d, \ + .expected_status = s, \ + } + TCASE(NTCREATEX_DISP_OPEN, NT_STATUS_OK), + TCASE(NTCREATEX_DISP_SUPERSEDE, NT_STATUS_ACCESS_DENIED), + TCASE(NTCREATEX_DISP_OVERWRITE, NT_STATUS_ACCESS_DENIED), + TCASE(NTCREATEX_DISP_OVERWRITE_IF, NT_STATUS_ACCESS_DENIED), + }; +#undef TCASE + + ret = smb2_util_setup_dir(tctx, tree, BASEDIR); + torture_assert_goto(tctx, ret, ret, done, "smb2_util_setup_dir not ok"); + + c = (struct smb2_create) { + .in.desired_access = SEC_STD_READ_CONTROL | + SEC_STD_WRITE_DAC | + SEC_STD_WRITE_OWNER, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_READ | + NTCREATEX_SHARE_ACCESS_WRITE, + .in.create_disposition = NTCREATEX_DISP_OPEN_IF, + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, tctx, &c); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_create failed\n"); + handle = c.out.file.handle; + + torture_comment(tctx, "get the original sd\n"); + + ZERO_STRUCT(q); + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC; + q.query_secdesc.in.file.handle = handle; + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER; + + status = smb2_getinfo_file(tree, tctx, &q); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_getinfo_file failed\n"); + sd_orig = q.query_secdesc.out.sd; + + owner_sid = dom_sid_string(tctx, sd_orig->owner_sid); + + sd = security_descriptor_dacl_create(tctx, + 0, NULL, NULL, + owner_sid, + SEC_ACE_TYPE_ACCESS_ALLOWED, + SEC_FILE_READ_DATA, + 0, + NULL); + + ZERO_STRUCT(set); + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; + set.set_secdesc.in.file.handle = handle; + set.set_secdesc.in.secinfo_flags = SECINFO_DACL; + set.set_secdesc.in.sd = sd; + + status = smb2_setinfo_file(tree, &set); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_setinfo_file failed\n"); + + smb2_util_close(tree, handle); + ZERO_STRUCT(handle); + + for (i = 0; i < ARRAY_SIZE(tcases); i++) { + torture_comment(tctx, "Verify open with %s dispostion\n", + tcases[i].disposition_string); + + c = (struct smb2_create) { + .in.create_disposition = tcases[i].disposition, + .in.desired_access = SEC_FILE_READ_DATA, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, tctx, &c); + smb2_util_close(tree, c.out.file.handle); + torture_assert_ntstatus_equal_goto( + tctx, status, tcases[i].expected_status, ret, done, + "smb2_create failed\n"); + }; + + torture_comment(tctx, "put back original sd\n"); + + c = (struct smb2_create) { + .in.desired_access = SEC_STD_WRITE_DAC, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_MASK, + .in.create_disposition = NTCREATEX_DISP_OPEN_IF, + .in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, tctx, &c); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_create failed\n"); + handle = c.out.file.handle; + + ZERO_STRUCT(set); + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; + set.set_secdesc.in.file.handle = handle; + set.set_secdesc.in.secinfo_flags = SECINFO_DACL; + set.set_secdesc.in.sd = sd_orig; + + status = smb2_setinfo_file(tree, &set); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_setinfo_file failed\n"); + + smb2_util_close(tree, handle); + ZERO_STRUCT(handle); + +done: + smb2_util_close(tree, handle); + smb2_util_unlink(tree, fname); + smb2_deltree(tree, BASEDIR); + return ret; +} + /* basic testing of SMB2 ACLs */ @@ -3051,6 +3193,7 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx) test_deny1); torture_suite_add_1smb2_test(suite, "MXAC-NOT-GRANTED", test_mxac_not_granted); + torture_suite_add_1smb2_test(suite, "OVERWRITE_READ_ONLY_FILE", test_overwrite_read_only_file); suite->description = talloc_strdup(suite, "SMB2-ACLS tests"); -- 2.41.0 From b9126b44a94dc7976597b54aa73e427c11b89ef5 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 1 Aug 2023 13:04:36 +0200 Subject: [PATCH 3/3] CVE-XXX: smbd: use open_access_mask for access check in open_file() If the client requested FILE_OVERWRITE[_IF], we're implicitly adding FILE_WRITE_DATA to the open_access_mask in open_file_ntcreate(), but for the access check we're using access_mask which doesn't contain the additional right, which means we can end up truncating a file for which the user has only read-only access via an SD. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15439 --- selftest/knownfail.d/samba3.smb2.acls | 1 - source3/smbd/open.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.smb2.acls diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls deleted file mode 100644 index 18df260c0e50..000000000000 --- a/selftest/knownfail.d/samba3.smb2.acls +++ /dev/null @@ -1 +0,0 @@ -^samba3.smb2.acls.OVERWRITE_READ_ONLY_FILE diff --git a/source3/smbd/open.c b/source3/smbd/open.c index d5d8ff1285c2..844a25f8058c 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -1442,7 +1442,7 @@ static NTSTATUS open_file(struct smb_request *req, dirfsp, fsp, false, - access_mask); + open_access_mask); if (!NT_STATUS_IS_OK(status)) { DBG_DEBUG("smbd_check_access_rights_fsp" @@ -1633,7 +1633,7 @@ static NTSTATUS open_file(struct smb_request *req, status = smbd_check_access_rights_fsp(dirfsp, fsp, false, - access_mask); + open_access_mask); if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND) && posix_open && -- 2.41.0