From 24e801cae0b64356007767c0f1177b17c79b03cd Mon Sep 17 00:00:00 2001 From: Lutz Justen Date: Thu, 21 Sep 2017 10:01:58 -0700 Subject: [PATCH 1/3] lib: gpo: Changes order to match GPO application order. The order of GPOs in a gpo_list generated by ads_get_gpo_list did not match the order of application. Since GPOs are pushed to the FRONT of gpo_list, GPOs have to be pushed in the opposite order of application. (Pushing to front is useful to get inheritance blocking right). BUG: https://bugzilla.samba.org/show_bug.cgi?id=13046 Signed-off-by: Lutz Justen Reviewed-by: Jeremy Allison Reviewed-by: Guenther Deschner (cherry picked from commit 6a531773b841f6b713226d1166a1e7d4dbc9b282) --- libgpo/gpo_ldap.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/libgpo/gpo_ldap.c b/libgpo/gpo_ldap.c index 4533d61a1e3..7ede12c3c21 100644 --- a/libgpo/gpo_ldap.c +++ b/libgpo/gpo_ldap.c @@ -812,12 +812,6 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, dump_gplink(&gp_link); } - /* block inheritance from now on */ - if (gp_link.gp_opts & - GPOPTIONS_BLOCK_INHERITANCE) { - add_only_forced_gpos = true; - } - status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, @@ -829,6 +823,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, if (!ADS_ERR_OK(status)) { return status; } + + /* block inheritance from now on */ + if (gp_link.gp_opts & + GPOPTIONS_BLOCK_INHERITANCE) { + add_only_forced_gpos = true; + } } } @@ -858,12 +858,6 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, dump_gplink(&gp_link); } - /* block inheritance from now on */ - if (gp_link.gp_opts & - GPOPTIONS_BLOCK_INHERITANCE) { - add_only_forced_gpos = true; - } - status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, @@ -875,6 +869,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, if (!ADS_ERR_OK(status)) { return status; } + + /* block inheritance from now on */ + if (gp_link.gp_opts & + GPOPTIONS_BLOCK_INHERITANCE) { + add_only_forced_gpos = true; + } } } -- 2.14.2.920.gcf0c67979c-goog From 15a5990e7f2e71e262b34640fc011eb6bbfd9d09 Mon Sep 17 00:00:00 2001 From: Lutz Justen Date: Thu, 21 Sep 2017 10:11:15 -0700 Subject: [PATCH 2/3] lib: gpo: Fixes issue with GPOPTIONS_BLOCK_INHERITANCE. GP links with the GPOPTIONS_BLOCK_INHERITANCE option set were blocking GPOs from the same link (i.e. an OU with the flag set would block its own GPOs). This patch makes sure the GPOs from the link are added to the list. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13046 Signed-off-by: Lutz Justen Reviewed-by: Jeremy Allison Reviewed-by: Guenther Deschner (cherry picked from commit 69410c0a02f7b4d7d20eadf4b4fda8ea064e4a0e) --- libgpo/gpo_ldap.c | 140 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 77 insertions(+), 63 deletions(-) diff --git a/libgpo/gpo_ldap.c b/libgpo/gpo_ldap.c index 7ede12c3c21..8fb5fdc40c1 100644 --- a/libgpo/gpo_ldap.c +++ b/libgpo/gpo_ldap.c @@ -561,10 +561,16 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, const struct security_token *token) { ADS_STATUS status; - int i; - - for (i = 0; i < gp_link->num_links; i++) { - + uint32_t count; + + /* + * Note: DLIST_ADD pushes to the front, + * so loop from last to first to get the + * order right. + */ + for (count = gp_link->num_links; count > 0; count--) { + /* NB. Index into arrays is one less than counter. */ + uint32_t i = count - 1; struct GROUP_POLICY_OBJECT *new_gpo = NULL; if (gp_link->link_opts[i] & GPO_LINK_OPT_DISABLED) { @@ -726,7 +732,15 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, const struct security_token *token, struct GROUP_POLICY_OBJECT **gpo_list) { - /* (L)ocal (S)ite (D)omain (O)rganizational(U)nit */ + /* + * Push GPOs to gpo_list so that the traversal order of the list matches + * the order of application: + * (L)ocal (S)ite (D)omain (O)rganizational(U)nit + * Within domains and OUs: parent-to-child. + * Since GPOs are pushed to the front of gpo_list, GPOs have to be + * pushed in the opposite order of application (OUs first, local last, + * child-to-parent). + */ ADS_STATUS status; struct GP_LINK gp_link; @@ -745,63 +759,18 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, DEBUG(10,("ads_get_gpo_list: getting GPO list for [%s]\n", dn)); - /* (L)ocal */ - status = add_local_policy_to_gpo_list(mem_ctx, gpo_list, - GP_LINK_LOCAL); - if (!ADS_ERR_OK(status)) { - return status; - } - - /* (S)ite */ - - /* are site GPOs valid for users as well ??? */ - if (flags & GPO_LIST_FLAG_MACHINE) { - - status = ads_site_dn_for_machine(ads, mem_ctx, - ads->config.ldap_server_name, - &site_dn); - if (!ADS_ERR_OK(status)) { - return status; - } - - DEBUG(10,("ads_get_gpo_list: query SITE: [%s] for GPOs\n", - site_dn)); - - status = ads_get_gpo_link(ads, mem_ctx, site_dn, &gp_link); - if (ADS_ERR_OK(status)) { - - if (DEBUGLEVEL >= 100) { - dump_gplink(&gp_link); - } - - status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, - site_dn, &gp_link, - GP_LINK_SITE, - add_only_forced_gpos, - token); - if (!ADS_ERR_OK(status)) { - return status; - } - - if (flags & GPO_LIST_FLAG_SITEONLY) { - return ADS_ERROR(LDAP_SUCCESS); - } - - /* inheritance can't be blocked at the site level */ - } - } - tmp_dn = dn; while ((parent_dn = ads_parent_dn(tmp_dn)) && (!strequal(parent_dn, ads_parent_dn(ads->config.bind_path)))) { - /* (D)omain */ - /* An account can just be a member of one domain */ - if (strncmp(parent_dn, "DC=", strlen("DC=")) == 0) { + /* (O)rganizational(U)nit */ - DEBUG(10,("ads_get_gpo_list: query DC: [%s] for GPOs\n", + /* An account can be a member of more OUs */ + if (strncmp(parent_dn, "OU=", strlen("OU=")) == 0) { + + DEBUG(10,("ads_get_gpo_list: query OU: [%s] for GPOs\n", parent_dn)); status = ads_get_gpo_link(ads, mem_ctx, parent_dn, @@ -817,7 +786,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, gpo_list, parent_dn, &gp_link, - GP_LINK_DOMAIN, + GP_LINK_OU, add_only_forced_gpos, token); if (!ADS_ERR_OK(status)) { @@ -833,6 +802,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, } tmp_dn = parent_dn; + } /* reset dn again */ @@ -841,13 +811,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, while ((parent_dn = ads_parent_dn(tmp_dn)) && (!strequal(parent_dn, ads_parent_dn(ads->config.bind_path)))) { + /* (D)omain */ - /* (O)rganizational(U)nit */ - - /* An account can be a member of more OUs */ - if (strncmp(parent_dn, "OU=", strlen("OU=")) == 0) { + /* An account can just be a member of one domain */ + if (strncmp(parent_dn, "DC=", strlen("DC=")) == 0) { - DEBUG(10,("ads_get_gpo_list: query OU: [%s] for GPOs\n", + DEBUG(10,("ads_get_gpo_list: query DC: [%s] for GPOs\n", parent_dn)); status = ads_get_gpo_link(ads, mem_ctx, parent_dn, @@ -863,7 +832,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, gpo_list, parent_dn, &gp_link, - GP_LINK_OU, + GP_LINK_DOMAIN, add_only_forced_gpos, token); if (!ADS_ERR_OK(status)) { @@ -879,8 +848,53 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, } tmp_dn = parent_dn; + } + + /* (S)ite */ + + /* are site GPOs valid for users as well ??? */ + if (flags & GPO_LIST_FLAG_MACHINE) { - }; + status = ads_site_dn_for_machine(ads, mem_ctx, + ads->config.ldap_server_name, + &site_dn); + if (!ADS_ERR_OK(status)) { + return status; + } + + DEBUG(10,("ads_get_gpo_list: query SITE: [%s] for GPOs\n", + site_dn)); + + status = ads_get_gpo_link(ads, mem_ctx, site_dn, &gp_link); + if (ADS_ERR_OK(status)) { + + if (DEBUGLEVEL >= 100) { + dump_gplink(&gp_link); + } + + status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, + site_dn, &gp_link, + GP_LINK_SITE, + add_only_forced_gpos, + token); + if (!ADS_ERR_OK(status)) { + return status; + } + + if (flags & GPO_LIST_FLAG_SITEONLY) { + return ADS_ERROR(LDAP_SUCCESS); + } + + /* inheritance can't be blocked at the site level */ + } + } + + /* (L)ocal */ + status = add_local_policy_to_gpo_list(mem_ctx, gpo_list, + GP_LINK_LOCAL); + if (!ADS_ERR_OK(status)) { + return status; + } return ADS_ERROR(LDAP_SUCCESS); } -- 2.14.2.920.gcf0c67979c-goog From 93ef238a96bc3faee097a1f18683714ac0581905 Mon Sep 17 00:00:00 2001 From: Lutz Justen Date: Thu, 21 Sep 2017 10:32:05 -0700 Subject: [PATCH 3/3] lib: gpo: Put enforced GPOs at the end of the list. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enforced GPOs should be applied on top of all non-enforced GPOs, so that they override policies set in non-enforced GPOs. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13046 Signed-off-by: Lutz Justen Reviewed-by: Jeremy Allison Reviewed-by: Guenther Deschner Autobuild-User(master): Günther Deschner Autobuild-Date(master): Sat Sep 23 05:25:19 CEST 2017 on sn-devel-144 (cherry picked from commit 5f2576a9af4f3c33121ad2b27a621b5f3bb34374) --- libgpo/gpo_ldap.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 8 deletions(-) diff --git a/libgpo/gpo_ldap.c b/libgpo/gpo_ldap.c index 8fb5fdc40c1..fec00053b49 100644 --- a/libgpo/gpo_ldap.c +++ b/libgpo/gpo_ldap.c @@ -554,6 +554,7 @@ ADS_STATUS ads_get_gpo(ADS_STRUCT *ads, static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, TALLOC_CTX *mem_ctx, struct GROUP_POLICY_OBJECT **gpo_list, + struct GROUP_POLICY_OBJECT **forced_gpo_list, const char *link_dn, struct GP_LINK *gp_link, enum GPO_LINK_TYPE link_type, @@ -571,7 +572,10 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, for (count = gp_link->num_links; count > 0; count--) { /* NB. Index into arrays is one less than counter. */ uint32_t i = count - 1; + struct GROUP_POLICY_OBJECT **target_list = NULL; struct GROUP_POLICY_OBJECT *new_gpo = NULL; + bool is_forced = + (gp_link->link_opts[i] & GPO_LINK_OPT_ENFORCED) != 0; if (gp_link->link_opts[i] & GPO_LINK_OPT_DISABLED) { DEBUG(10,("skipping disabled GPO\n")); @@ -580,7 +584,7 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, if (only_add_forced_gpos) { - if (!(gp_link->link_opts[i] & GPO_LINK_OPT_ENFORCED)) { + if (!is_forced) { DEBUG(10,("skipping nonenforced GPO link " "because GPOPTIONS_BLOCK_INHERITANCE " "has been set\n")); @@ -623,7 +627,8 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, new_gpo->link = link_dn; new_gpo->link_type = link_type; - DLIST_ADD(*gpo_list, new_gpo); + target_list = is_forced ? forced_gpo_list : gpo_list; + DLIST_ADD(*target_list, new_gpo); DEBUG(10,("add_gplink_to_gplist: added GPLINK #%d %s " "to GPO list\n", i, gp_link->link_names[i])); @@ -722,24 +727,27 @@ static ADS_STATUS add_local_policy_to_gpo_list(TALLOC_CTX *mem_ctx, } /**************************************************************** - get the full list of GROUP_POLICY_OBJECTs for a given dn + Get the full list of GROUP_POLICY_OBJECTs for a given dn. ****************************************************************/ -ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, +static ADS_STATUS ads_get_gpo_list_internal(ADS_STRUCT *ads, TALLOC_CTX *mem_ctx, const char *dn, uint32_t flags, const struct security_token *token, - struct GROUP_POLICY_OBJECT **gpo_list) + struct GROUP_POLICY_OBJECT **gpo_list, + struct GROUP_POLICY_OBJECT **forced_gpo_list) { /* * Push GPOs to gpo_list so that the traversal order of the list matches * the order of application: * (L)ocal (S)ite (D)omain (O)rganizational(U)nit - * Within domains and OUs: parent-to-child. + * For different domains and OUs: parent-to-child. + * Within same level of domains and OUs: Link order. * Since GPOs are pushed to the front of gpo_list, GPOs have to be * pushed in the opposite order of application (OUs first, local last, * child-to-parent). + * Forced GPOs are appended in the end since they override all others. */ ADS_STATUS status; @@ -748,6 +756,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, bool add_only_forced_gpos = false; ZERO_STRUCTP(gpo_list); + ZERO_STRUCTP(forced_gpo_list); if (!dn) { return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER); @@ -784,6 +793,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, + forced_gpo_list, parent_dn, &gp_link, GP_LINK_OU, @@ -830,6 +840,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, + forced_gpo_list, parent_dn, &gp_link, GP_LINK_DOMAIN, @@ -872,8 +883,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, dump_gplink(&gp_link); } - status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, - site_dn, &gp_link, + status = add_gplink_to_gpo_list(ads, + mem_ctx, + gpo_list, + forced_gpo_list, + site_dn, + &gp_link, GP_LINK_SITE, add_only_forced_gpos, token); @@ -899,4 +914,38 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, return ADS_ERROR(LDAP_SUCCESS); } +/**************************************************************** + Get the full list of GROUP_POLICY_OBJECTs for a given dn, wrapper + around ads_get_gpo_list_internal() that ensures correct ordering. +****************************************************************/ + +ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, + TALLOC_CTX *mem_ctx, + const char *dn, + uint32_t flags, + const struct security_token *token, + struct GROUP_POLICY_OBJECT **gpo_list) +{ + struct GROUP_POLICY_OBJECT *forced_gpo_list = NULL; + ADS_STATUS status; + + status = ads_get_gpo_list_internal(ads, + mem_ctx, + dn, + flags, + token, + gpo_list, + &forced_gpo_list); + if (!ADS_ERR_OK(status)) { + return status; + } + /* + * Append |forced_gpo_list| at the end of |gpo_list|, + * so that forced GPOs are applied on top of non enforced GPOs. + */ + DLIST_CONCATENATE(*gpo_list, forced_gpo_list); + + return ADS_ERROR(LDAP_SUCCESS); +} + #endif /* HAVE_LDAP */ -- 2.14.2.920.gcf0c67979c-goog