From e5a1c1cfb0a73a37001afee530ae09bf5c58b515 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 25 Jul 2023 17:41:04 -0700 Subject: [PATCH 01/30] CVE-2023-3961:s3:smbd: Catch any incoming pipe path that could exit socket_dir. For now, SMB_ASSERT() to exit the server. We will remove this once the test code is in place. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15422 Signed-off-by: Jeremy Allison --- source3/rpc_client/local_np.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/source3/rpc_client/local_np.c b/source3/rpc_client/local_np.c index 0b323404f06..95228d5d801 100644 --- a/source3/rpc_client/local_np.c +++ b/source3/rpc_client/local_np.c @@ -542,6 +542,24 @@ struct tevent_req *local_np_connect_send( return tevent_req_post(req, ev); } + /* + * Ensure we cannot process a path that exits + * the socket_dir. + */ + if (ISDOTDOT(lower_case_pipename) || + (strchr(lower_case_pipename, '/')!=NULL)) + { + DBG_DEBUG("attempt to connect to invalid pipe pathname %s\n", + lower_case_pipename); + /* + * For now, panic the server until we have + * the test code in place. + */ + SMB_ASSERT(false); + tevent_req_error(req, ENOENT); + return tevent_req_post(req, ev); + } + state->socketpath = talloc_asprintf( state, "%s/np/%s", socket_dir, lower_case_pipename); if (tevent_req_nomem(state->socketpath, req)) { -- 2.34.1 From 125ce23115b92045a1584f5654669180bea83067 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 25 Jul 2023 17:49:21 -0700 Subject: [PATCH 02/30] CVE-2023-3961:s3:torture: Add test SMB2-INVALID-PIPENAME to show we allow bad pipenames with unix separators through to the UNIX domain socket code. The raw SMB2-INVALID-PIPENAME test passes against Windows 2022, as it just returns NT_STATUS_OBJECT_NAME_NOT_FOUND. Add the knownfail. BUG:https://bugzilla.samba.org/show_bug.cgi?id=15422 Signed-off-by: Jeremy Allison [abartlet@samba.org backported to Samba 4.17 due to conflicts from context of other new torture tests missing in this version and changes in smb2cli_create() arguments] --- selftest/knownfail.d/badpipename | 1 + source3/selftest/tests.py | 15 +++++ source3/torture/proto.h | 1 + source3/torture/test_smb2.c | 105 +++++++++++++++++++++++++++++++ source3/torture/torture.c | 4 ++ 5 files changed, 126 insertions(+) create mode 100644 selftest/knownfail.d/badpipename diff --git a/selftest/knownfail.d/badpipename b/selftest/knownfail.d/badpipename new file mode 100644 index 00000000000..e69715f863d --- /dev/null +++ b/selftest/knownfail.d/badpipename @@ -0,0 +1 @@ +^samba3.smbtorture_s3.smb2.SMB2-INVALID-PIPENAME.smbtorture\(fileserver\) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 831fdd6db2e..e93365e3db5 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -263,6 +263,21 @@ plantestsuite("samba3.smbtorture_s3.smb1.MSDFS-ATTRIBUTE", "-mNT1", "-f msdfs-src1"]) +# +# BUG: https://bugzilla.samba.org/show_bug.cgi?id=15422 +# Prevent bad pipenames. +# +plantestsuite("samba3.smbtorture_s3.smb2.SMB2-INVALID-PIPENAME", + "fileserver", + [os.path.join(samba3srcdir, + "script/tests/test_smbtorture_s3.sh"), + 'SMB2-INVALID-PIPENAME', + '//$SERVER_IP/tmp', + '$USERNAME', + '$PASSWORD', + smbtorture3, + "-mSMB2"]) + # # SMB2-STREAM-ACL needs to run against a special share - vfs_wo_fruit # diff --git a/source3/torture/proto.h b/source3/torture/proto.h index 4fa2fbd12a1..6c60e80a95e 100644 --- a/source3/torture/proto.h +++ b/source3/torture/proto.h @@ -120,6 +120,7 @@ bool run_smb2_path_slash(int dummy); bool run_smb2_sacl(int dummy); bool run_smb2_quota1(int dummy); bool run_smb2_stream_acl(int dummy); +bool run_smb2_invalid_pipename(int dummy); bool run_list_dir_async_test(int dummy); bool run_delete_on_close_non_empty(int dummy); bool run_delete_on_close_nonwrite_delete_yes_test(int dummy); diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c index c3f014100d9..f6afdf0b553 100644 --- a/source3/torture/test_smb2.c +++ b/source3/torture/test_smb2.c @@ -3608,3 +3608,108 @@ bool run_delete_on_close_nonwrite_delete_no_test(int dummy) } return ret; } + +bool run_smb2_invalid_pipename(int dummy) +{ + struct cli_state *cli = NULL; + NTSTATUS status; + uint64_t fid_persistent = 0; + uint64_t fid_volatile = 0; + const char *unknown_pipe = "badpipe"; + const char *invalid_pipe = "../../../../../../../../../badpipe"; + + printf("Starting SMB2-INVALID-PIPENAME\n"); + + if (!torture_init_connection(&cli)) { + return false; + } + + status = smbXcli_negprot(cli->conn, + cli->timeout, + PROTOCOL_SMB2_02, + PROTOCOL_SMB3_11); + if (!NT_STATUS_IS_OK(status)) { + printf("smbXcli_negprot returned %s\n", nt_errstr(status)); + return false; + } + + status = cli_session_setup_creds(cli, torture_creds); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_session_setup returned %s\n", nt_errstr(status)); + return false; + } + + status = cli_tree_connect(cli, "IPC$", "?????", NULL); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_tree_connect returned %s\n", nt_errstr(status)); + return false; + } + + /* Try and connect to an unknown pipename. */ + status = smb2cli_create(cli->conn, + cli->timeout, + cli->smb2.session, + cli->smb2.tcon, + unknown_pipe, + SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */ + SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */ + SEC_STD_SYNCHRONIZE| + SEC_FILE_READ_DATA| + SEC_FILE_WRITE_DATA| + SEC_FILE_READ_ATTRIBUTE, /* desired_access, */ + FILE_ATTRIBUTE_NORMAL, /* file_attributes, */ + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */ + FILE_CREATE, /* create_disposition, */ + 0, /* create_options, */ + NULL, /* smb2_create_blobs *blobs */ + &fid_persistent, + &fid_volatile, + NULL, /* struct smb_create_returns * */ + talloc_tos(), /* mem_ctx. */ + NULL); /* struct smb2_create_blobs */ + /* We should get NT_STATUS_OBJECT_NAME_NOT_FOUND */ + if (!NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) { + printf("%s:%d smb2cli_create on name %s returned %s\n", + __FILE__, + __LINE__, + unknown_pipe, + nt_errstr(status)); + return false; + } + + /* Try and connect to an invalid pipename containing unix separators. */ + status = smb2cli_create(cli->conn, + cli->timeout, + cli->smb2.session, + cli->smb2.tcon, + invalid_pipe, + SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */ + SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */ + SEC_STD_SYNCHRONIZE| + SEC_FILE_READ_DATA| + SEC_FILE_WRITE_DATA| + SEC_FILE_READ_ATTRIBUTE, /* desired_access, */ + FILE_ATTRIBUTE_NORMAL, /* file_attributes, */ + FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */ + FILE_CREATE, /* create_disposition, */ + 0, /* create_options, */ + NULL, /* smb2_create_blobs *blobs */ + &fid_persistent, + &fid_volatile, + NULL, /* struct smb_create_returns * */ + talloc_tos(), /* mem_ctx. */ + NULL); /* struct smb2_create_blobs */ + /* + * We should still get NT_STATUS_OBJECT_NAME_NOT_FOUND + * (tested against Windows 2022). + */ + if (!NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) { + printf("%s:%d smb2cli_create on name %s returned %s\n", + __FILE__, + __LINE__, + invalid_pipe, + nt_errstr(status)); + return false; + } + return true; +} diff --git a/source3/torture/torture.c b/source3/torture/torture.c index 4b22958c838..6dd37148137 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -15763,6 +15763,10 @@ static struct { .name = "OPLOCK-CANCEL", .fn = run_oplock_cancel, }, + { + .name = "SMB2-INVALID-PIPENAME", + .fn = run_smb2_invalid_pipename, + }, { .name = "SMB1-TRUNCATED-SESSSETUP", .fn = run_smb1_truncated_sesssetup, -- 2.34.1 From 4b3e5c2f036f868e38ad5da7faba05db32f624f4 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 25 Jul 2023 17:54:41 -0700 Subject: [PATCH 03/30] CVE-2023-3961:s3: smbd: Remove the SMB_ASSERT() that crashes on bad pipenames. We correctly handle this and just return ENOENT (NT_STATUS_OBJECT_NAME_NOT_FOUND). Remove knowfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15422 Signed-off-by: Jeremy Allison --- selftest/knownfail.d/badpipename | 1 - source3/rpc_client/local_np.c | 5 ----- 2 files changed, 6 deletions(-) delete mode 100644 selftest/knownfail.d/badpipename diff --git a/selftest/knownfail.d/badpipename b/selftest/knownfail.d/badpipename deleted file mode 100644 index e69715f863d..00000000000 --- a/selftest/knownfail.d/badpipename +++ /dev/null @@ -1 +0,0 @@ -^samba3.smbtorture_s3.smb2.SMB2-INVALID-PIPENAME.smbtorture\(fileserver\) diff --git a/source3/rpc_client/local_np.c b/source3/rpc_client/local_np.c index 95228d5d801..791ded99a47 100644 --- a/source3/rpc_client/local_np.c +++ b/source3/rpc_client/local_np.c @@ -551,11 +551,6 @@ struct tevent_req *local_np_connect_send( { DBG_DEBUG("attempt to connect to invalid pipe pathname %s\n", lower_case_pipename); - /* - * For now, panic the server until we have - * the test code in place. - */ - SMB_ASSERT(false); tevent_req_error(req, ENOENT); return tevent_req_post(req, ev); } -- 2.34.1 From b08a60160e6ab8d982d31844bcbf7ab67ff3a8de Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 1 Aug 2023 12:30:00 +0200 Subject: [PATCH 04/30] CVE-2023-4091: smbtorture: test overwrite dispositions on read-only file BUG: https://bugzilla.samba.org/show_bug.cgi?id=15439 Signed-off-by: Ralph Boehme --- 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 00000000000..18df260c0e5 --- /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 a27d4e079e6..5a892d004ea 100644 --- a/source4/torture/smb2/acls.c +++ b/source4/torture/smb2/acls.c @@ -2989,6 +2989,148 @@ done: 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 */ @@ -3017,6 +3159,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.34.1 From 8b26f634372f11edcbea33dfd68a3d57889dfcc5 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 1 Aug 2023 13:04:36 +0200 Subject: [PATCH 05/30] CVE-2023-4091: 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 Signed-off-by: Ralph Boehme --- 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 18df260c0e5..00000000000 --- 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 dbf4e40adf4..704c1d7e312 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.34.1 From 8c0be1d17a5f4e700fe38b5f58d1caa359e8c383 Mon Sep 17 00:00:00 2001 From: Christian Merten Date: Mon, 19 Sep 2022 22:47:10 +0200 Subject: [PATCH 06/30] CVE-2023-4154 libcli security_descriptor: Add function to delete a given ace from a security descriptor Two functions have been added to delete a given ace from the SACL or the DACL of a security descriptor. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Christian Merten Reviewed-by: Douglas Bagnall Reviewed-by: Jeremy Allison (cherry picked from commit 7efe673fbdcd27ddd23f36281c5f5338681a68fe) --- libcli/security/security_descriptor.c | 66 +++++++++++++++++++++++++++ libcli/security/security_descriptor.h | 4 ++ 2 files changed, 70 insertions(+) diff --git a/libcli/security/security_descriptor.c b/libcli/security/security_descriptor.c index ba142016389..64c2d027876 100644 --- a/libcli/security/security_descriptor.c +++ b/libcli/security/security_descriptor.c @@ -419,6 +419,72 @@ NTSTATUS security_descriptor_sacl_del(struct security_descriptor *sd, return security_descriptor_acl_del(sd, true, trustee); } +/* + delete the given ACE in the SACL or DACL of a security_descriptor +*/ +static NTSTATUS security_descriptor_acl_del_ace(struct security_descriptor *sd, + bool sacl_del, + const struct security_ace *ace) +{ + uint32_t i; + bool found = false; + struct security_acl *acl = NULL; + + if (sacl_del) { + acl = sd->sacl; + } else { + acl = sd->dacl; + } + + if (acl == NULL) { + return NT_STATUS_OBJECT_NAME_NOT_FOUND; + } + + for (i=0;inum_aces;i++) { + if (security_ace_equal(ace, &acl->aces[i])) { + ARRAY_DEL_ELEMENT(acl->aces, i, acl->num_aces); + acl->num_aces--; + if (acl->num_aces == 0) { + acl->aces = NULL; + } + found = true; + i--; + } + } + + if (!found) { + return NT_STATUS_OBJECT_NAME_NOT_FOUND; + } + + acl->revision = SECURITY_ACL_REVISION_NT4; + + for (i=0;inum_aces;i++) { + switch (acl->aces[i].type) { + case SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT: + case SEC_ACE_TYPE_ACCESS_DENIED_OBJECT: + case SEC_ACE_TYPE_SYSTEM_AUDIT_OBJECT: + case SEC_ACE_TYPE_SYSTEM_ALARM_OBJECT: + acl->revision = SECURITY_ACL_REVISION_ADS; + return NT_STATUS_OK; + default: + break; /* only for the switch statement */ + } + } + + return NT_STATUS_OK; +} + +NTSTATUS security_descriptor_dacl_del_ace(struct security_descriptor *sd, + const struct security_ace *ace) +{ + return security_descriptor_acl_del_ace(sd, false, ace); +} + +NTSTATUS security_descriptor_sacl_del_ace(struct security_descriptor *sd, + const struct security_ace *ace) +{ + return security_descriptor_acl_del_ace(sd, true, ace); +} /* compare two security ace structures */ diff --git a/libcli/security/security_descriptor.h b/libcli/security/security_descriptor.h index 7e6df87fefa..46545321d15 100644 --- a/libcli/security/security_descriptor.h +++ b/libcli/security/security_descriptor.h @@ -39,6 +39,10 @@ NTSTATUS security_descriptor_dacl_del(struct security_descriptor *sd, const struct dom_sid *trustee); NTSTATUS security_descriptor_sacl_del(struct security_descriptor *sd, const struct dom_sid *trustee); +NTSTATUS security_descriptor_dacl_del_ace(struct security_descriptor *sd, + const struct security_ace *ace); +NTSTATUS security_descriptor_sacl_del_ace(struct security_descriptor *sd, + const struct security_ace *ace); bool security_ace_equal(const struct security_ace *ace1, const struct security_ace *ace2); bool security_acl_equal(const struct security_acl *acl1, -- 2.34.1 From d7034c4194a2cec0a88870ea3c7709d2a323653a Mon Sep 17 00:00:00 2001 From: Christian Merten Date: Mon, 19 Sep 2022 23:01:34 +0200 Subject: [PATCH 07/30] CVE-2023-4154 librpc ndr/py_security: Export ACE deletion functions to python Exported security_descriptor_sacl_del and security_descriptor_dacl_del as new methods of the security descriptor class to python. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Christian Merten Reviewed-by: Douglas Bagnall Reviewed-by: Jeremy Allison (cherry picked from commit 84a54d2fa2b1590fdb4e2ea986ded9c39a82cf78) --- source4/librpc/ndr/py_security.c | 52 +++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/source4/librpc/ndr/py_security.c b/source4/librpc/ndr/py_security.c index e79e7170812..e61b994d7cb 100644 --- a/source4/librpc/ndr/py_security.c +++ b/source4/librpc/ndr/py_security.c @@ -234,6 +234,52 @@ static PyObject *py_descriptor_sacl_del(PyObject *self, PyObject *args) Py_RETURN_NONE; } +static PyObject *py_descriptor_dacl_del_ace(PyObject *self, PyObject *args) +{ + struct security_descriptor *desc = pytalloc_get_ptr(self); + NTSTATUS status; + struct security_ace *ace = NULL; + PyObject *py_ace = Py_None; + + if (!PyArg_ParseTuple(args, "O!", &security_ace_Type, &py_ace)) + return NULL; + + if (!PyObject_TypeCheck(py_ace, &security_ace_Type)) { + PyErr_SetString(PyExc_TypeError, + "expected security.security_ace " + "for first argument to .dacl_del_ace"); + return NULL; + } + + ace = pytalloc_get_ptr(py_ace); + status = security_descriptor_dacl_del_ace(desc, ace); + PyErr_NTSTATUS_IS_ERR_RAISE(status); + Py_RETURN_NONE; +} + +static PyObject *py_descriptor_sacl_del_ace(PyObject *self, PyObject *args) +{ + struct security_descriptor *desc = pytalloc_get_ptr(self); + NTSTATUS status; + struct security_ace *ace = NULL; + PyObject *py_ace = Py_None; + + if (!PyArg_ParseTuple(args, "O!", &security_ace_Type, &py_ace)) + return NULL; + + if (!PyObject_TypeCheck(py_ace, &security_ace_Type)) { + PyErr_SetString(PyExc_TypeError, + "expected security.security_ace " + "for first argument to .sacl_del_ace"); + return NULL; + } + + ace = pytalloc_get_ptr(py_ace); + status = security_descriptor_sacl_del_ace(desc, ace); + PyErr_NTSTATUS_IS_ERR_RAISE(status); + Py_RETURN_NONE; +} + static PyObject *py_descriptor_new(PyTypeObject *self, PyObject *args, PyObject *kwargs) { return pytalloc_steal(self, security_descriptor_initialise(NULL)); @@ -302,7 +348,11 @@ static PyMethodDef py_descriptor_extra_methods[] = { NULL }, { "sacl_del", (PyCFunction)py_descriptor_sacl_del, METH_VARARGS, NULL }, - { "from_sddl", (PyCFunction)py_descriptor_from_sddl, METH_VARARGS|METH_CLASS, + { "dacl_del_ace", (PyCFunction)py_descriptor_dacl_del_ace, METH_VARARGS, + NULL }, + { "sacl_del_ace", (PyCFunction)py_descriptor_sacl_del_ace, METH_VARARGS, + NULL }, + { "from_sddl", (PyCFunction)py_descriptor_from_sddl, METH_VARARGS|METH_CLASS, NULL }, { "as_sddl", (PyCFunction)py_descriptor_as_sddl, METH_VARARGS, NULL }, -- 2.34.1 From bea7fd5eadccb670d3cfc233fd5cbc6c80d5cf95 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 1 Mar 2023 14:49:06 +1300 Subject: [PATCH 08/30] CVE-2023-4154 dsdb: Remove remaining references to DC_MODE_RETURN_NONE and DC_MODE_RETURN_ALL The confidential_attrs test no longer uses DC_MODE_RETURN_NONE we can now remove the complexity. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton (cherry picked from commit 82d2ec786f7e75ff6f34eb3357964345b10de091) --- .../dsdb/tests/python/confidential_attr.py | 86 ++++--------------- 1 file changed, 16 insertions(+), 70 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index 6889d5a5560..ac83f488061 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -70,20 +70,6 @@ lp = sambaopts.get_loadparm() creds = credopts.get_credentials(lp) creds.set_gensec_features(creds.get_gensec_features() | gensec.FEATURE_SEAL) -# When a user does not have access rights to view the objects' attributes, -# Windows and Samba behave slightly differently. -# A windows DC will always act as if the hidden attribute doesn't exist AT ALL -# (for an unprivileged user). So, even for a user that lacks access rights, -# the inverse/'!' queries should return ALL objects. This is similar to the -# kludgeaclredacted behaviour on Samba. -# However, on Samba (for implementation simplicity) we never return a matching -# result for an unprivileged user. -# Either approach is OK, so long as it gets applied consistently and we don't -# disclose any sensitive details by varying what gets returned by the search. -DC_MODE_RETURN_NONE = 0 -DC_MODE_RETURN_ALL = 1 - - # # Tests start here # @@ -193,25 +179,6 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # reset the value after the test completes self.addCleanup(self.set_attr_search_flags, self.attr_dn, old_value) - # The behaviour of the DC can differ in some cases, depending on whether - # we're talking to a Windows DC or a Samba DC - def guess_dc_mode(self): - # if we're in selftest, we can be pretty sure it's a Samba DC - if os.environ.get('SAMBA_SELFTEST') == '1': - return DC_MODE_RETURN_NONE - - searches = self.get_negative_match_all_searches() - res = self.ldb_user.search(self.test_dn, expression=searches[0], - scope=SCOPE_SUBTREE) - - # we default to DC_MODE_RETURN_NONE (samba).Update this if it - # looks like we're talking to a Windows DC - if len(res) == self.total_objects: - return DC_MODE_RETURN_ALL - - # otherwise assume samba DC behaviour - return DC_MODE_RETURN_NONE - def get_user_dn(self, name): return "CN={0},{1}".format(name, self.ou) @@ -359,7 +326,7 @@ class ConfidentialAttrCommon(samba.tests.TestCase): return expected_results # Returns the expected negative (i.e. '!') search behaviour when talking to - # a DC with DC_MODE_RETURN_ALL behaviour, i.e. we assert that users + # a DC, i.e. we assert that users # without rights always see ALL objects in '!' searches def negative_searches_return_all(self, has_rights_to=0, total_objects=None): @@ -409,32 +376,24 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # and what access rights the user has. # Note we only handle has_rights_to="all", 1 (the test object), or 0 (i.e. # we don't have rights to any objects) - def negative_search_expected_results(self, has_rights_to, dc_mode, - total_objects=None): + def negative_search_expected_results(self, has_rights_to, total_objects=None): if has_rights_to == "all": expect_results = self.negative_searches_all_rights(total_objects) - # if it's a Samba DC, we only expect the 'match-all' searches to return - # the objects that we have access rights to (all others are hidden). - # Whereas Windows 'hides' the objects by always returning all of them - elif dc_mode == DC_MODE_RETURN_NONE: - expect_results = self.negative_searches_return_none(has_rights_to) else: expect_results = self.negative_searches_return_all(has_rights_to, total_objects) return expect_results - def assert_negative_searches(self, has_rights_to=0, - dc_mode=DC_MODE_RETURN_NONE, samdb=None): + def assert_negative_searches(self, has_rights_to=0, samdb=None): """Asserts user without rights cannot see objects in '!' searches""" if samdb is None: samdb = self.ldb_user # build a dictionary of key=search-expr, value=expected_num assertions - expected_results = self.negative_search_expected_results(has_rights_to, - dc_mode) + expected_results = self.negative_search_expected_results(has_rights_to) for search, expected_num in expected_results.items(): self.assert_search_result(expected_num, search, samdb) @@ -490,8 +449,7 @@ class ConfidentialAttrTest(ConfidentialAttrCommon): self.make_attr_confidential() self.assert_conf_attr_searches(has_rights_to=0) - dc_mode = DC_MODE_RETURN_ALL - self.assert_negative_searches(has_rights_to=0, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=0) self.assert_attr_visible(expect_attr=False) # sanity-check we haven't hidden the attribute from the admin as well @@ -503,8 +461,7 @@ class ConfidentialAttrTest(ConfidentialAttrCommon): self.make_attr_confidential() self.assert_conf_attr_searches(has_rights_to=0) - dc_mode = DC_MODE_RETURN_ALL - self.assert_negative_searches(has_rights_to=0, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=0) self.assert_attr_visible(expect_attr=False) # apply the allow ACE to the object under test @@ -513,7 +470,7 @@ class ConfidentialAttrTest(ConfidentialAttrCommon): # the user should now be able to see the attribute for the one object # we gave it rights to self.assert_conf_attr_searches(has_rights_to=1) - self.assert_negative_searches(has_rights_to=1, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=1) self.assert_attr_visible(expect_attr=True) # sanity-check the admin can still see the attribute @@ -566,8 +523,7 @@ class ConfidentialAttrTest(ConfidentialAttrCommon): self.make_attr_confidential() self.assert_conf_attr_searches(has_rights_to=0) - dc_mode = DC_MODE_RETURN_ALL - self.assert_negative_searches(has_rights_to=0, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=0) self.assert_attr_visible(expect_attr=False) # apply the ACE to the object under test @@ -575,7 +531,7 @@ class ConfidentialAttrTest(ConfidentialAttrCommon): # this should make no difference to the user's ability to see the attr self.assert_conf_attr_searches(has_rights_to=0) - self.assert_negative_searches(has_rights_to=0, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=0) self.assert_attr_visible(expect_attr=False) # sanity-check the admin can still see the attribute @@ -707,8 +663,7 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon): return expected_results # override method specifically for deny ACL test cases - def assert_negative_searches(self, has_rights_to=0, - dc_mode=DC_MODE_RETURN_NONE, samdb=None): + def assert_negative_searches(self, has_rights_to=0, samdb=None): """Asserts user without rights cannot see objects in '!' searches""" if samdb is None: @@ -719,12 +674,9 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon): # assert this if the '!'/negative search behaviour is to suppress any # objects we don't have access rights to) excl_testobj = False - if has_rights_to != "all" and dc_mode == DC_MODE_RETURN_NONE: - excl_testobj = True # build a dictionary of key=search-expr, value=expected_num assertions - expected_results = self.negative_search_expected_results(has_rights_to, - dc_mode) + expected_results = self.negative_search_expected_results(has_rights_to) for search, expected_num in expected_results.items(): self.assert_search_result(expected_num, search, samdb, @@ -741,9 +693,7 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon): # the user shouldn't be able to see the attribute anymore self.assert_conf_attr_searches(has_rights_to="deny-one") - dc_mode = DC_MODE_RETURN_ALL - self.assert_negative_searches(has_rights_to="deny-one", - dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to="deny-one") self.assert_attr_visible(expect_attr=False) # sanity-check we haven't hidden the attribute from the admin as well @@ -887,8 +837,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): attrs=['name']) # override method specifically for dirsync (total object count differs) - def assert_negative_searches(self, has_rights_to=0, - dc_mode=DC_MODE_RETURN_NONE, samdb=None): + def assert_negative_searches(self, has_rights_to=0, samdb=None): """Asserts user without rights cannot see objects in '!' searches""" if samdb is None: @@ -898,7 +847,6 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): # here only includes the user objects (not the parent OU) total_objects = len(self.all_users) expected_results = self.negative_search_expected_results(has_rights_to, - dc_mode, total_objects) for search, expected_num in expected_results.items(): @@ -917,8 +865,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): self.assert_conf_attr_searches(has_rights_to=0) self.assert_attr_visible(expect_attr=False) - dc_mode = DC_MODE_RETURN_ALL - self.assert_negative_searches(has_rights_to=0, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=0) # as a final sanity-check, make sure the admin can still see the attr self.assert_conf_attr_searches(has_rights_to="all", @@ -1012,8 +959,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): # check we can't see the objects now, even with using dirsync controls self.assert_conf_attr_searches(has_rights_to=0) self.assert_attr_visible(expect_attr=False) - dc_mode = DC_MODE_RETURN_ALL - self.assert_negative_searches(has_rights_to=0, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=0) # now delete the users (except for the user whose LDB connection # we're currently using) @@ -1023,7 +969,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): # check we still can't see the objects self.assert_conf_attr_searches(has_rights_to=0) - self.assert_negative_searches(has_rights_to=0, dc_mode=dc_mode) + self.assert_negative_searches(has_rights_to=0) def test_timing_attack(self): # Create the machine account. -- 2.34.1 From 38d62aa3b2b202d2080b8814f6d9acd8bf99f226 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Fri, 27 Jan 2023 07:43:40 +1300 Subject: [PATCH 09/30] CVE-2023-4154 s4:dsdb:tests: Refactor confidential attributes test Use more specific unittest methods, and remove unused code. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett (cherry picked from commit 2e5d08c908b3fa48b9b374279a331061cb77bce3) --- .../dsdb/tests/python/confidential_attr.py | 69 +++++-------------- 1 file changed, 16 insertions(+), 53 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index ac83f488061..eb75da6374f 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -24,7 +24,6 @@ import sys sys.path.insert(0, "bin/python") import samba -import os import random import statistics import time @@ -136,9 +135,9 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # sanity-check the flag is not already set (this'll cause problems if # previous test run didn't clean up properly) search_flags = self.get_attr_search_flags(self.attr_dn) - self.assertTrue(int(search_flags) & SEARCH_FLAG_CONFIDENTIAL == 0, - "{0} searchFlags already {1}".format(self.conf_attr, - search_flags)) + self.assertEqual(0, int(search_flags) & SEARCH_FLAG_CONFIDENTIAL, + "{0} searchFlags already {1}".format(self.conf_attr, + search_flags)) def tearDown(self): super(ConfidentialAttrCommon, self).tearDown() @@ -208,9 +207,9 @@ class ConfidentialAttrCommon(samba.tests.TestCase): for attr in attr_filters: res = samdb.search(self.test_dn, expression=expr, scope=SCOPE_SUBTREE, attrs=attr) - self.assertTrue(len(res) == expected_num, - "%u results, not %u for search %s, attr %s" % - (len(res), expected_num, expr, str(attr))) + self.assertEqual(len(res), expected_num, + "%u results, not %u for search %s, attr %s" % + (len(res), expected_num, expr, str(attr))) # return a selection of searches that match exactly against the test object def get_exact_match_searches(self): @@ -352,25 +351,6 @@ class ConfidentialAttrCommon(samba.tests.TestCase): return expected_results - # Returns the expected negative (i.e. '!') search behaviour when talking to - # a DC with DC_MODE_RETURN_NONE behaviour, i.e. we assert that users - # without rights cannot see objects in '!' searches at all - def negative_searches_return_none(self, has_rights_to=0): - expected_results = {} - - # the 'match-all' searches should only return the objects we have - # access rights to (if any) - for search in self.get_negative_match_all_searches(): - expected_results[search] = has_rights_to - - # for inverse matches, we should NOT be told about any objects at all - inverse_searches = self.get_inverse_match_searches() - inverse_searches += ["(!({0}=*))".format(self.conf_attr)] - for search in inverse_searches: - expected_results[search] = 0 - - return expected_results - # Returns the expected negative (i.e. '!') search behaviour. This varies # depending on what type of DC we're talking to (i.e. Windows or Samba) # and what access rights the user has. @@ -403,7 +383,7 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # checks whether the confidential attribute is present res = samdb.search(self.conf_dn, expression="(objectClass=*)", scope=SCOPE_SUBTREE, attrs=attrs) - self.assertTrue(len(res) == 1) + self.assertEqual(1, len(res)) attr_returned = False for msg in res: @@ -586,9 +566,9 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon): for attr in attr_filters: res = samdb.search(self.test_dn, expression=expr, scope=SCOPE_SUBTREE, attrs=attr) - self.assertTrue(len(res) == expected_num, - "%u results, not %u for search %s, attr %s" % - (len(res), expected_num, expr, str(attr))) + self.assertEqual(len(res), expected_num, + "%u results, not %u for search %s, attr %s" % + (len(res), expected_num, expr, str(attr))) # assert we haven't revealed the hidden test-object if excl_testobj: @@ -604,7 +584,7 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon): # make sure the test object is not returned if we've been denied rights # to it via an ACE - excl_testobj = True if has_rights_to == "deny-one" else False + excl_testobj = has_rights_to == "deny-one" # these first few searches we just expect to match against the one # object under test that we're trying to guess the value of @@ -625,24 +605,6 @@ class ConfidentialAttrTestDenyAcl(ConfidentialAttrCommon): self.assert_search_result(expected_num, search, samdb, excl_testobj) - # override method specifically for deny ACL test cases. Instead of being - # granted access to either no objects or only one, we are being denied - # access to only one object (but can still access the rest). - def negative_searches_return_none(self, has_rights_to=0): - expected_results = {} - - # on Samba we will see the objects we have rights to, but the one we - # are denied access to will be hidden - searches = self.get_negative_match_all_searches() - searches += self.get_inverse_match_searches() - for search in searches: - expected_results[search] = self.total_objects - 1 - - # The wildcard returns the objects without this attribute as normal. - search = "(!({0}=*))".format(self.conf_attr) - expected_results[search] = self.total_objects - self.objects_with_attr - return expected_results - # override method specifically for deny ACL test cases def negative_searches_return_all(self, has_rights_to=0, total_objects=None): @@ -783,7 +745,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): for attr in self.attr_filters: res = samdb.search(base_dn, expression=search, scope=SCOPE_SUBTREE, attrs=attr, controls=self.dirsync) - self.assertTrue(len(res) == expected_num, + self.assertEqual(len(res), expected_num, "%u results, not %u for search %s, attr %s" % (len(res), expected_num, search, str(attr))) @@ -797,7 +759,8 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): expr = self.single_obj_filter res = samdb.search(self.base_dn, expression=expr, scope=SCOPE_SUBTREE, attrs=attrs, controls=self.dirsync) - self.assertTrue(len(res) == 1 or no_result_ok) + if not no_result_ok: + self.assertEqual(1, len(res)) attr_returned = False for msg in res: @@ -888,7 +851,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): search_flags = int(self.get_attr_search_flags(self.attr_dn)) # check we've already set the confidential flag - self.assertTrue(search_flags & SEARCH_FLAG_CONFIDENTIAL != 0) + self.assertNotEqual(0, search_flags & SEARCH_FLAG_CONFIDENTIAL) search_flags |= SEARCH_FLAG_PRESERVEONDELETE self.set_attr_search_flags(self.attr_dn, str(search_flags)) @@ -964,7 +927,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): # now delete the users (except for the user whose LDB connection # we're currently using) for user in self.all_users: - if user != self.user: + if user is not self.user: self.ldb_admin.delete(self.get_user_dn(user)) # check we still can't see the objects -- 2.34.1 From 76091f35016bd6e642237973981b1c88a9e44062 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Wed, 2 Aug 2023 10:44:32 +0200 Subject: [PATCH 10/30] CVE-2023-4154 s4:dsdb:tests: Fix code spelling BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andreas Schneider Reviewed-by: Joseph Sutton (cherry picked from commit b29793ffdee5d9b9c1c05830622e80f7faec7670) --- source4/dsdb/tests/python/acl.py | 12 ++++++------ .../dsdb/tests/python/ad_dc_search_performance.py | 2 +- source4/dsdb/tests/python/confidential_attr.py | 2 +- source4/dsdb/tests/python/dirsync.py | 8 ++++---- source4/dsdb/tests/python/ldap.py | 14 +++++++------- source4/dsdb/tests/python/ldap_modify_order.py | 4 ++-- source4/dsdb/tests/python/ldap_syntaxes.py | 4 ++-- source4/dsdb/tests/python/login_basics.py | 2 +- source4/dsdb/tests/python/password_settings.py | 4 ++-- source4/dsdb/tests/python/passwords.py | 4 ++-- source4/dsdb/tests/python/sam.py | 2 +- source4/dsdb/tests/python/sec_descriptor.py | 14 +++++++------- source4/dsdb/tests/python/token_group.py | 4 ++-- source4/dsdb/tests/python/user_account_control.py | 2 +- 14 files changed, 39 insertions(+), 39 deletions(-) diff --git a/source4/dsdb/tests/python/acl.py b/source4/dsdb/tests/python/acl.py index 6751934af0c..eb2bafda3e5 100755 --- a/source4/dsdb/tests/python/acl.py +++ b/source4/dsdb/tests/python/acl.py @@ -174,7 +174,7 @@ class AclAddTests(AclTests): self.assertEqual(len(res), 0) def test_add_u1(self): - """Testing OU with the rights of Doman Admin not creator of the OU """ + """Testing OU with the rights of Domain Admin not creator of the OU """ self.assert_top_ou_deleted() # Change descriptor for top level OU self.ldb_owner.create_ou("OU=test_add_ou1," + self.base_dn) @@ -187,7 +187,7 @@ class AclAddTests(AclTests): self.ldb_notowner.newgroup("test_add_group1", groupou="OU=test_add_ou2,OU=test_add_ou1", grouptype=samba.dsdb.GTYPE_DISTRIBUTION_DOMAIN_LOCAL_GROUP) # Make sure we HAVE created the two objects -- user and group - # !!! We should not be able to do that, but however beacuse of ACE ordering our inherited Deny ACE + # !!! We should not be able to do that, but however because of ACE ordering our inherited Deny ACE # !!! comes after explicit (A;;RPWPCRCCDCLCLORCWOWDSDDTSW;;;DA) that comes from somewhere res = self.ldb_admin.search(self.base_dn, expression="(distinguishedName=%s,%s)" % ("CN=test_add_user1,OU=test_add_ou2,OU=test_add_ou1", self.base_dn)) self.assertTrue(len(res) > 0) @@ -248,7 +248,7 @@ class AclAddTests(AclTests): self.assertEqual(len(res), 0) def test_add_u4(self): - """ 4 Testing OU with the rights of Doman Admin creator of the OU""" + """ 4 Testing OU with the rights of Domain Admin creator of the OU""" self.assert_top_ou_deleted() self.ldb_owner.create_ou("OU=test_add_ou1," + self.base_dn) self.ldb_owner.create_ou("OU=test_add_ou2,OU=test_add_ou1," + self.base_dn) @@ -1890,7 +1890,7 @@ class AclDeleteTests(AclTests): self.assertEqual(len(res), 0) def test_delete_u3(self): - """User indentified by SID has RIGHT_DELETE to another User object""" + """User identified by SID has RIGHT_DELETE to another User object""" user_dn = self.get_user_dn("test_delete_user1") # Create user that we try to delete self.ldb_admin.newuser("test_delete_user1", self.user_pass) @@ -2181,7 +2181,7 @@ class AclCARTests(AclTests): minPwdAge = self.ldb_admin.get_minPwdAge() # Reset the "minPwdAge" as it was before self.addCleanup(self.ldb_admin.set_minPwdAge, minPwdAge) - # Set it temporarely to "0" + # Set it temporarily to "0" self.ldb_admin.set_minPwdAge("0") self.user_with_wp = "acl_car_user1" @@ -2637,7 +2637,7 @@ class AclUndeleteTests(AclTests): self.sd_utils.dacl_add_ace(self.deleted_dn2, mod) self.undelete_deleted(self.deleted_dn2, self.testuser2_dn) - # attempt undelete with simultanious addition of url, WP to which is denied + # attempt undelete with simultaneous addition of url, WP to which is denied mod = "(OD;;WP;9a9a0221-4a5b-11d1-a9c3-0000f80367c1;;%s)" % str(self.sid) self.sd_utils.dacl_add_ace(self.deleted_dn3, mod) try: diff --git a/source4/dsdb/tests/python/ad_dc_search_performance.py b/source4/dsdb/tests/python/ad_dc_search_performance.py index 0afd7a2582e..44e468097d8 100644 --- a/source4/dsdb/tests/python/ad_dc_search_performance.py +++ b/source4/dsdb/tests/python/ad_dc_search_performance.py @@ -180,7 +180,7 @@ class UserTests(samba.tests.TestCase): maybe_not = ['!(', ''] joiners = ['&', '|'] - # The number of permuations is 18432, which is not huge but + # The number of permutations is 18432, which is not huge but # would take hours to search. So we take a sample. all_permutations = list(itertools.product(joiners, classes, classes, diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index eb75da6374f..8ca56bd1023 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -722,7 +722,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): self.attr_filters = [None, ["*"], ["name"]] - # Note dirsync behaviour is slighty different for the attribute under + # Note dirsync behaviour is slightly different for the attribute under # test - when you have full access rights, it only returns the objects # that actually have this attribute (i.e. it doesn't return an empty # message with just the DN). So we add the 'name' attribute into the diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index 1ac719e4332..ca0947e2d21 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -338,7 +338,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(len(res.msgs[0]), 3) def test_dirsync_othernc(self): - """Check that dirsync return information for entries that are normaly referrals (ie. other NCs)""" + """Check that dirsync return information for entries that are normally referrals (ie. other NCs)""" res = self.ldb_admin.search(self.base_dn, expression="(objectclass=configuration)", attrs=["name"], @@ -459,7 +459,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): delete_force(self.ldb_admin, ouname) def test_dirsync_linkedattributes(self): - """Check that dirsync returnd deleted objects too""" + """Check that dirsync returned deleted objects too""" # Let's search for members self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) res = self.ldb_simple.search(self.base_dn, @@ -541,7 +541,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(len(res[0].get("member")), 0) def test_dirsync_deleted_items(self): - """Check that dirsync returnd deleted objects too""" + """Check that dirsync returned deleted objects too""" # Let's create an OU ouname = "OU=testou3,%s" % self.base_dn self.ouname = ouname @@ -712,7 +712,7 @@ class ExtendedDirsyncTests(SimpleDirsyncTests): self.assertIn(b">; Date: Tue, 14 Feb 2023 17:19:27 +1300 Subject: [PATCH 11/30] CVE-2023-4154 s4-dsdb: Remove DSDB_ACL_CHECKS_DIRSYNC_FLAG It's no longer used anywhere. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett (cherry picked from commit 8b4e6f7b3fb8018cb64deef9b8e1cbc2e5ba12cf) --- source4/dsdb/samdb/ldb_modules/dirsync.c | 11 ++--------- source4/dsdb/samdb/samdb.h | 1 - 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/dirsync.c b/source4/dsdb/samdb/ldb_modules/dirsync.c index fa57af49e8f..b3c463741c8 100644 --- a/source4/dsdb/samdb/ldb_modules/dirsync.c +++ b/source4/dsdb/samdb/ldb_modules/dirsync.c @@ -1005,7 +1005,6 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req struct dirsync_context *dsc; struct ldb_context *ldb; struct ldb_parse_tree *new_tree = req->op.search.tree; - uint32_t flags = 0; enum ndr_err_code ndr_err; DATA_BLOB blob; const char **attrs; @@ -1117,13 +1116,8 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req return ret; } talloc_free(acl_res); - } else { - flags |= DSDB_ACL_CHECKS_DIRSYNC_FLAG; - - if (ret != LDB_SUCCESS) { - return ret; - } - + } else if (ret != LDB_SUCCESS) { + return ret; } dsc->functional_level = dsdb_functional_level(ldb); @@ -1394,7 +1388,6 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req req->controls, dsc, dirsync_search_callback, req); - ldb_req_set_custom_flags(down_req, flags); LDB_REQ_SET_LOCATION(down_req); if (ret != LDB_SUCCESS) { return ret; diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h index 5cae2681ed0..7df86e56683 100644 --- a/source4/dsdb/samdb/samdb.h +++ b/source4/dsdb/samdb/samdb.h @@ -362,7 +362,6 @@ struct dsdb_extended_dn_store_format { #define DSDB_FULL_JOIN_REPLICATION_COMPLETED_OPAQUE_NAME "DSDB_FULL_JOIN_REPLICATION_COMPLETED" -#define DSDB_ACL_CHECKS_DIRSYNC_FLAG 0x1 #define DSDB_SAMDB_MINIMUM_ALLOWED_RID 1000 #define DSDB_METADATA_SCHEMA_SEQ_NUM "SCHEMA_SEQ_NUM" -- 2.34.1 From 60baeea804aeaf9a2ea618d14985a9b7560e03a7 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 10 Mar 2023 18:25:18 +0100 Subject: [PATCH 12/30] CVE-2023-4154 python:sd_utils: introduce update_aces_in_dacl() helper This is a more generic api that can be re-used in other places as well in future. It operates on a security descriptor object instead of SDDL. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 8411e6d302e25d10f1035ebbdcbde7308566e930) --- python/samba/sd_utils.py | 124 +++++++++++++++++++++++++++++++++++---- 1 file changed, 112 insertions(+), 12 deletions(-) diff --git a/python/samba/sd_utils.py b/python/samba/sd_utils.py index 26e80ee2f4a..52a78de5d09 100644 --- a/python/samba/sd_utils.py +++ b/python/samba/sd_utils.py @@ -21,8 +21,11 @@ import samba from ldb import Message, MessageElement, Dn from ldb import FLAG_MOD_REPLACE, SCOPE_BASE -from samba.ndr import ndr_pack, ndr_unpack +from samba.ndr import ndr_pack, ndr_unpack, ndr_deepcopy from samba.dcerpc import security +from samba.ntstatus import ( + NT_STATUS_OBJECT_NAME_NOT_FOUND, +) class SDUtils(object): @@ -63,19 +66,116 @@ class SDUtils(object): res = self.ldb.search(object_dn) return ndr_unpack(security.dom_sid, res[0]["objectSid"][0]) + def update_aces_in_dacl(self, dn, del_aces=None, add_aces=None, + sddl_attr=None, controls=None): + if del_aces is None: + del_aces=[] + if add_aces is None: + add_aces=[] + + def ace_from_sddl(ace_sddl): + ace_sd = security.descriptor.from_sddl("D:" + ace_sddl, self.domain_sid) + assert(len(ace_sd.dacl.aces)==1) + return ace_sd.dacl.aces[0] + + if sddl_attr is None: + if controls is None: + controls=["sd_flags:1:%d" % security.SECINFO_DACL] + sd = self.read_sd_on_dn(dn, controls=controls) + if not sd.type & security.SEC_DESC_DACL_PROTECTED: + # if the DACL is not protected remove all + # inherited aces, as they will be re-inherited + # on the server, we need a ndr_deepcopy in order + # to avoid reference problems while deleting + # the aces while looping over them + dacl_copy = ndr_deepcopy(sd.dacl) + for ace in dacl_copy.aces: + if ace.flags & security.SEC_ACE_FLAG_INHERITED_ACE: + try: + sd.dacl_del_ace(ace) + except samba.NTSTATUSError as err: + if err.args[0] != NT_STATUS_OBJECT_NAME_NOT_FOUND: + raise err + # dacl_del_ace may remove more than + # one ace, so we may not find it anymore + pass + else: + if controls is None: + controls=[] + res = self.ldb.search(dn, SCOPE_BASE, None, + [sddl_attr], controls=controls) + old_sddl = str(res[0][sddl_attr][0]) + sd = security.descriptor.from_sddl(old_sddl, self.domain_sid) + + num_changes = 0 + del_ignored = [] + add_ignored = [] + inherited_ignored = [] + + for ace in del_aces: + if isinstance(ace, str): + ace = ace_from_sddl(ace) + assert(isinstance(ace, security.ace)) + + if ace.flags & security.SEC_ACE_FLAG_INHERITED_ACE: + inherited_ignored.append(ace) + continue + + if ace not in sd.dacl.aces: + del_ignored.append(ace) + continue + + sd.dacl_del_ace(ace) + num_changes += 1 + + for ace in add_aces: + add_idx = -1 + if isinstance(ace, dict): + if "idx" in ace: + add_idx = ace["idx"] + ace = ace["ace"] + if isinstance(ace, str): + ace = ace_from_sddl(ace) + assert(isinstance(ace, security.ace)) + + if ace.flags & security.SEC_ACE_FLAG_INHERITED_ACE: + inherited_ignored.append(ace) + continue + + if ace in sd.dacl.aces: + add_ignored.append(ace) + continue + + sd.dacl_add(ace, add_idx) + num_changes += 1 + + if num_changes == 0: + return del_ignored, add_ignored, inherited_ignored + + if sddl_attr is None: + self.modify_sd_on_dn(dn, sd, controls=controls) + else: + new_sddl = sd.as_sddl(self.domain_sid) + m = Message() + m.dn = dn + m[sddl_attr] = MessageElement(new_sddl.encode('ascii'), + FLAG_MOD_REPLACE, + sddl_attr) + self.ldb.modify(m, controls=controls) + + return del_ignored, add_ignored, inherited_ignored + def dacl_add_ace(self, object_dn, ace): - """Add an ACE to an objects security descriptor + """Add an ACE (or more) to an objects security descriptor """ - desc = self.read_sd_on_dn(object_dn, ["show_deleted:1"]) - desc_sddl = desc.as_sddl(self.domain_sid) - if ace in desc_sddl: - return - if desc_sddl.find("(") >= 0: - desc_sddl = (desc_sddl[:desc_sddl.index("(")] + ace + - desc_sddl[desc_sddl.index("("):]) - else: - desc_sddl = desc_sddl + ace - self.modify_sd_on_dn(object_dn, desc_sddl, ["show_deleted:1"]) + ace_sd = security.descriptor.from_sddl("D:" + ace, self.domain_sid) + add_aces = [] + add_idx = 0 + for ace in ace_sd.dacl.aces: + add_aces.append({"idx": add_idx, "ace": ace}) + add_idx += 1 + _,_,_ = self.update_aces_in_dacl(object_dn, add_aces=add_aces, + controls=["show_deleted:1"]) def get_sd_as_sddl(self, object_dn, controls=[]): """Return object nTSecutiryDescriptor in SDDL format -- 2.34.1 From d038ac36c13b5eb8f17491c9c066d3111a8f7d79 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 18:03:10 +0100 Subject: [PATCH 13/30] CVE-2023-4154 python:sd_utils: add dacl_{prepend,append,delete}_aces() helpers They better represent what they are doing, we keep dacl_add_ace() as wrapper of dacl_prepend_aces() in order to let existing callers work as before. In future it would be good to have a dacl_insert_aces() that would canonicalize the ace order before storing, but that a task for another day. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit a1109a9bf12e020636b8d66fc54984aac58bfe6b) --- python/samba/sd_utils.py | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/python/samba/sd_utils.py b/python/samba/sd_utils.py index 52a78de5d09..462bbfbaf18 100644 --- a/python/samba/sd_utils.py +++ b/python/samba/sd_utils.py @@ -165,17 +165,46 @@ class SDUtils(object): return del_ignored, add_ignored, inherited_ignored - def dacl_add_ace(self, object_dn, ace): - """Add an ACE (or more) to an objects security descriptor + def dacl_prepend_aces(self, object_dn, aces, controls=None): + """Prepend an ACE (or more) to an objects security descriptor """ - ace_sd = security.descriptor.from_sddl("D:" + ace, self.domain_sid) + ace_sd = security.descriptor.from_sddl("D:" + aces, self.domain_sid) add_aces = [] add_idx = 0 for ace in ace_sd.dacl.aces: add_aces.append({"idx": add_idx, "ace": ace}) add_idx += 1 - _,_,_ = self.update_aces_in_dacl(object_dn, add_aces=add_aces, - controls=["show_deleted:1"]) + _,ai,ii = self.update_aces_in_dacl(object_dn, add_aces=add_aces, + controls=controls) + return ai, ii + + def dacl_add_ace(self, object_dn, ace): + """Add an ACE (or more) to an objects security descriptor + """ + _,_ = self.dacl_prepend_aces(object_dn, ace, + controls=["show_deleted:1"]) + + def dacl_append_aces(self, object_dn, aces, controls=None): + """Append an ACE (or more) to an objects security descriptor + """ + ace_sd = security.descriptor.from_sddl("D:" + aces, self.domain_sid) + add_aces = [] + for ace in ace_sd.dacl.aces: + add_aces.append(ace) + _,ai,ii = self.update_aces_in_dacl(object_dn, add_aces=add_aces, + controls=controls) + return ai, ii + + def dacl_delete_aces(self, object_dn, aces, controls=None): + """Delete an ACE (or more) to an objects security descriptor + """ + del_sd = security.descriptor.from_sddl("D:" + aces, self.domain_sid) + del_aces = [] + for ace in del_sd.dacl.aces: + del_aces.append(ace) + di,_,ii = self.update_aces_in_dacl(object_dn, del_aces=del_aces, + controls=controls) + return di, ii def get_sd_as_sddl(self, object_dn, controls=[]): """Return object nTSecutiryDescriptor in SDDL format -- 2.34.1 From ebd421306e7b1ec37e7a477937d04a27de838cff Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 10:11:05 +0100 Subject: [PATCH 14/30] CVE-2023-4154 py_security: allow idx argument to descriptor.[s|d]acl_add() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 9ea06aaf9f57e3c7094553d9ac40fb73057a9b74) --- source4/librpc/ndr/py_security.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/source4/librpc/ndr/py_security.c b/source4/librpc/ndr/py_security.c index e61b994d7cb..4a8271a11db 100644 --- a/source4/librpc/ndr/py_security.c +++ b/source4/librpc/ndr/py_security.c @@ -175,12 +175,13 @@ static PyObject *py_descriptor_sacl_add(PyObject *self, PyObject *args) NTSTATUS status; struct security_ace *ace; PyObject *py_ace; + Py_ssize_t idx = -1; - if (!PyArg_ParseTuple(args, "O", &py_ace)) + if (!PyArg_ParseTuple(args, "O|n", &py_ace, &idx)) return NULL; ace = pytalloc_get_ptr(py_ace); - status = security_descriptor_sacl_add(desc, ace); + status = security_descriptor_sacl_insert(desc, ace, idx); PyErr_NTSTATUS_IS_ERR_RAISE(status); Py_RETURN_NONE; } @@ -191,13 +192,14 @@ static PyObject *py_descriptor_dacl_add(PyObject *self, PyObject *args) NTSTATUS status; struct security_ace *ace; PyObject *py_ace; + Py_ssize_t idx = -1; - if (!PyArg_ParseTuple(args, "O", &py_ace)) + if (!PyArg_ParseTuple(args, "O|n", &py_ace, &idx)) return NULL; ace = pytalloc_get_ptr(py_ace); - status = security_descriptor_dacl_add(desc, ace); + status = security_descriptor_dacl_insert(desc, ace, idx); PyErr_NTSTATUS_IS_ERR_RAISE(status); Py_RETURN_NONE; } -- 2.34.1 From 92cf3328a00cacb07fe7c6b7abf5335dc8235e86 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 17 Mar 2023 14:08:34 +0100 Subject: [PATCH 15/30] CVE-2023-4154 python/samba/ndr: add ndr_deepcopy() helper This uses ndr_pack/unpack in order to create a deep copy of the given object. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 4627997ddae44265ad35b3234232eb74458c6c34) --- python/samba/ndr.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/python/samba/ndr.py b/python/samba/ndr.py index 35b2414e8ae..8369abfb2d0 100644 --- a/python/samba/ndr.py +++ b/python/samba/ndr.py @@ -56,6 +56,25 @@ def ndr_print(object): return ndr_print() +def ndr_deepcopy(object): + """Create a deep copy of a NDR object, using pack/unpack + + :param object: Object to copy + :return: The object copy + """ + ndr_pack = getattr(object, "__ndr_pack__", None) + if ndr_pack is None: + raise TypeError("%r is not a NDR object" % object) + data = ndr_pack() + cls = type(object) + copy = cls() + ndr_unpack = getattr(copy, "__ndr_unpack__", None) + if ndr_unpack is None: + raise TypeError("%r is not a NDR object" % copy) + ndr_unpack(data, allow_remaining=False) + return copy + + def ndr_pack_in(object, bigendian=False, ndr64=False): """Pack the input of an NDR function object. -- 2.34.1 From 2c7710bd5bc979d5fa601d2ee841592694bb14df Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 09:57:43 +0100 Subject: [PATCH 16/30] CVE-2023-4154 replace: add ARRAY_INSERT_ELEMENT() helper BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 9d8ff0d1e0b2ba7c84af36e1931f5bc99902a44b) --- lib/replace/replace.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/replace/replace.h b/lib/replace/replace.h index bd7f6e53e81..bcd5c09bf7c 100644 --- a/lib/replace/replace.h +++ b/lib/replace/replace.h @@ -889,6 +889,21 @@ typedef unsigned long long ptrdiff_t ; #define ARRAY_DEL_ELEMENT(a,i,n) \ if((i)<((n)-1)){memmove(&((a)[(i)]),&((a)[(i)+1]),(sizeof(*(a))*((n)-(i)-1)));} +/** + * Insert an array element by moving the rest one up + * + */ +#define ARRAY_INSERT_ELEMENT(__array,__old_last_idx,__new_elem,__new_idx) do { \ + if ((__new_idx) < (__old_last_idx)) { \ + const void *__src = &((__array)[(__new_idx)]); \ + void *__dst = &((__array)[(__new_idx)+1]); \ + size_t __num = (__old_last_idx)-(__new_idx); \ + size_t __len = sizeof(*(__array)) * __num; \ + memmove(__dst, __src, __len); \ + } \ + (__array)[(__new_idx)] = (__new_elem); \ +} while(0) + /** * Pointer difference macro */ -- 2.34.1 From 3c34a51da12c4b1fb446f9a384ff57cdc6019632 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 10:00:11 +0100 Subject: [PATCH 17/30] CVE-2023-4154 libcli/security: prepare security_descriptor_acl_add() to place the ace at a position Often it is important to insert an ace at a specific position in the ACL. As a default we still append by default by using -1, which is the generic version of passing the number of existing aces. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit c3cb915a67aff6739b72b86d7d139609df309ada) --- libcli/security/security_descriptor.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/libcli/security/security_descriptor.c b/libcli/security/security_descriptor.c index 64c2d027876..8657c797364 100644 --- a/libcli/security/security_descriptor.c +++ b/libcli/security/security_descriptor.c @@ -267,9 +267,11 @@ NTSTATUS security_descriptor_for_client(TALLOC_CTX *mem_ctx, static NTSTATUS security_descriptor_acl_add(struct security_descriptor *sd, bool add_to_sacl, - const struct security_ace *ace) + const struct security_ace *ace, + ssize_t _idx) { struct security_acl *acl = NULL; + ssize_t idx; if (add_to_sacl) { acl = sd->sacl; @@ -288,15 +290,28 @@ static NTSTATUS security_descriptor_acl_add(struct security_descriptor *sd, acl->aces = NULL; } + if (_idx < 0) { + idx = (acl->num_aces + 1) + _idx; + } else { + idx = _idx; + } + + if (idx < 0) { + return NT_STATUS_ARRAY_BOUNDS_EXCEEDED; + } else if (idx > acl->num_aces) { + return NT_STATUS_ARRAY_BOUNDS_EXCEEDED; + } + acl->aces = talloc_realloc(acl, acl->aces, struct security_ace, acl->num_aces+1); if (acl->aces == NULL) { return NT_STATUS_NO_MEMORY; } - acl->aces[acl->num_aces] = *ace; + ARRAY_INSERT_ELEMENT(acl->aces, acl->num_aces, *ace, idx); + acl->num_aces++; - switch (acl->aces[acl->num_aces].type) { + switch (acl->aces[idx].type) { case SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT: case SEC_ACE_TYPE_ACCESS_DENIED_OBJECT: case SEC_ACE_TYPE_SYSTEM_AUDIT_OBJECT: @@ -307,8 +322,6 @@ static NTSTATUS security_descriptor_acl_add(struct security_descriptor *sd, break; } - acl->num_aces++; - if (add_to_sacl) { sd->sacl = acl; sd->type |= SEC_DESC_SACL_PRESENT; @@ -327,7 +340,7 @@ static NTSTATUS security_descriptor_acl_add(struct security_descriptor *sd, NTSTATUS security_descriptor_sacl_add(struct security_descriptor *sd, const struct security_ace *ace) { - return security_descriptor_acl_add(sd, true, ace); + return security_descriptor_acl_add(sd, true, ace, -1); } /* @@ -337,7 +350,7 @@ NTSTATUS security_descriptor_sacl_add(struct security_descriptor *sd, NTSTATUS security_descriptor_dacl_add(struct security_descriptor *sd, const struct security_ace *ace) { - return security_descriptor_acl_add(sd, false, ace); + return security_descriptor_acl_add(sd, false, ace, -1); } /* -- 2.34.1 From 3de5d8a01163bfa70464d7532069ed467e4ffd10 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 10:03:44 +0100 Subject: [PATCH 18/30] CVE-2023-4154 libcli/security: add security_descriptor_[s|d]acl_insert() helpers BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 2c02378029fff6636b8f19e45af78b265f2210ed) --- libcli/security/security_descriptor.c | 28 +++++++++++++++++++++++++++ libcli/security/security_descriptor.h | 6 ++++++ 2 files changed, 34 insertions(+) diff --git a/libcli/security/security_descriptor.c b/libcli/security/security_descriptor.c index 8657c797364..08f2cf19ee8 100644 --- a/libcli/security/security_descriptor.c +++ b/libcli/security/security_descriptor.c @@ -343,6 +343,20 @@ NTSTATUS security_descriptor_sacl_add(struct security_descriptor *sd, return security_descriptor_acl_add(sd, true, ace, -1); } +/* + insert an ACE at a given index to the SACL of a security_descriptor + + idx can be negative, which means it's related to the new size from the + end, so -1 means the ace is appended at the end. +*/ + +NTSTATUS security_descriptor_sacl_insert(struct security_descriptor *sd, + const struct security_ace *ace, + ssize_t idx) +{ + return security_descriptor_acl_add(sd, true, ace, idx); +} + /* add an ACE to the DACL of a security_descriptor */ @@ -353,6 +367,20 @@ NTSTATUS security_descriptor_dacl_add(struct security_descriptor *sd, return security_descriptor_acl_add(sd, false, ace, -1); } +/* + insert an ACE at a given index to the DACL of a security_descriptor + + idx can be negative, which means it's related to the new size from the + end, so -1 means the ace is appended at the end. +*/ + +NTSTATUS security_descriptor_dacl_insert(struct security_descriptor *sd, + const struct security_ace *ace, + ssize_t idx) +{ + return security_descriptor_acl_add(sd, false, ace, idx); +} + /* delete the ACE corresponding to the given trustee in an ACL of a security_descriptor diff --git a/libcli/security/security_descriptor.h b/libcli/security/security_descriptor.h index 46545321d15..354bc17e925 100644 --- a/libcli/security/security_descriptor.h +++ b/libcli/security/security_descriptor.h @@ -33,8 +33,14 @@ NTSTATUS security_descriptor_for_client(TALLOC_CTX *mem_ctx, struct security_descriptor **_csd); NTSTATUS security_descriptor_sacl_add(struct security_descriptor *sd, const struct security_ace *ace); +NTSTATUS security_descriptor_sacl_insert(struct security_descriptor *sd, + const struct security_ace *ace, + ssize_t idx); NTSTATUS security_descriptor_dacl_add(struct security_descriptor *sd, const struct security_ace *ace); +NTSTATUS security_descriptor_dacl_insert(struct security_descriptor *sd, + const struct security_ace *ace, + ssize_t idx); NTSTATUS security_descriptor_dacl_del(struct security_descriptor *sd, const struct dom_sid *trustee); NTSTATUS security_descriptor_sacl_del(struct security_descriptor *sd, -- 2.34.1 From d7ab8d4c2ea390d4d4f9be55f7072fa875457721 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 11:55:55 +1200 Subject: [PATCH 19/30] CVE-2023-4154 dsdb/tests: Do not run SimpleDirsyncTests twice To re-use setup code, the super-class must have no test_*() methods otherwise these will be run as well as the class-local tests. We rename tests that would otherwise have duplicate names BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- selftest/knownfail | 2 +- source4/dsdb/tests/python/dirsync.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/selftest/knownfail b/selftest/knownfail index f9ca4984176..a89616c1dbe 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -151,7 +151,7 @@ ^samba4.smb2.acls.*.inheritflags ^samba4.smb2.acls.*.owner ^samba4.smb2.acls.*.ACCESSBASED -^samba4.ldap.dirsync.python.ad_dc_ntvfs..__main__.ExtendedDirsyncTests.test_dirsync_deleted_items +^samba4.ldap.dirsync.python.ad_dc_ntvfs..__main__.SimpleDirsyncTests.test_dirsync_deleted_items_OBJECT_SECURITY #^samba4.ldap.dirsync.python.ad_dc_ntvfs..__main__.ExtendedDirsyncTests.* ^samba4.libsmbclient.opendir.(NT1|SMB3).opendir # This requires netbios browsing ^samba4.rpc.drsuapi.*.drsuapi.DsGetDomainControllerInfo\(.*\)$ diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index ca0947e2d21..ad136b7efee 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -458,7 +458,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertTrue(res[0].get("name") is not None) delete_force(self.ldb_admin, ouname) - def test_dirsync_linkedattributes(self): + def test_dirsync_linkedattributes_OBJECT_SECURITY(self): """Check that dirsync returned deleted objects too""" # Let's search for members self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) @@ -585,9 +585,6 @@ class SimpleDirsyncTests(DirsyncBaseTests): expression="(&(objectClass=organizationalUnit)(!(isDeleted=*)))", controls=controls) - -class ExtendedDirsyncTests(SimpleDirsyncTests): - def test_dirsync_linkedattributes_range(self): self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) res = self.ldb_admin.search(self.base_dn, @@ -711,7 +708,7 @@ class ExtendedDirsyncTests(SimpleDirsyncTests): self.assertIn(b"; Date: Mon, 7 Aug 2023 13:15:40 +1200 Subject: [PATCH 20/30] CVE-2023-4154 dsdb/tests: Use self.addCleanup() and delete_force() Thie helps ensure this test is reliable even in spite of errors while running. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/confidential_attr.py | 6 ++---- source4/dsdb/tests/python/dirsync.py | 6 +----- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index 8ca56bd1023..3997848f8f9 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -98,7 +98,9 @@ class ConfidentialAttrCommon(samba.tests.TestCase): userou = "OU=conf-attr-test" self.ou = "{0},{1}".format(userou, self.base_dn) + samba.tests.delete_force(self.ldb_admin, self.ou, controls=['tree_delete:1']) self.ldb_admin.create_ou(self.ou) + self.addCleanup(samba.tests.delete_force, self.ldb_admin, self.ou, controls=['tree_delete:1']) # use a common username prefix, so we can use sAMAccountName=CATC-* as # a search filter to only return the users we're interested in @@ -139,10 +141,6 @@ class ConfidentialAttrCommon(samba.tests.TestCase): "{0} searchFlags already {1}".format(self.conf_attr, search_flags)) - def tearDown(self): - super(ConfidentialAttrCommon, self).tearDown() - self.ldb_admin.delete(self.ou, ["tree_delete:1"]) - def add_attr(self, dn, attr, value): m = Message() m.dn = Dn(self.ldb_admin, dn) diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index ad136b7efee..e06b85bc749 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -137,10 +137,6 @@ class SimpleDirsyncTests(DirsyncBaseTests): if self.ouname: delete_force(self.ldb_admin, self.ouname) self.sd_utils.modify_sd_on_dn(self.base_dn, self.desc_sddl) - try: - self.ldb_admin.deletegroup("testgroup") - except Exception: - pass # def test_dirsync_errors(self): @@ -499,6 +495,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(len(res[0].get("member")), size) self.ldb_admin.newgroup("testgroup") + self.addCleanup(self.ldb_admin.deletegroup, "testgroup") self.ldb_admin.add_remove_group_members("testgroup", [self.simple_user], add_members_operation=True) @@ -537,7 +534,6 @@ class SimpleDirsyncTests(DirsyncBaseTests): attrs=["member"], controls=[control1]) - self.ldb_admin.deletegroup("testgroup") self.assertEqual(len(res[0].get("member")), 0) def test_dirsync_deleted_items(self): -- 2.34.1 From c18f819f8ce285e014cfb51279e144eb4d141d9e Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 14:44:28 +1200 Subject: [PATCH 21/30] CVE-2023-4154 dsdb/tests: Force the test attribute to be not-confidential at the start Rather than fail, if the last run failed to reset things, just force the DC into the required state. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/confidential_attr.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index 3997848f8f9..ee7f554a008 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -136,10 +136,12 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # sanity-check the flag is not already set (this'll cause problems if # previous test run didn't clean up properly) - search_flags = self.get_attr_search_flags(self.attr_dn) - self.assertEqual(0, int(search_flags) & SEARCH_FLAG_CONFIDENTIAL, - "{0} searchFlags already {1}".format(self.conf_attr, - search_flags)) + search_flags = int(self.get_attr_search_flags(self.attr_dn)) + if search_flags & SEARCH_FLAG_CONFIDENTIAL: + self.set_attr_search_flags(self.attr_dn, str(search_flags &~ SEARCH_FLAG_CONFIDENTIAL)) + search_flags = int(self.get_attr_search_flags(self.attr_dn)) + self.assertEqual(0, search_flags & SEARCH_FLAG_CONFIDENTIAL, + f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL ({search_flags})") def add_attr(self, dn, attr, value): m = Message() -- 2.34.1 From e0cec7f7908ecbdd6a2d9785352279416cae1ece Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 11:56:56 +1200 Subject: [PATCH 22/30] CVE-2023-4154 dsdb/tests: Check that secret attributes are not visible with DirSync ever. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/dirsync | 1 + source4/dsdb/tests/python/dirsync.py | 12 ++++++++++++ 2 files changed, 13 insertions(+) create mode 100644 selftest/knownfail.d/dirsync diff --git a/selftest/knownfail.d/dirsync b/selftest/knownfail.d/dirsync new file mode 100644 index 00000000000..9367f92e109 --- /dev/null +++ b/selftest/knownfail.d/dirsync @@ -0,0 +1 @@ +^samba4.ldap.dirsync.python\(.*\).__main__.SimpleDirsyncTests.test_dirsync_unicodePwd \ No newline at end of file diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index e06b85bc749..2cacaf01251 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -742,6 +742,18 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(guid2, guid) self.assertEqual(str(res[0].dn), "") + def test_dirsync_unicodePwd(self): + res = self.ldb_admin.search(self.base_dn, + attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], + expression="(samAccountName=krbtgt)", + controls=["dirsync:1:0:0"]) + + self.assertTrue(len(res) == 1) + # This form ensures this is a case insensitive comparison + self.assertTrue("samAccountName" in res[0]) + self.assertTrue(res[0].get("samAccountName")) + self.assertTrue(res[0].get("unicodePwd") is None) + self.assertTrue(res[0].get("supplementalCredentials") is None) if not getattr(opts, "listtests", False): lp = sambaopts.get_loadparm() -- 2.34.1 From d30349ac4cfa27c5950c54b7c083cb2c53300a0f Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 11:18:46 +1200 Subject: [PATCH 23/30] CVE-2023-4154 dsdb/tests: Speed up DirSync test by only checking positive matches once When we (expect to) get back a result, do not waste time against a potentially slow server confirming we also get back results for all the other attribute combinations. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/confidential_attr.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index ee7f554a008..678a5a82948 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -742,7 +742,13 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): # want to weed out results from any previous test runs search = "(&{0}{1})".format(expr, self.extra_filter) - for attr in self.attr_filters: + # If we expect to return multiple results, only check the first + if expected_num > 0: + attr_filters = [self.attr_filters[0]] + else: + attr_filters = self.attr_filters + + for attr in attr_filters: res = samdb.search(base_dn, expression=search, scope=SCOPE_SUBTREE, attrs=attr, controls=self.dirsync) self.assertEqual(len(res), expected_num, -- 2.34.1 From b586f8cc9c797b3dd89d32d12921e2820dbcf1ce Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 14:30:19 +1200 Subject: [PATCH 24/30] CVE-2023-4154 dsdb/tests: Add test for SEARCH_FLAG_RODC_ATTRIBUTE behaviour SEARCH_FLAG_RODC_ATTRIBUTE should be like SEARCH_FLAG_CONFIDENTIAL, but for DirSync and DRS replication. Accounts with GUID_DRS_GET_CHANGES rights should not be able to read this attribute. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- .../dsdb/tests/python/confidential_attr.py | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index 678a5a82948..edbc9593204 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -30,13 +30,15 @@ import time from samba.tests.subunitrun import SubunitOptions, TestProgram import samba.getopt as options from ldb import SCOPE_BASE, SCOPE_SUBTREE -from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_PRESERVEONDELETE +from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_RODC_ATTRIBUTE, SEARCH_FLAG_PRESERVEONDELETE from ldb import Message, MessageElement, Dn from ldb import FLAG_MOD_REPLACE, FLAG_MOD_ADD from samba.auth import system_session from samba import gensec, sd_utils from samba.samdb import SamDB from samba.credentials import Credentials, DONT_USE_KERBEROS +from samba.dcerpc import security + import samba.tests import samba.dsdb @@ -137,11 +139,11 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # sanity-check the flag is not already set (this'll cause problems if # previous test run didn't clean up properly) search_flags = int(self.get_attr_search_flags(self.attr_dn)) - if search_flags & SEARCH_FLAG_CONFIDENTIAL: - self.set_attr_search_flags(self.attr_dn, str(search_flags &~ SEARCH_FLAG_CONFIDENTIAL)) + if search_flags & SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE: + self.set_attr_search_flags(self.attr_dn, str(search_flags &~ (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE))) search_flags = int(self.get_attr_search_flags(self.attr_dn)) - self.assertEqual(0, search_flags & SEARCH_FLAG_CONFIDENTIAL, - f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL ({search_flags})") + self.assertEqual(0, search_flags & (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE), + f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL and SEARCH_FLAG_RODC_ATTRIBUTE ({search_flags})") def add_attr(self, dn, attr, value): m = Message() @@ -1098,5 +1100,38 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): user_matching, user_non_matching = time_searches(self.ldb_user) assertRangesOverlap(user_matching, user_non_matching) +# Check that using the dirsync controls doesn't reveal confidential +# "RODC filtered attribute" values to users with only +# GUID_DRS_GET_CHANGES. The tests is so similar to the Confidential +# attribute test we base it on that. +class RodcFilteredAttrDirsync(ConfidentialAttrTestDirsync): + + def setUp(self): + super().setUp() + self.dirsync = ["dirsync:1:0:1000"] + + user_sid = self.sd_utils.get_object_sid(self.get_user_dn(self.user)) + mod = "(OA;;CR;%s;;%s)" % (security.GUID_DRS_GET_CHANGES, + str(user_sid)) + self.sd_utils.dacl_add_ace(self.base_dn, mod) + + self.ldb_user = self.get_ldb_connection(self.user, self.user_pass) + + self.addCleanup(self.sd_utils.dacl_delete_aces, self.base_dn, mod) + + def make_attr_confidential(self): + """Marks the attribute under test as being confidential AND RODC + filtered (which should mean it is not visible with only + GUID_DRS_GET_CHANGES) + """ + + # work out the original 'searchFlags' value before we overwrite it + old_value = self.get_attr_search_flags(self.attr_dn) + + self.set_attr_search_flags(self.attr_dn, str(SEARCH_FLAG_RODC_ATTRIBUTE|SEARCH_FLAG_CONFIDENTIAL)) + + # reset the value after the test completes + self.addCleanup(self.set_attr_search_flags, self.attr_dn, old_value) + TestProgram(module=__name__, opts=subunitopts) -- 2.34.1 From 4c897f5b8542ad29b51ffc9eb219fcb9eaf7754b Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 22 Aug 2023 15:08:17 +1200 Subject: [PATCH 25/30] CVE-2023-4154 dsdb/tests: Extend attribute read DirSync tests The aim here is to document the expected (even if not implemented) SEARCH_FLAG_RODC_ATTRIBUTE vs SEARCH_FLAG_CONFIDENTIAL, behaviour, so that any change once CVE-2023-4154 is fixed can be noted. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/dirsync | 15 +- source4/dsdb/tests/python/dirsync.py | 456 +++++++++++++++++++++++---- 2 files changed, 414 insertions(+), 57 deletions(-) diff --git a/selftest/knownfail.d/dirsync b/selftest/knownfail.d/dirsync index 9367f92e109..db098549a08 100644 --- a/selftest/knownfail.d/dirsync +++ b/selftest/knownfail.d/dirsync @@ -1 +1,14 @@ -^samba4.ldap.dirsync.python\(.*\).__main__.SimpleDirsyncTests.test_dirsync_unicodePwd \ No newline at end of file +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_OBJ_SEC_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_with_GET_CHANGES_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_attr\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES_attr\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES_insist_on_empty_element\(.*\) diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index 2cacaf01251..a0691f0afe0 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -3,6 +3,7 @@ # Unit tests for dirsync control # Copyright (C) Matthieu Patou 2011 # Copyright (C) Jelmer Vernooij 2014 +# Copyright (C) Catalyst.Net Ltd # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -30,7 +31,8 @@ import base64 import ldb from ldb import LdbError, SCOPE_BASE from ldb import Message, MessageElement, Dn -from ldb import FLAG_MOD_ADD, FLAG_MOD_DELETE +from ldb import FLAG_MOD_ADD, FLAG_MOD_DELETE, FLAG_MOD_REPLACE +from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_RODC_ATTRIBUTE from samba.dcerpc import security, misc, drsblobs from samba.ndr import ndr_unpack, ndr_pack @@ -60,7 +62,6 @@ if len(args) < 1: host = args.pop() if "://" not in host: ldaphost = "ldap://%s" % host - ldapshost = "ldaps://%s" % host else: ldaphost = host start = host.rindex("://") @@ -77,8 +78,8 @@ creds = credopts.get_credentials(lp) class DirsyncBaseTests(samba.tests.TestCase): def setUp(self): - super(DirsyncBaseTests, self).setUp() - self.ldb_admin = SamDB(ldapshost, credentials=creds, session_info=system_session(lp), lp=lp) + super().setUp() + self.ldb_admin = SamDB(ldaphost, credentials=creds, session_info=system_session(lp), lp=lp) self.base_dn = self.ldb_admin.domain_dn() self.domain_sid = security.dom_sid(self.ldb_admin.get_domain_sid()) self.user_pass = samba.generate_random_password(12, 16) @@ -87,63 +88,60 @@ class DirsyncBaseTests(samba.tests.TestCase): # used for anonymous login print("baseDN: %s" % self.base_dn) - def get_user_dn(self, name): - return "CN=%s,CN=Users,%s" % (name, self.base_dn) + userou = "OU=dirsync-test" + self.ou = f"{userou},{self.base_dn}" + samba.tests.delete_force(self.ldb_admin, self.ou, controls=['tree_delete:1']) + self.ldb_admin.create_ou(self.ou) + self.addCleanup(samba.tests.delete_force, self.ldb_admin, self.ou, controls=['tree_delete:1']) - def get_ldb_connection(self, target_username, target_password): - creds_tmp = Credentials() - creds_tmp.set_username(target_username) - creds_tmp.set_password(target_password) - creds_tmp.set_domain(creds.get_domain()) - creds_tmp.set_realm(creds.get_realm()) - creds_tmp.set_workstation(creds.get_workstation()) - creds_tmp.set_gensec_features(creds_tmp.get_gensec_features() - | gensec.FEATURE_SEAL) - creds_tmp.set_kerberos_state(DONT_USE_KERBEROS) # kinit is too expensive to use in a tight loop - ldb_target = SamDB(url=ldaphost, credentials=creds_tmp, lp=lp) - return ldb_target - - -# tests on ldap add operations -class SimpleDirsyncTests(DirsyncBaseTests): - - def setUp(self): - super(SimpleDirsyncTests, self).setUp() # Regular user self.dirsync_user = "test_dirsync_user" self.simple_user = "test_simple_user" self.admin_user = "test_admin_user" - self.ouname = None + self.dirsync_pass = self.user_pass + self.simple_pass = self.user_pass + self.admin_pass = self.user_pass - self.ldb_admin.newuser(self.dirsync_user, self.user_pass) - self.ldb_admin.newuser(self.simple_user, self.user_pass) - self.ldb_admin.newuser(self.admin_user, self.user_pass) + self.ldb_admin.newuser(self.dirsync_user, self.dirsync_pass, userou=userou) + self.ldb_admin.newuser(self.simple_user, self.simple_pass, userou=userou) + self.ldb_admin.newuser(self.admin_user, self.admin_pass, userou=userou) self.desc_sddl = self.sd_utils.get_sd_as_sddl(self.base_dn) user_sid = self.sd_utils.get_object_sid(self.get_user_dn(self.dirsync_user)) mod = "(OA;;CR;%s;;%s)" % (security.GUID_DRS_GET_CHANGES, str(user_sid)) self.sd_utils.dacl_add_ace(self.base_dn, mod) + self.addCleanup(self.sd_utils.dacl_delete_aces, self.base_dn, mod) # add admins to the Domain Admins group self.ldb_admin.add_remove_group_members("Domain Admins", [self.admin_user], add_members_operation=True) - def tearDown(self): - super(SimpleDirsyncTests, self).tearDown() - delete_force(self.ldb_admin, self.get_user_dn(self.dirsync_user)) - delete_force(self.ldb_admin, self.get_user_dn(self.simple_user)) - delete_force(self.ldb_admin, self.get_user_dn(self.admin_user)) - if self.ouname: - delete_force(self.ldb_admin, self.ouname) - self.sd_utils.modify_sd_on_dn(self.base_dn, self.desc_sddl) + def get_user_dn(self, name): + return ldb.Dn(self.ldb_admin, "CN={0},{1}".format(name, self.ou)) + + def get_ldb_connection(self, target_username, target_password): + creds_tmp = Credentials() + creds_tmp.set_username(target_username) + creds_tmp.set_password(target_password) + creds_tmp.set_domain(creds.get_domain()) + creds_tmp.set_realm(creds.get_realm()) + creds_tmp.set_workstation(creds.get_workstation()) + creds_tmp.set_gensec_features(creds_tmp.get_gensec_features() + | gensec.FEATURE_SEAL) + creds_tmp.set_kerberos_state(DONT_USE_KERBEROS) # kinit is too expensive to use in a tight loop + ldb_target = SamDB(url=ldaphost, credentials=creds_tmp, lp=lp) + return ldb_target + +# tests on ldap add operations +class SimpleDirsyncTests(DirsyncBaseTests): # def test_dirsync_errors(self): def test_dirsync_supported(self): """Test the basic of the dirsync is supported""" self.ldb_dirsync = self.get_ldb_connection(self.dirsync_user, self.user_pass) - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_admin.search(self.base_dn, expression="samaccountname=*", controls=["dirsync:1:0:1"]) res = self.ldb_dirsync.search(self.base_dn, expression="samaccountname=*", controls=["dirsync:1:0:1"]) try: @@ -169,8 +167,8 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_errors(self): """Test if dirsync returns the correct LDAP errors in case of pb""" - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) - self.ldb_dirsync = self.get_ldb_connection(self.dirsync_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) + self.ldb_dirsync = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) try: self.ldb_simple.search(self.base_dn, expression="samaccountname=*", @@ -292,11 +290,11 @@ class SimpleDirsyncTests(DirsyncBaseTests): attrs=["parentGUID"], controls=["dirsync:1:0:1"]) self.assertEqual(len(res.msgs), 0) - ouname = "OU=testou,%s" % self.base_dn + ouname = "OU=testou,%s" % self.ou self.ouname = ouname self.ldb_admin.create_ou(ouname) delta = Message() - delta.dn = Dn(self.ldb_admin, str(ouname)) + delta.dn = Dn(self.ldb_admin, ouname) delta["cn"] = MessageElement("test ou", FLAG_MOD_ADD, "cn") @@ -457,7 +455,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_linkedattributes_OBJECT_SECURITY(self): """Check that dirsync returned deleted objects too""" # Let's search for members - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_simple.search(self.base_dn, expression="(name=Administrators)", controls=["dirsync:1:1:1"]) @@ -582,7 +580,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): controls=controls) def test_dirsync_linkedattributes_range(self): - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_admin.search(self.base_dn, attrs=["member;range=1-1"], expression="(name=Administrators)", @@ -594,7 +592,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertTrue(len(res[0].get("member")) > 0) def test_dirsync_linkedattributes_range_user(self): - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) try: res = self.ldb_simple.search(self.base_dn, attrs=["member;range=1-1"], @@ -608,7 +606,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_linkedattributes(self): flag_incr_linked = 2147483648 - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_admin.search(self.base_dn, attrs=["member"], expression="(name=Administrators)", @@ -676,7 +674,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_extended_dn(self): """Check that dirsync works together with the extended_dn control""" # Let's search for members - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_simple.search(self.base_dn, expression="(name=Administrators)", controls=["dirsync:1:1:1"]) @@ -707,7 +705,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_deleted_items_OBJECT_SECURITY(self): """Check that dirsync returned deleted objects too""" # Let's create an OU - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) ouname = "OU=testou3,%s" % self.base_dn self.ouname = ouname self.ldb_admin.create_ou(ouname) @@ -742,18 +740,364 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(guid2, guid) self.assertEqual(str(res[0].dn), "") - def test_dirsync_unicodePwd(self): +class SpecialDirsyncTests(DirsyncBaseTests): + + def setUp(self): + super().setUp() + + self.schema_dn = self.ldb_admin.get_schema_basedn() + + # the tests work by setting the 'Confidential' or 'RODC Filtered' bit in the searchFlags + # for an existing schema attribute. This only works against Windows if + # the systemFlags does not have FLAG_SCHEMA_BASE_OBJECT set for the + # schema attribute being modified. There are only a few attributes that + # meet this criteria (most of which only apply to 'user' objects) + self.conf_attr = "homePostalAddress" + attr_cn = "CN=Address-Home" + # schemaIdGuid for homePostalAddress (used for ACE tests) + self.attr_dn = f"{attr_cn},{self.schema_dn}" + + userou = "OU=conf-attr-test" + self.ou = "{0},{1}".format(userou, self.base_dn) + samba.tests.delete_force(self.ldb_admin, self.ou, controls=['tree_delete:1']) + self.ldb_admin.create_ou(self.ou) + self.addCleanup(samba.tests.delete_force, self.ldb_admin, self.ou, controls=['tree_delete:1']) + + # add a test object with this attribute set + self.conf_value = "abcdef" + self.conf_user = "conf-user" + self.ldb_admin.newuser(self.conf_user, self.user_pass, userou=userou) + self.conf_dn = self.get_user_dn(self.conf_user) + self.add_attr(self.conf_dn, self.conf_attr, self.conf_value) + + # sanity-check the flag is not already set (this'll cause problems if + # previous test run didn't clean up properly) + + search_flags = int(self.get_attr_search_flags(self.attr_dn)) + if search_flags & SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE: + self.set_attr_search_flags(self.attr_dn, str(search_flags &~ (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE))) + search_flags = int(self.get_attr_search_flags(self.attr_dn)) + self.assertEqual(0, search_flags & (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE), + f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL and SEARCH_FLAG_RODC_ATTRIBUTE ({search_flags})") + + # work out the original 'searchFlags' value before we overwrite it + old_value = self.get_attr_search_flags(self.attr_dn) + + self.set_attr_search_flags(self.attr_dn, str(self.flag_under_test)) + + # reset the value after the test completes + self.addCleanup(self.set_attr_search_flags, self.attr_dn, old_value) + + def add_attr(self, dn, attr, value): + m = Message() + m.dn = dn + m[attr] = MessageElement(value, FLAG_MOD_ADD, attr) + self.ldb_admin.modify(m) + + def set_attr_search_flags(self, attr_dn, flags): + """Modifies the searchFlags for an object in the schema""" + m = Message() + m.dn = Dn(self.ldb_admin, attr_dn) + m['searchFlags'] = MessageElement(flags, FLAG_MOD_REPLACE, + 'searchFlags') + self.ldb_admin.modify(m) + + # note we have to update the schema for this change to take effect (on + # Windows, at least) + self.ldb_admin.set_schema_update_now() + + def get_attr_search_flags(self, attr_dn): + res = self.ldb_admin.search(attr_dn, scope=SCOPE_BASE, + attrs=['searchFlags']) + return res[0]['searchFlags'][0] + + def find_under_current_ou(self, res): + for msg in res: + if msg.dn == self.conf_dn: + return msg + self.fail(f"Failed to find object {self.conf_dn} in {len(res)} results") + + +class ConfidentialDirsyncTests(SpecialDirsyncTests): + + def setUp(self): + self.flag_under_test = SEARCH_FLAG_CONFIDENTIAL + super().setUp() + + def test_unicodePwd_normal(self): res = self.ldb_admin.search(self.base_dn, attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], - expression="(samAccountName=krbtgt)", - controls=["dirsync:1:0:0"]) + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get("unicodePwd") is None) + self.assertTrue(msg.get("supplementalCredentials") is None) + + def _test_dirsync_unicodePwd(self, ldb_conn, control=None, insist_on_empty_element=False): + res = ldb_conn.search(self.base_dn, + attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=[control]) + + msg = self.find_under_current_ou(res) - self.assertTrue(len(res) == 1) + self.assertTrue("samAccountName" in msg) # This form ensures this is a case insensitive comparison - self.assertTrue("samAccountName" in res[0]) - self.assertTrue(res[0].get("samAccountName")) - self.assertTrue(res[0].get("unicodePwd") is None) - self.assertTrue(res[0].get("supplementalCredentials") is None) + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get("unicodePwd") is not None) + self.assertEqual(len(msg.get("unicodePwd")), 0) + self.assertTrue(msg.get("supplementalCredentials") is not None) + self.assertEqual(len(msg.get("supplementalCredentials")), 0) + else: + self.assertTrue(msg.get("unicodePwd") is None + or len(msg.get("unicodePwd")) == 0) + self.assertTrue(msg.get("supplementalCredentials") is None + or len(msg.get("supplementalCredentials")) == 0) + + def test_dirsync_unicodePwd_OBJ_SEC(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0") + + def test_dirsync_unicodePwd_OBJ_SEC_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0", insist_on_empty_element=True) + + def test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0") + + def test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0", insist_on_empty_element=True) + + def test_dirsync_unicodePwd_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:0:0") + + def test_dirsync_unicodePwd_with_GET_CHANGES_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:0:0", insist_on_empty_element=True) + + def test_normal(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr) is None) + + def _test_dirsync_OBJECT_SECURITY(self, ldb_conn, insist_on_empty_element=False): + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:1:0"]) + + msg = self.find_under_current_ou(res) + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + + def test_dirsync_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + + msg = self.find_under_current_ou(res) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr)) + self.assertEqual(len(msg.get(self.conf_attr)), 1) + + def test_dirsync_with_GET_CHANGES_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_with_GET_CHANGES_OBJECT_SECURITY_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + +class FilteredDirsyncTests(SpecialDirsyncTests): + + def setUp(self): + self.flag_under_test = SEARCH_FLAG_RODC_ATTRIBUTE + super().setUp() + + def test_attr(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr)) + self.assertEqual(len(msg.get(self.conf_attr)), 1) + + def _test_dirsync_OBJECT_SECURITY(self, ldb_conn): + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:1:0"]) + + msg = self.find_under_current_ou(res) + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr)) + self.assertEqual(len(msg.get(self.conf_attr)), 1) + + def test_dirsync_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def _test_dirsync_with_GET_CHANGES(self, insist_on_empty_element=False): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + res = ldb_conn.search(self.base_dn, + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + + msg = self.find_under_current_ou(res) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_with_GET_CHANGES(self): + self._test_dirsync_with_GET_CHANGES() + + def test_dirsync_with_GET_CHANGES_insist_on_empty_element(self): + self._test_dirsync_with_GET_CHANGES(insist_on_empty_element=True) + + def test_dirsync_with_GET_CHANGES_attr(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + try: + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + self.fail("ldb.search() should have failed with LDAP_INSUFFICIENT_ACCESS_RIGHTS") + except ldb.LdbError as e: + (errno, errstr) = e.args + self.assertEqual(errno, ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS) + +class ConfidentialFilteredDirsyncTests(SpecialDirsyncTests): + + def setUp(self): + self.flag_under_test = SEARCH_FLAG_RODC_ATTRIBUTE|SEARCH_FLAG_CONFIDENTIAL + super().setUp() + + def test_attr(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + res = ldb_conn.search(self.base_dn, + attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr) is None) + + def _test_dirsync_OBJECT_SECURITY(self, ldb_conn, insist_on_empty_element=False): + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:1:0"]) + + msg = self.find_under_current_ou(res) + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + + def test_dirsync_OBJECT_SECURITY_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_with_GET_CHANGES_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + + def _test_dirsync_with_GET_CHANGES(self, insist_on_empty_element=False): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + res = ldb_conn.search(self.base_dn, + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + + msg = self.find_under_current_ou(res) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_with_GET_CHANGES(self): + self._test_dirsync_with_GET_CHANGES() + + def test_dirsync_with_GET_CHANGES_insist_on_empty_element(self): + self._test_dirsync_with_GET_CHANGES(insist_on_empty_element=True) + + def test_dirsync_with_GET_CHANGES_attr(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + try: + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + self.fail("ldb.search() should have failed with LDAP_INSUFFICIENT_ACCESS_RIGHTS") + except ldb.LdbError as e: + (errno, errstr) = e.args + self.assertEqual(errno, ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS) + if not getattr(opts, "listtests", False): lp = sambaopts.get_loadparm() -- 2.34.1 From 8f87277b4e926035d825e81c4f8381d917e9d229 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 17:58:27 +1200 Subject: [PATCH 26/30] CVE-2023-4154: Unimplement the original DirSync behaviour without LDAP_DIRSYNC_OBJECT_SECURITY This makes LDAP_DIRSYNC_OBJECT_SECURITY the only behaviour provided by Samba. Having a second access control system withing the LDAP stack is unsafe and this layer is incomplete. The current system gives all accounts that have been given the GUID_DRS_GET_CHANGES extended right SYSTEM access. Currently in Samba this equates to full access to passwords as well as "RODC Filtered attributes" (often used with confidential attributes). Rather than attempting to correctly filter for secrets (passwords) and these filtered attributes, as well as preventing search expressions for both, we leave this complexity to the acl_read module which has this facility already well tested. The implication is that callers will only see and filter by attribute in DirSync that they could without DirSync. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/dirsync | 3 +-- source4/dsdb/samdb/ldb_modules/dirsync.c | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/selftest/knownfail.d/dirsync b/selftest/knownfail.d/dirsync index db098549a08..fcf4d469d6e 100644 --- a/selftest/knownfail.d/dirsync +++ b/selftest/knownfail.d/dirsync @@ -1,12 +1,11 @@ ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_OBJ_SEC_insist_on_empty_element\(.*\) -^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_with_GET_CHANGES_insist_on_empty_element\(.*\) -^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_attr\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) diff --git a/source4/dsdb/samdb/ldb_modules/dirsync.c b/source4/dsdb/samdb/ldb_modules/dirsync.c index b3c463741c8..fbb75790095 100644 --- a/source4/dsdb/samdb/ldb_modules/dirsync.c +++ b/source4/dsdb/samdb/ldb_modules/dirsync.c @@ -56,7 +56,6 @@ struct dirsync_context { bool linkIncrVal; bool localonly; bool partial; - bool assystem; int functional_level; const struct GUID *our_invocation_id; const struct dsdb_schema *schema; @@ -872,10 +871,6 @@ static int dirsync_search_callback(struct ldb_request *req, struct ldb_reply *ar DSDB_SEARCH_SHOW_DELETED | DSDB_SEARCH_SHOW_EXTENDED_DN; - if (dsc->assystem) { - flags = flags | DSDB_FLAG_AS_SYSTEM; - } - ret = dsdb_module_search_tree(dsc->module, dsc, &res, dn, LDB_SCOPE_BASE, req->op.search.tree, @@ -1102,16 +1097,21 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req return LDB_ERR_OPERATIONS_ERROR; } objectclass = dsdb_get_structural_oc_from_msg(schema, acl_res->msgs[0]); + + /* + * While we never use the answer to this for access + * control (after CVE-2023-4154), we return a + * different error message depending on if the user + * was granted GUID_DRS_GET_CHANGES to provide a closer + * emulation and keep some tests passing. + * + * (Samba's ACL logic is not well suited to redacting + * only the secret and RODC filtered attributes). + */ ret = acl_check_extended_right(dsc, module, req, objectclass, sd, acl_user_token(module), GUID_DRS_GET_CHANGES, SEC_ADS_CONTROL_ACCESS, sid); - if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) { - return ret; - } - dsc->assystem = true; - ret = ldb_request_add_control(req, LDB_CONTROL_AS_SYSTEM_OID, false, NULL); - if (ret != LDB_SUCCESS) { return ret; } -- 2.34.1 From a16b210ec651b535b43c21574ca439238e2f8772 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 18:59:44 +1200 Subject: [PATCH 27/30] CVE-2023-42669 s4-rpc_server: Disable rpcecho server by default The rpcecho server is useful in development and testing, but should never have been allowed into production, as it includes the facility to do a blocking sleep() in the single-threaded rpc worker. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15474 Signed-off-by: Andrew Bartlett --- docs-xml/smbdotconf/protocol/dcerpcendpointservers.xml | 2 +- lib/param/loadparm.c | 2 +- selftest/target/Samba4.pm | 2 +- source3/param/loadparm.c | 2 +- source4/rpc_server/wscript_build | 3 ++- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs-xml/smbdotconf/protocol/dcerpcendpointservers.xml b/docs-xml/smbdotconf/protocol/dcerpcendpointservers.xml index 8a217cc7f11..c6642b795fd 100644 --- a/docs-xml/smbdotconf/protocol/dcerpcendpointservers.xml +++ b/docs-xml/smbdotconf/protocol/dcerpcendpointservers.xml @@ -6,6 +6,6 @@ Specifies which DCE/RPC endpoint servers should be run. -epmapper, wkssvc, rpcecho, samr, netlogon, lsarpc, drsuapi, dssetup, unixinfo, browser, eventlog6, backupkey, dnsserver +epmapper, wkssvc, samr, netlogon, lsarpc, drsuapi, dssetup, unixinfo, browser, eventlog6, backupkey, dnsserver rpcecho diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c index f70823fe366..664fae70c9b 100644 --- a/lib/param/loadparm.c +++ b/lib/param/loadparm.c @@ -2732,7 +2732,7 @@ struct loadparm_context *loadparm_init(TALLOC_CTX *mem_ctx) lpcfg_do_global_parameter(lp_ctx, "ntvfs handler", "unixuid default"); lpcfg_do_global_parameter(lp_ctx, "max connections", "0"); - lpcfg_do_global_parameter(lp_ctx, "dcerpc endpoint servers", "epmapper wkssvc rpcecho samr netlogon lsarpc drsuapi dssetup unixinfo browser eventlog6 backupkey dnsserver"); + lpcfg_do_global_parameter(lp_ctx, "dcerpc endpoint servers", "epmapper wkssvc samr netlogon lsarpc drsuapi dssetup unixinfo browser eventlog6 backupkey dnsserver"); lpcfg_do_global_parameter(lp_ctx, "server services", "s3fs rpc nbt wrepl ldap cldap kdc drepl winbindd ntp_signd kcc dnsupdate dns"); lpcfg_do_global_parameter(lp_ctx, "kccsrv:samba_kcc", "true"); /* the winbind method for domain controllers is for both RODC diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm index 5cbc5ccf2b8..7033146f46a 100755 --- a/selftest/target/Samba4.pm +++ b/selftest/target/Samba4.pm @@ -782,7 +782,7 @@ sub provision_raw_step1($$) wins support = yes server role = $ctx->{server_role} server services = +echo $services - dcerpc endpoint servers = +winreg +srvsvc + dcerpc endpoint servers = +winreg +srvsvc +rpcecho notify:inotify = false ldb:nosync = true ldap server require strong auth = yes diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c index 97d02037a89..1f7b6d16a78 100644 --- a/source3/param/loadparm.c +++ b/source3/param/loadparm.c @@ -883,7 +883,7 @@ static void init_globals(struct loadparm_context *lp_ctx, bool reinit_globals) Globals.server_services = str_list_make_v3_const(NULL, "s3fs rpc nbt wrepl ldap cldap kdc drepl winbindd ntp_signd kcc dnsupdate dns", NULL); - Globals.dcerpc_endpoint_servers = str_list_make_v3_const(NULL, "epmapper wkssvc rpcecho samr netlogon lsarpc drsuapi dssetup unixinfo browser eventlog6 backupkey dnsserver", NULL); + Globals.dcerpc_endpoint_servers = str_list_make_v3_const(NULL, "epmapper wkssvc samr netlogon lsarpc drsuapi dssetup unixinfo browser eventlog6 backupkey dnsserver", NULL); Globals.tls_enabled = true; Globals.tls_verify_peer = TLS_VERIFY_PEER_AS_STRICT_AS_POSSIBLE; diff --git a/source4/rpc_server/wscript_build b/source4/rpc_server/wscript_build index 0e44a3c2bae..31ec4f60c9a 100644 --- a/source4/rpc_server/wscript_build +++ b/source4/rpc_server/wscript_build @@ -33,7 +33,8 @@ bld.SAMBA_MODULE('dcerpc_rpcecho', source='echo/rpc_echo.c', subsystem='dcerpc_server', init_function='dcerpc_server_rpcecho_init', - deps='ndr-standard events' + deps='ndr-standard events', + enabled=bld.CONFIG_GET('ENABLE_SELFTEST') ) -- 2.34.1 From d4d49635247ab4bc580899d7c5fb54484b806225 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 19:01:03 +1200 Subject: [PATCH 28/30] CVE-2023-42669 s3-rpc_server: Disable rpcecho for consistency with the AD DC The rpcecho server in source3 does have samba the sleep() feature that the s4 version has, but the task architecture is different, so there is not the same impact. Hoever equally this is not something that should be enabled on production builds of Samba, so restrict to selftest builds. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15474 Signed-off-by: Andrew Bartlett --- source3/rpc_server/wscript_build | 1 + 1 file changed, 1 insertion(+) diff --git a/source3/rpc_server/wscript_build b/source3/rpc_server/wscript_build index 341df41a321..5ed81283395 100644 --- a/source3/rpc_server/wscript_build +++ b/source3/rpc_server/wscript_build @@ -38,6 +38,7 @@ bld.SAMBA3_BINARY('rpcd_rpcecho', RPC_WORKER RPC_RPCECHO ''', + for_selftest=True, install_path='${SAMBA_LIBEXECDIR}') bld.SAMBA3_BINARY('rpcd_classic', -- 2.34.1 From 51bc79f85a8d63ed5428c2975f60094157dda2e5 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 12:28:49 +1200 Subject: [PATCH 29/30] CVE-2023-42670 s3-rpc_server: Strictly refuse to start RPC servers in conflict with AD DC Just as we refuse to start NETLOGON except on the DC, we must refuse to start all of the RPC services that are provided by the AD DC. Most critically of course this applies to netlogon, lsa and samr. This avoids the supression of these services being the result of a runtime epmapper lookup, as if that fails these services can disrupt service to end users by listening on the same socket as the AD DC servers. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15473 Signed-off-by: Andrew Bartlett --- source3/rpc_server/rpcd_classic.c | 45 ++++++++++++++++++++++++++---- source3/rpc_server/rpcd_epmapper.c | 33 ++++++++++++++++++++-- source3/rpc_server/rpcd_lsad.c | 21 ++++++++++++++ source3/rpc_server/rpcd_rpcecho.c | 33 ++++++++++++++++++++-- 4 files changed, 122 insertions(+), 10 deletions(-) diff --git a/source3/rpc_server/rpcd_classic.c b/source3/rpc_server/rpcd_classic.c index 4f6164c814c..8494af575ec 100644 --- a/source3/rpc_server/rpcd_classic.c +++ b/source3/rpc_server/rpcd_classic.c @@ -42,14 +42,34 @@ static size_t classic_interfaces( static const struct ndr_interface_table *ifaces[] = { &ndr_table_srvsvc, &ndr_table_netdfs, - &ndr_table_wkssvc, + &ndr_table_initshutdown, &ndr_table_svcctl, &ndr_table_ntsvcs, &ndr_table_eventlog, - &ndr_table_initshutdown, + /* + * This last item is truncated from the list by the + * num_ifaces -= 1 below. Take care when adding new + * services. + */ + &ndr_table_wkssvc, }; + size_t num_ifaces = ARRAY_SIZE(ifaces); + + switch(lp_server_role()) { + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * On the AD DC wkssvc is provided by the 'samba' + * binary from source4/ + */ + num_ifaces -= 1; + break; + default: + break; + } + *pifaces = ifaces; - return ARRAY_SIZE(ifaces); + return num_ifaces; + } static size_t classic_servers( @@ -58,15 +78,28 @@ static size_t classic_servers( void *private_data) { static const struct dcesrv_endpoint_server *ep_servers[7] = { NULL }; + size_t num_servers = ARRAY_SIZE(ep_servers); bool ok; ep_servers[0] = srvsvc_get_ep_server(); ep_servers[1] = netdfs_get_ep_server(); - ep_servers[2] = wkssvc_get_ep_server(); + ep_servers[2] = initshutdown_get_ep_server(); ep_servers[3] = svcctl_get_ep_server(); ep_servers[4] = ntsvcs_get_ep_server(); ep_servers[5] = eventlog_get_ep_server(); - ep_servers[6] = initshutdown_get_ep_server(); + ep_servers[6] = wkssvc_get_ep_server(); + + switch(lp_server_role()) { + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * On the AD DC wkssvc is provided by the 'samba' + * binary from source4/ + */ + num_servers -= 1; + break; + default: + break; + } ok = secrets_init(); if (!ok) { @@ -85,7 +118,7 @@ static size_t classic_servers( mangle_reset_cache(); *_ep_servers = ep_servers; - return ARRAY_SIZE(ep_servers); + return num_servers; } int main(int argc, const char *argv[]) diff --git a/source3/rpc_server/rpcd_epmapper.c b/source3/rpc_server/rpcd_epmapper.c index 950ba7ec12a..455179ccfba 100644 --- a/source3/rpc_server/rpcd_epmapper.c +++ b/source3/rpc_server/rpcd_epmapper.c @@ -19,6 +19,8 @@ #include "rpc_worker.h" #include "librpc/gen_ndr/ndr_epmapper.h" #include "librpc/gen_ndr/ndr_epmapper_scompat.h" +#include "param/loadparm.h" +#include "libds/common/roles.h" static size_t epmapper_interfaces( const struct ndr_interface_table ***pifaces, @@ -27,8 +29,22 @@ static size_t epmapper_interfaces( static const struct ndr_interface_table *ifaces[] = { &ndr_table_epmapper, }; + size_t num_ifaces = ARRAY_SIZE(ifaces); + + switch(lp_server_role()) { + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * On the AD DC epmapper is provided by the 'samba' + * binary from source4/ + */ + num_ifaces = 0; + break; + default: + break; + } + *pifaces = ifaces; - return ARRAY_SIZE(ifaces); + return num_ifaces; } static size_t epmapper_servers( @@ -37,11 +53,24 @@ static size_t epmapper_servers( void *private_data) { static const struct dcesrv_endpoint_server *ep_servers[] = { NULL }; + size_t num_servers = ARRAY_SIZE(ep_servers); ep_servers[0] = epmapper_get_ep_server(); + switch(lp_server_role()) { + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * On the AD DC epmapper is provided by the 'samba' + * binary from source4/ + */ + num_servers = 0; + break; + default: + break; + } + *_ep_servers = ep_servers; - return ARRAY_SIZE(ep_servers); + return num_servers; } int main(int argc, const char *argv[]) diff --git a/source3/rpc_server/rpcd_lsad.c b/source3/rpc_server/rpcd_lsad.c index 3ca0ed43fdd..b0e021493e7 100644 --- a/source3/rpc_server/rpcd_lsad.c +++ b/source3/rpc_server/rpcd_lsad.c @@ -36,6 +36,11 @@ static size_t lsad_interfaces( &ndr_table_lsarpc, &ndr_table_samr, &ndr_table_dssetup, + /* + * This last item is truncated from the list by the + * num_ifaces -= 1 below for the fileserver. Take + * care when adding new services. + */ &ndr_table_netlogon, }; size_t num_ifaces = ARRAY_SIZE(ifaces); @@ -46,6 +51,14 @@ static size_t lsad_interfaces( /* no netlogon for non-dc */ num_ifaces -= 1; break; + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * All these services are provided by the 'samba' + * binary from source4, not this code which is the + * source3 / NT4-like "classic" DC implementation + */ + num_ifaces = 0; + break; default: break; } @@ -80,6 +93,14 @@ static size_t lsad_servers( /* no netlogon for non-dc */ num_servers -= 1; break; + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * All these services are provided by the 'samba' + * binary from source4, not this code which is the + * source3 / NT4-like "classic" DC implementation + */ + num_servers = 0; + break; default: break; } diff --git a/source3/rpc_server/rpcd_rpcecho.c b/source3/rpc_server/rpcd_rpcecho.c index 9176039819f..37391f563db 100644 --- a/source3/rpc_server/rpcd_rpcecho.c +++ b/source3/rpc_server/rpcd_rpcecho.c @@ -19,6 +19,8 @@ #include "rpc_worker.h" #include "librpc/gen_ndr/ndr_echo.h" #include "librpc/gen_ndr/ndr_echo_scompat.h" +#include "param/loadparm.h" +#include "libds/common/roles.h" static size_t rpcecho_interfaces( const struct ndr_interface_table ***pifaces, @@ -27,8 +29,22 @@ static size_t rpcecho_interfaces( static const struct ndr_interface_table *ifaces[] = { &ndr_table_rpcecho, }; + size_t num_ifaces = ARRAY_SIZE(ifaces); + + switch(lp_server_role()) { + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * On the AD DC rpcecho is provided by the 'samba' + * binary from source4/ + */ + num_ifaces = 0; + break; + default: + break; + } + *pifaces = ifaces; - return ARRAY_SIZE(ifaces); + return num_ifaces; } static size_t rpcecho_servers( @@ -37,11 +53,24 @@ static size_t rpcecho_servers( void *private_data) { static const struct dcesrv_endpoint_server *ep_servers[1] = { NULL }; + size_t num_servers = ARRAY_SIZE(ep_servers); ep_servers[0] = rpcecho_get_ep_server(); + switch(lp_server_role()) { + case ROLE_ACTIVE_DIRECTORY_DC: + /* + * On the AD DC rpcecho is provided by the 'samba' + * binary from source4/ + */ + num_servers = 0; + break; + default: + break; + } + *_ep_servers = ep_servers; - return ARRAY_SIZE(ep_servers); + return num_servers; } int main(int argc, const char *argv[]) -- 2.34.1 From 2acdaf9860f127c179a3d2e2adb18f901854aebf Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 16:23:49 +1200 Subject: [PATCH 30/30] CVE-2023-42670 s3-rpc_server: Remove cross-check with "samba" EPM lookup We now have ensured that no conflicting services attempt to start so we do not need the runtime lookup and so avoid the risk that the lookup may fail. This means that any duplicates will be noticed early not just in a race condition. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15473 Signed-off-by: Andrew Bartlett --- source3/rpc_server/rpc_host.c | 154 +--------------------------------- 1 file changed, 4 insertions(+), 150 deletions(-) diff --git a/source3/rpc_server/rpc_host.c b/source3/rpc_server/rpc_host.c index 321d30cef2c..810d128d958 100644 --- a/source3/rpc_server/rpc_host.c +++ b/source3/rpc_server/rpc_host.c @@ -214,7 +214,6 @@ struct rpc_server_get_endpoints_state { char **argl; char *ncalrpc_endpoint; enum dcerpc_transport_t only_transport; - struct dcerpc_binding **existing_bindings; struct rpc_host_iface_name *iface_names; struct rpc_host_endpoint **endpoints; @@ -235,7 +234,6 @@ static void rpc_server_get_endpoints_done(struct tevent_req *subreq); * @param[in] ev Event context to run this on * @param[in] rpc_server_exe Binary to ask with --list-interfaces * @param[in] only_transport Filter out anything but this - * @param[in] existing_bindings Filter out endpoints served by "samba" * @return The tevent_req representing this process */ @@ -243,8 +241,7 @@ static struct tevent_req *rpc_server_get_endpoints_send( TALLOC_CTX *mem_ctx, struct tevent_context *ev, const char *rpc_server_exe, - enum dcerpc_transport_t only_transport, - struct dcerpc_binding **existing_bindings) + enum dcerpc_transport_t only_transport) { struct tevent_req *req = NULL, *subreq = NULL; struct rpc_server_get_endpoints_state *state = NULL; @@ -256,7 +253,6 @@ static struct tevent_req *rpc_server_get_endpoints_send( return NULL; } state->only_transport = only_transport; - state->existing_bindings = existing_bindings; progname = strrchr(rpc_server_exe, '/'); if (progname != NULL) { @@ -417,37 +413,17 @@ static bool dcerpc_binding_same_endpoint( * In member mode, we only serve named pipes. Indicated by NCACN_NP * passed in via "only_transport". * - * In AD mode, the "samba" process already serves many endpoints, - * passed in via "existing_binding". Don't serve those from - * samba-dcerpcd. - * * @param[in] binding Which binding is in question? * @param[in] only_transport Exclusive transport to serve - * @param[in] existing_bindings Endpoints served by "samba" already * @return Do we want to serve "binding" from samba-dcerpcd? */ static bool rpc_host_serve_endpoint( struct dcerpc_binding *binding, - enum dcerpc_transport_t only_transport, - struct dcerpc_binding **existing_bindings) + enum dcerpc_transport_t only_transport) { enum dcerpc_transport_t transport = dcerpc_binding_get_transport(binding); - size_t i, num_existing_bindings; - - num_existing_bindings = talloc_array_length(existing_bindings); - - for (i=0; ibinding, state->only_transport, state->existing_bindings); + ep->binding, state->only_transport); if (!serve_this) { goto fail; } @@ -1607,7 +1583,6 @@ static struct tevent_req *rpc_server_setup_send( TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct rpc_host *host, - struct dcerpc_binding **existing_bindings, const char *rpc_server_exe) { struct tevent_req *req = NULL, *subreq = NULL; @@ -1639,8 +1614,7 @@ static struct tevent_req *rpc_server_setup_send( state, ev, rpc_server_exe, - host->np_helper ? NCACN_NP : NCA_UNKNOWN, - existing_bindings); + host->np_helper ? NCACN_NP : NCA_UNKNOWN); if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); } @@ -2344,7 +2318,6 @@ static struct tevent_req *rpc_host_send( TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct messaging_context *msg_ctx, - struct dcerpc_binding **existing_bindings, char *servers, int ready_signal_fd, const char *daemon_ready_progname, @@ -2465,7 +2438,6 @@ static struct tevent_req *rpc_host_send( state, ev, host, - existing_bindings, exe); if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); @@ -2648,117 +2620,6 @@ static int rpc_host_pidfile_create( return EAGAIN; } -/* - * Find which interfaces are already being served by the samba AD - * DC so we know not to serve them. Some interfaces like netlogon - * are served by "samba", some like srvsvc will be served by the - * source3 based RPC servers. - */ -static NTSTATUS rpc_host_epm_lookup( - TALLOC_CTX *mem_ctx, - struct dcerpc_binding ***pbindings) -{ - struct rpc_pipe_client *cli = NULL; - struct pipe_auth_data *auth = NULL; - struct policy_handle entry_handle = { .handle_type = 0 }; - struct dcerpc_binding **bindings = NULL; - NTSTATUS status = NT_STATUS_UNSUCCESSFUL; - - status = rpc_pipe_open_ncalrpc(mem_ctx, &ndr_table_epmapper, &cli); - if (!NT_STATUS_IS_OK(status)) { - DBG_DEBUG("rpc_pipe_open_ncalrpc failed: %s\n", - nt_errstr(status)); - goto fail; - } - status = rpccli_ncalrpc_bind_data(cli, &auth); - if (!NT_STATUS_IS_OK(status)) { - DBG_DEBUG("rpccli_ncalrpc_bind_data failed: %s\n", - nt_errstr(status)); - goto fail; - } - status = rpc_pipe_bind(cli, auth); - if (!NT_STATUS_IS_OK(status)) { - DBG_DEBUG("rpc_pipe_bind failed: %s\n", nt_errstr(status)); - goto fail; - } - - for (;;) { - size_t num_bindings = talloc_array_length(bindings); - struct dcerpc_binding **tmp = NULL; - uint32_t num_entries = 0; - struct epm_entry_t *entry = NULL; - struct dcerpc_binding *binding = NULL; - uint32_t result; - - entry = talloc(cli, struct epm_entry_t); - if (entry == NULL) { - goto fail; - } - - status = dcerpc_epm_Lookup( - cli->binding_handle, /* binding_handle */ - cli, /* mem_ctx */ - 0, /* rpc_c_ep_all */ - NULL, /* object */ - NULL, /* interface id */ - 0, /* rpc_c_vers_all */ - &entry_handle, /* entry_handle */ - 1, /* max_ents */ - &num_entries, /* num_ents */ - entry, /* entries */ - &result); /* result */ - if (!NT_STATUS_IS_OK(status)) { - DBG_DEBUG("dcerpc_epm_Lookup failed: %s\n", - nt_errstr(status)); - goto fail; - } - - if (result == EPMAPPER_STATUS_NO_MORE_ENTRIES) { - break; - } - - if (result != EPMAPPER_STATUS_OK) { - DBG_DEBUG("dcerpc_epm_Lookup returned %"PRIu32"\n", - result); - break; - } - - if (num_entries != 1) { - DBG_DEBUG("epm_Lookup returned %"PRIu32" " - "entries, expected one\n", - num_entries); - break; - } - - status = dcerpc_binding_from_tower( - mem_ctx, &entry->tower->tower, &binding); - if (!NT_STATUS_IS_OK(status)) { - break; - } - - tmp = talloc_realloc( - mem_ctx, - bindings, - struct dcerpc_binding *, - num_bindings+1); - if (tmp == NULL) { - status = NT_STATUS_NO_MEMORY; - goto fail; - } - bindings = tmp; - - bindings[num_bindings] = talloc_move(bindings, &binding); - - TALLOC_FREE(entry); - } - - *pbindings = bindings; - status = NT_STATUS_OK; -fail: - TALLOC_FREE(cli); - return status; -} - static void samba_dcerpcd_stdin_handler( struct tevent_context *ev, struct tevent_fd *fde, @@ -2788,7 +2649,6 @@ int main(int argc, const char *argv[]) struct tevent_context *ev_ctx = NULL; struct messaging_context *msg_ctx = NULL; struct tevent_req *req = NULL; - struct dcerpc_binding **existing_bindings = NULL; char *servers = NULL; const char *arg = NULL; size_t num_servers; @@ -2995,11 +2855,6 @@ int main(int argc, const char *argv[]) exit(1); } - status = rpc_host_epm_lookup(frame, &existing_bindings); - DBG_DEBUG("rpc_host_epm_lookup returned %s, %zu bindings\n", - nt_errstr(status), - talloc_array_length(existing_bindings)); - ret = rpc_host_pidfile_create(msg_ctx, progname, ready_signal_fd); if (ret != 0) { DBG_DEBUG("rpc_host_pidfile_create failed: %s\n", @@ -3013,7 +2868,6 @@ int main(int argc, const char *argv[]) ev_ctx, ev_ctx, msg_ctx, - existing_bindings, servers, ready_signal_fd, cmdline_daemon_cfg->fork ? NULL : progname, -- 2.34.1