The Samba-Bugzilla – Attachment 13490 Details for
Bug 12977
[SECURITY ADVISORY][Fixed in 4.7] An RODC can access GetNCChanges info it shouldn't
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
A related change which potentially highlights this problem
v4.7-tangential-bugfix.patch (text/plain), 7.23 KB, created by
Tim Beale
on 2017-08-23 05:17:29 UTC
(
hide
)
Description:
A related change which potentially highlights this problem
Filename:
MIME Type:
Creator:
Tim Beale
Created:
2017-08-23 05:17:29 UTC
Size:
7.23 KB
patch
obsolete
>From 4ef46f216c5e5dc55a1a4332342c3daabb455c97 Mon Sep 17 00:00:00 2001 >From: Tim Beale <timbeale@catalyst.net.nz> >Date: Mon, 14 Aug 2017 15:31:08 +1200 >Subject: [PATCH 1/2] selftest: GetNCChanges can 'accept' a repeated bad > request > >In theory, if we send the exact same rejected request again, we should >get the same response back from the DC. However, we don't - the request >is accepted if we send it a second time. > >This patch updates the repl_rodc test to demonstrate the problem (which >now causes the test to fail). > >Note that although the bad GetNCChanges request is not rejected outright, >the response that gets sent back is empty - it has no objects in it, so >it's not an actual security hole. It is annoying problem for writing >self-tests though. > >Signed-off-by: Tim Beale <timbeale@catalyst.net.nz> >--- > selftest/knownfail.d/repl_rodc | 3 +++ > source4/torture/drs/python/repl_rodc.py | 7 +++++++ > 2 files changed, 10 insertions(+) > create mode 100644 selftest/knownfail.d/repl_rodc > >diff --git a/selftest/knownfail.d/repl_rodc b/selftest/knownfail.d/repl_rodc >new file mode 100644 >index 0000000..0e8e534 >--- /dev/null >+++ b/selftest/knownfail.d/repl_rodc >@@ -0,0 +1,3 @@ >+samba4.drs.repl_rodc.python\(ad_dc_ntvfs\).repl_rodc.DrsRodcTestCase.test_rodc_repl_secrets\(ad_dc_ntvfs\) >+ >+ >diff --git a/source4/torture/drs/python/repl_rodc.py b/source4/torture/drs/python/repl_rodc.py >index 01c9c6d..ca3744c 100644 >--- a/source4/torture/drs/python/repl_rodc.py >+++ b/source4/torture/drs/python/repl_rodc.py >@@ -202,6 +202,13 @@ class DrsRodcTestCase(drs_base.DrsBaseTestCase): > except WERRORError as (enum, estr): > self.assertEquals(enum, 8630) # ERROR_DS_DRA_SECRETS_DENIED > >+ # send the same request again and we should get the same response >+ try: >+ (level, ctr) = self.rodc_drs.DsGetNCChanges(self.rodc_drs_handle, 10, req10) >+ self.fail("Successfully replicated secrets to an RODC that shouldn't have been replicated.") >+ except WERRORError as (enum, estr): >+ self.assertEquals(enum, 8630) # ERROR_DS_DRA_SECRETS_DENIED >+ > # Retry with Administrator credentials, ignores password replication groups > (level, ctr) = self.drs.DsGetNCChanges(self.drs_handle, 10, req10) > >-- >2.7.4 > > >From 259c0c48820018a9e8cae1923e73ec20ae8bf3c4 Mon Sep 17 00:00:00 2001 >From: Tim Beale <timbeale@catalyst.net.nz> >Date: Wed, 16 Aug 2017 16:20:37 +1200 >Subject: [PATCH 2/2] s4-drsuapi: Set getnc_state *after* we've checked request > is valid > >We were creating the getnc_state (and storing it on the connection) >before we had done some basic checks that the request was valid. If the >request was not valid and we returned early with an error, then the >partially-initialized getnc_state was left hanging on the connection. >The next request that got sent on the connection would try to use this, >rather than creating a new getnc_state from scratch. > >The main side-effect of this was if you sent an invalid GetNCChanges >request twice, then it could be rejected the first time and accepted the >second time. > >Note that although an invalid request was accepted, it would typically >not return any objects, so it would not actually leak any secure >information. > >Signed-off-by: Tim Beale <timbeale@catalyst.net.nz> >--- > selftest/knownfail.d/repl_rodc | 3 -- > source4/rpc_server/drsuapi/getncchanges.c | 55 +++++++++++++++++-------------- > 2 files changed, 31 insertions(+), 27 deletions(-) > delete mode 100644 selftest/knownfail.d/repl_rodc > >diff --git a/selftest/knownfail.d/repl_rodc b/selftest/knownfail.d/repl_rodc >deleted file mode 100644 >index 0e8e534..0000000 >--- a/selftest/knownfail.d/repl_rodc >+++ /dev/null >@@ -1,3 +0,0 @@ >-samba4.drs.repl_rodc.python\(ad_dc_ntvfs\).repl_rodc.DrsRodcTestCase.test_rodc_repl_secrets\(ad_dc_ntvfs\) >- >- >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 1b39c17..6646ccd 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -2221,8 +2221,8 @@ allowed: > ldb_dn_get_linearized(new_dn), > ldb_dn_get_linearized(getnc_state->ncRoot_dn), > ldb_dn_get_linearized(getnc_state->last_dn))); >- talloc_free(getnc_state); >- getnc_state = NULL; >+ TALLOC_FREE(getnc_state); >+ b_state->getncchanges_state = NULL; > } > } > >@@ -2235,8 +2235,8 @@ allowed: > ldb_dn_get_linearized(getnc_state->ncRoot_dn), > (ret > 0) ? "older" : "newer", > ldb_dn_get_linearized(getnc_state->last_dn))); >- talloc_free(getnc_state); >- getnc_state = NULL; >+ TALLOC_FREE(getnc_state); >+ b_state->getncchanges_state = NULL; > } > } > >@@ -2248,39 +2248,25 @@ allowed: > NULL > }; > uint32_t nc_instanceType; >+ struct ldb_dn *ncRoot_dn; > >- getnc_state = talloc_zero(b_state, struct drsuapi_getncchanges_state); >- if (getnc_state == NULL) { >- return WERR_NOT_ENOUGH_MEMORY; >- } >- b_state->getncchanges_state = getnc_state; >- getnc_state->ncRoot_dn = drs_ObjectIdentifier_to_dn(getnc_state, sam_ctx, ncRoot); >- if (getnc_state->ncRoot_dn == NULL) { >+ ncRoot_dn = drs_ObjectIdentifier_to_dn(mem_ctx, sam_ctx, ncRoot); >+ if (ncRoot_dn == NULL) { > return WERR_NOT_ENOUGH_MEMORY; > } > > ret = dsdb_search_dn(sam_ctx, mem_ctx, &res, >- getnc_state->ncRoot_dn, attrs, >+ ncRoot_dn, attrs, > DSDB_SEARCH_SHOW_DELETED | > DSDB_SEARCH_SHOW_RECYCLED); > if (ret != LDB_SUCCESS) { > DBG_WARNING("Failed to find ncRoot_dn %s\n", >- ldb_dn_get_linearized(getnc_state->ncRoot_dn)); >+ ldb_dn_get_linearized(ncRoot_dn)); > return WERR_DS_CANT_FIND_EXPECTED_NC; > } >- getnc_state->ncRoot_guid = samdb_result_guid(res->msgs[0], >- "objectGUID"); > nc_instanceType = ldb_msg_find_attr_as_int(res->msgs[0], > "instanceType", > 0); >- TALLOC_FREE(res); >- ncRoot->guid = getnc_state->ncRoot_guid; >- >- /* find out if we are to replicate Schema NC */ >- ret = ldb_dn_compare_base(ldb_get_schema_basedn(sam_ctx), >- getnc_state->ncRoot_dn); >- >- getnc_state->is_schema_nc = (0 == ret); > > if (req10->extended_op != DRSUAPI_EXOP_NONE) { > r->out.ctr->ctr6.extended_ret = DRSUAPI_EXOP_ERR_SUCCESS; >@@ -2296,7 +2282,7 @@ allowed: > case DRSUAPI_EXOP_NONE: > if ((nc_instanceType & INSTANCE_TYPE_IS_NC_HEAD) == 0) { > const char *dn_str >- = ldb_dn_get_linearized(getnc_state->ncRoot_dn); >+ = ldb_dn_get_linearized(ncRoot_dn); > > DBG_NOTICE("Rejecting full replication on " > "not NC %s", dn_str); >@@ -2354,6 +2340,27 @@ allowed: > (unsigned)req10->extended_op)); > return WERR_DS_DRA_NOT_SUPPORTED; > } >+ >+ /* Initialize the state we'll store over the replication cycle */ >+ getnc_state = talloc_zero(b_state, struct drsuapi_getncchanges_state); >+ if (getnc_state == NULL) { >+ return WERR_NOT_ENOUGH_MEMORY; >+ } >+ b_state->getncchanges_state = getnc_state; >+ >+ getnc_state->ncRoot_dn = ncRoot_dn; >+ talloc_steal(getnc_state, ncRoot_dn); >+ >+ getnc_state->ncRoot_guid = samdb_result_guid(res->msgs[0], >+ "objectGUID"); >+ ncRoot->guid = getnc_state->ncRoot_guid; >+ >+ /* find out if we are to replicate Schema NC */ >+ ret = ldb_dn_compare_base(ldb_get_schema_basedn(sam_ctx), >+ ncRoot_dn); >+ getnc_state->is_schema_nc = (0 == ret); >+ >+ TALLOC_FREE(res); > } > > if (!ldb_dn_validate(getnc_state->ncRoot_dn) || >-- >2.7.4 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 12977
: 13490