From 84b5d3640f7103dcc8984df7be679967bc06fd44 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 25 Jul 2023 17:41:04 -0700 Subject: [PATCH 01/28] 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 d1a26b4f46b4f27d01c90a71b9bbd065c2d0efd1 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 25 Jul 2023 17:49:21 -0700 Subject: [PATCH 02/28] 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 --- selftest/knownfail.d/badpipename | 1 + source3/selftest/tests.py | 14 ++++ source3/torture/proto.h | 1 + source3/torture/test_smb2.c | 107 +++++++++++++++++++++++++++++++ source3/torture/torture.c | 4 ++ 5 files changed, 127 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 1fdcad1089f..2c8336d35e8 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -309,6 +309,20 @@ plantestsuite("samba3.smbtorture_s3.smb2.SMB2-DFS-FILENAME-LEADING-BACKSLASH", smbtorture3, "-mSMB2"]) +# 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-NON-DFS-SHARE needs to run against a special share non-msdfs-pathname-share # This is an empty non-DFS share with no links, used merely to test diff --git a/source3/torture/proto.h b/source3/torture/proto.h index 21d7b3e00a7..3751697596a 100644 --- a/source3/torture/proto.h +++ b/source3/torture/proto.h @@ -125,6 +125,7 @@ bool run_smb2_non_dfs_share(int dummy); bool run_smb2_dfs_share_non_dfs_path(int dummy); bool run_smb2_dfs_filename_leading_backslash(int dummy); bool run_smb2_pipe_read_async_disconnect(int dummy); +bool run_smb2_invalid_pipename(int dummy); bool run_smb1_dfs_paths(int dummy); bool run_smb1_dfs_search_paths(int dummy); bool run_smb1_dfs_operations(int dummy); diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c index 269ade4ef61..7ea3d83de10 100644 --- a/source3/torture/test_smb2.c +++ b/source3/torture/test_smb2.c @@ -5253,3 +5253,110 @@ bool run_smb2_pipe_read_async_disconnect(int dummy) } return retval; } + +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 * */ + NULL); /* struct symlink_reparse_struct */ + /* 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 * */ + NULL); /* struct symlink_reparse_struct */ + /* + * 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 1315b328f5f..d53699c3b02 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -15763,6 +15763,10 @@ static struct { .name = "SMB2-DFS-FILENAME-LEADING-BACKSLASH", .fn = run_smb2_dfs_filename_leading_backslash, }, + { + .name = "SMB2-INVALID-PIPENAME", + .fn = run_smb2_invalid_pipename, + }, { .name = "SMB2-PIPE-READ-ASYNC-DISCONNECT", .fn = run_smb2_pipe_read_async_disconnect, -- 2.34.1 From 3e64edae781fe8cdec33b68b00f5daa51bb74b5d Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 25 Jul 2023 17:54:41 -0700 Subject: [PATCH 03/28] 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 bfe8e10bf3b5dafecd0b19ef8ea163327f73a531 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 1 Aug 2023 12:30:00 +0200 Subject: [PATCH 04/28] 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 3c432b144690353b7c86daf38612a2e19eb82084 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 1 Aug 2023 13:04:36 +0200 Subject: [PATCH 05/28] 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 94f50becb24..0c9ddfe7c94 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 582f4f2e844d95e48444d2b98c7397cac32ad6d4 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 1 Mar 2023 14:49:06 +1300 Subject: [PATCH 06/28] 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 5ca0ee6f111e63ef92bbb8fc94b81a08b490854f Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Fri, 27 Jan 2023 07:43:40 +1300 Subject: [PATCH 07/28] 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 e8df1a60866651678ce99d730f6a5e4bcc671b1d Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Wed, 2 Aug 2023 10:44:32 +0200 Subject: [PATCH 08/28] 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 ee6b5ae5cf6..280913d9b1c 100755 --- a/source4/dsdb/tests/python/acl.py +++ b/source4/dsdb/tests/python/acl.py @@ -228,7 +228,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) @@ -241,7 +241,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) @@ -302,7 +302,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) @@ -4298,7 +4298,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) @@ -4589,7 +4589,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" @@ -5045,7 +5045,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 09/28] 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 d054244b20d..9fbff1e119e 100644 --- a/source4/dsdb/samdb/samdb.h +++ b/source4/dsdb/samdb/samdb.h @@ -374,7 +374,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 b65b141ed7572503fc896b5efd46b3a48ef847d1 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 10 Mar 2023 18:25:18 +0100 Subject: [PATCH 10/28] 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 8ebcfe5599c5540da2fdd161d5108275d22c959e Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 18:03:10 +0100 Subject: [PATCH 11/28] 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 8a2b11fda30eef3883bbe9ea538dae6f68216fd9 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 10:11:05 +0100 Subject: [PATCH 12/28] 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 ff3de775af6..ac92f3a47a3 100644 --- a/source4/librpc/ndr/py_security.c +++ b/source4/librpc/ndr/py_security.c @@ -176,12 +176,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; } @@ -192,13 +193,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 217b30b05e24b66a427d9cc605141f917b88745c Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 17 Mar 2023 14:08:34 +0100 Subject: [PATCH 13/28] 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 da9bdf36c357826f4dd25cf1121dfdbba3ed1dd2 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 09:57:43 +0100 Subject: [PATCH 14/28] 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 b15f3d14c8a..25e6e145eeb 100644 --- a/lib/replace/replace.h +++ b/lib/replace/replace.h @@ -885,6 +885,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 7ebf51dd8b57b5932bb6f923d513e3f84c653567 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 10:00:11 +0100 Subject: [PATCH 15/28] 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 23d436dbaeb..bc38a405e1e 100644 --- a/libcli/security/security_descriptor.c +++ b/libcli/security/security_descriptor.c @@ -268,9 +268,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; @@ -289,15 +291,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: @@ -308,8 +323,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; @@ -328,7 +341,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); } /* @@ -338,7 +351,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 570e892a0e811b1c90b7fe6b065b16591d38f7ee Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 16 Mar 2023 10:03:44 +0100 Subject: [PATCH 16/28] 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 bc38a405e1e..db998bbf816 100644 --- a/libcli/security/security_descriptor.c +++ b/libcli/security/security_descriptor.c @@ -344,6 +344,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 */ @@ -354,6 +368,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 8ad21108f88be4fcabc1919757eed2ed06c06fba Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 11:55:55 +1200 Subject: [PATCH 17/28] 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 c6e0a242ecb..f1768a270b5 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 18/28] 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 23031057e8626e61994bf833226c196e0d966e63 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 14:44:28 +1200 Subject: [PATCH 19/28] 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 3e7bdcd0e488fe0788ca537ca9894f0c4fda6be6 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 11:56:56 +1200 Subject: [PATCH 20/28] 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 ebc2796a029b4dbe803457db0de9e999d1203460 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 11:18:46 +1200 Subject: [PATCH 21/28] 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 9d249db44c7c8feb1c4e8719739a5cee60b25842 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 14:30:19 +1200 Subject: [PATCH 22/28] 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 e691257c61813f2cf9513d149ba82b021ec824ee Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 22 Aug 2023 15:08:17 +1200 Subject: [PATCH 23/28] 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 4b3da3a97d1cbfd17a4eef466eb3bc1fc4887a34 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 17:58:27 +1200 Subject: [PATCH 24/28] 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 e652fbe8525dfaa5b7d794cac90f9d216432e78c Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 18:59:44 +1200 Subject: [PATCH 25/28] 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 16cb0d47f31..83b05260e09 100644 --- a/lib/param/loadparm.c +++ b/lib/param/loadparm.c @@ -2730,7 +2730,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 d15156a538b..5687d2a8587 100755 --- a/selftest/target/Samba4.pm +++ b/selftest/target/Samba4.pm @@ -783,7 +783,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 12718ced9e7..e33751a27e3 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 2ef556473bd858fc3dbcd6372835ded48f75135d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 19:01:03 +1200 Subject: [PATCH 26/28] 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 4eba269b1ba4ce6e9f71efed9f537249d1bd2c5d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 12:28:49 +1200 Subject: [PATCH 27/28] 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 eb6f2d92e8af60d67334a94ab5df56785a1508f2 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 12 Sep 2023 16:23:49 +1200 Subject: [PATCH 28/28] 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 1e27776d6fc..39ecd09f846 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