From 4ef46f216c5e5dc55a1a4332342c3daabb455c97 Mon Sep 17 00:00:00 2001 From: Tim Beale 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 --- 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 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 --- 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