#3320 Fix group policy security filtering and download
Closed 4 years ago by pbrezina. Opened 7 years ago by thor.
SSSD/ thor/sssd master  into  master

file modified
+224 -74
@@ -134,7 +134,7 @@ 

      const char *policy_filename;

  };

  

- enum ace_eval_status {

+ enum ace_eval_agp_status {

      AD_GPO_ACE_DENIED,

      AD_GPO_ACE_ALLOWED,

      AD_GPO_ACE_NEUTRAL
@@ -698,40 +698,55 @@ 

  }

  

  /*

-  * This function determines whether use of the extended right

-  * named "ApplyGroupPolicy" (AGP) is allowed, by comparing the specified

-  * user_sid and group_sids against the specified access control entry (ACE).

+  * This function determines whether use of the extended right named

+  * "ApplyGroupPolicy" (AGP) is allowed for the GPO, by comparing the

+  * specified user_sid and group_sids against the passed access control

+  * entry (ACE).

   * This function returns ALLOWED, DENIED, or NEUTRAL depending on whether

   * the ACE explicitly allows, explicitly denies, or does neither.

   *

-  * Note that the 'M' abbreviation used in the evaluation algorithm stands for

-  * "access_mask", which represents the set of access rights associated with an

-  * individual ACE. The access right of interest to the GPO code is

+  * Notes:

+  * (1) Abbreviation 'M' used in the evaluation algorithm stands for

+  * "access_mask", which represents the set of access rights associated with

+  * the passed ACE. The access right of interest to the GPO code is

   * RIGHT_DS_CONTROL_ACCESS, which serves as a container for all control access

   * rights. The specific control access right is identified by a GUID in the

   * ACE's ObjectType. In our case, this is the GUID corresponding to AGP.

+  * (2) ACE that require an evaluation algorithm different from [MS-ADTS]

+  * 5.1.3.3.4, e. g. RIGHT_DS_CONTROL_ACCESS (CR) is not present in M, are

+  * ignored.

   *

   * The ACE evaluation algorithm is specified in [MS-ADTS] 5.1.3.3.4:

-  * - Deny access by default

-  * - If the "Inherit Only" (IO) flag is set in the ACE, skip the ACE.

-  * - If the SID in the ACE does not match any SID in the requester's

-  *   security context, skip the ACE

-  * - If the ACE type is "Object Access Allowed", the access right

-  *   RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType

-  *   field in the ACE is either not present OR contains a GUID value equal

-  *   to AGP, then grant requested control access right. Stop access checking.

-  * - If the ACE type is "Object Access Denied", the access right

-  *   RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType

-  *   field in the ACE is either not present OR contains a GUID value equal to

-  *   AGP, then deny the requested control access right. Stop access checking.

+  * Evaluate the DACL by examining each ACE in sequence, starting with the first

+  * ACE. Perform the following sequence of actions for each ACE in the order as

+  * shown:

+  * 1. If the "Inherit Only" (IO) flag is set in the ACE, skip the ACE.

+  * 2. If the SID in the ACE does not match any SID in the requester's

+  *    security context, skip the ACE.

+  * 3. If the ACE type is "Object Access Allowed", the access right

+  *    RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType

+  *    field in the ACE is not present, then grant the requested control

+  *    access right. Stop any further access checks.

+  * 4. If the ACE type is "Object Access Allowed" the access right

+  *    RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType

+  *    field in the ACE contains a GUID value equal to AGP, then grant

+  *    the requested control access right. Stop any further access checks.

+  * 5. If the ACE type is "Object Access Denied", the access right

+  *    RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType

+  *    field in the ACE is not present, then deny the requested control

+  *    access right. Stop any further access checks.

+  * 6. If the ACE type is "Object Access Denied" the access right

+  *    RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType

+  *    field in the ACE contains a GUID value equal to AGP, then deny

+  *    the requested control access right. Stop any further access checks.

   */

- static enum ace_eval_status ad_gpo_evaluate_ace(struct security_ace *ace,

-                                                 struct sss_idmap_ctx *idmap_ctx,

-                                                 const char *user_sid,

-                                                 const char **group_sids,

-                                                 int group_size)

+ static enum ace_eval_agp_status

+ ad_gpo_evaluate_agp_ace(struct security_ace *ace,

+                         struct sss_idmap_ctx *idmap_ctx,

+                         const char *user_sid,

+                         const char **group_sids,

+                         int group_size)

  {

-     bool agp_included = false;

      bool included = false;

      int ret = 0;

      struct security_ace_object object;
@@ -752,36 +767,100 @@ 

          return AD_GPO_ACE_NEUTRAL;

      }

  

-     object = ace->object.object;

-     GUID_from_string(AD_AGP_GUID, &ext_right_agp_guid);

- 

-     if (object.flags & SEC_ACE_OBJECT_TYPE_PRESENT) {

-         if (GUID_equal(&object.type.type, &ext_right_agp_guid)) {

-             agp_included = true;

-         }

-     } else {

-         agp_included = false;

-     }

- 

      if (ace->access_mask & SEC_ADS_CONTROL_ACCESS) {

-         if (agp_included) {

-             if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {

-                 return AD_GPO_ACE_ALLOWED;

-             } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED_OBJECT) {

-                 return AD_GPO_ACE_DENIED;

+         object = ace->object.object;

+         if (object.flags & SEC_ACE_OBJECT_TYPE_PRESENT) {

+             GUID_from_string(AD_AGP_GUID, &ext_right_agp_guid);

+             if (!GUID_equal(&object.type.type, &ext_right_agp_guid)) {

+                 return AD_GPO_ACE_NEUTRAL;

              }

          }

+         if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {

+             return AD_GPO_ACE_ALLOWED;

+         } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED_OBJECT) {

+             return AD_GPO_ACE_DENIED;

+         }

+     }

+ 

+     return AD_GPO_ACE_NEUTRAL;

+ }

+ 

+ /*

+  * This function evaluates, which standard access rights the passed access

+  * control entry (ACE) allows or denies for the entire GPO.

+  *

+  * Notes:

+  * (1) Abbreviation 'M' used in the evaluation algorithm stands for

+  * "access_mask", which represents the set of access rights associated with

+  * the passed ACE.

+  * (2) Abbreviation 'G' used in the evaluation algorithm stands for

+  * "granted rights", which represents the set of access rights, that

+  * have already been granted by previously evaluated ACEs.

+  * (3) Abbreviation 'D' used in the evaluation algorithm stands for

+  * "denied rights", which represents the set of access rights, that

+  * have already been explicitly denied by previously evaluated ACEs.

+  *

+  * The simple ACE evaluation algorithm is specified in [MS-ADTS] 5.1.3.3.2:

+  * Evaluate the DACL by examining each ACE in sequence, starting with the first

+  * ACE. Perform the following sequence of actions for each ACE in the order as

+  * shown:

+  * 1. If the "Inherit Only" (IO) flag is set in the ACE, skip the ACE.

+  * 2. If the SID in the ACE does not match any SID in the requester's

+  *    security context, skip the ACE.

+  * 3. If the ACE type is "Access Denied" and the access rights in M

+  *    are not in G, then add the rights in M to D.

+  * 4. If the ACE type is "Access Allowed" and the access rights in M

+  *    are not in D, then add the rights in M to G.

+  */

+ static errno_t ad_gpo_simple_evaluate_ace(struct security_ace *ace,

+                                           struct sss_idmap_ctx *idmap_ctx,

+                                           const char *user_sid,

+                                           const char **group_sids,

+                                           int group_size,

+                                           uint32_t *_gpo_access_granted_status,

+                                           uint32_t *_gpo_access_denied_status)

+ {

+     bool included = false;

+     uint32_t filtered_access_rights = 0;

+     int ret = 0;

+ 

+     if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {

+         return EOK;

+     }

+ 

+     ret = ad_gpo_ace_includes_client_sid(user_sid, group_sids, group_size,

+                                          ace->trustee, idmap_ctx, &included);

+ 

+     if (ret != EOK || !included) {

+         return ret;

      }

  

-     return AD_GPO_ACE_DENIED;

+     if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {

+         filtered_access_rights = ace->access_mask & ~*_gpo_access_granted_status;

+         *_gpo_access_denied_status |= filtered_access_rights;

+     } else if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {

+         filtered_access_rights = ace->access_mask & ~*_gpo_access_denied_status;

+         *_gpo_access_granted_status |= filtered_access_rights;

+     }

+ 

+     return ret;

  }

  

+ 

  /*

   * This function extracts the GPO's DACL (discretionary access control list)

   * from the GPO's specified security descriptor, and determines whether

   * the GPO is applicable to the policy target, by comparing the specified

   * user_sid and group_sids against each access control entry (ACE) in the DACL.

-  * The boolean result is assigned to the _access_allowed output parameter.

+  * The GPO is only applicable to the target, if the requester has been granted

+  * read access (RIGHT_DS_READ_PROPERTY) to the properties of the GPO and

+  * control access (RIGHT_DS_CONTROL_ACCESS) to apply the GPO (AGP).

+  * The required read and control access rights for a particular trustee are

+  * usually located in different ACEs, i.e. one ACE for control of read access

+  * and one for control access.

+  * If it comes to the end of the DACL, and the required access is still not

+  * explicitly allowed or denied, SSSD denies access to the object as specified

+  * in [MS-ADTS] 5.1.3.1.

   */

  static errno_t ad_gpo_evaluate_dacl(struct security_acl *dacl,

                                      struct sss_idmap_ctx *idmap_ctx,
@@ -791,14 +870,19 @@ 

                                      bool *_dacl_access_allowed)

  {

      uint32_t num_aces = 0;

-     enum ace_eval_status ace_status;

-     int i = 0;

+     uint32_t access_granted_status = 0;

+     uint32_t access_denied_status = 0;

+     enum ace_eval_agp_status ace_status;

      struct security_ace *ace = NULL;

+     int i = 0;

+     int ret = 0;

+     enum idmap_error_code err;

+     char *trustee_dom_sid_str = NULL;

  

      num_aces = dacl->num_aces;

  

      /*

-      * [MS-ADTS] 5.1.3.3.4:

+      * [MS-ADTS] 5.1.3.3.2. and 5.1.3.3.4:

       * If the DACL does not have any ACE, then deny the requester the

       * requested control access right.

       */
@@ -807,22 +891,88 @@ 

          return EOK;

      }

  

+     /*

+      * [MS-GOPD] 2.4:

+      * To process a policy that applies to a Group Policy client, the core

+      * Group Policy engine must be able to read the policy data from the

+      * directory service so that the policy settings can be applied to the

+      * Group Policy client or the interactive user.

+      */

+     for (i = 0; i < dacl->num_aces; i++) {

+         ace = &dacl->aces[i];

+ 

+         ret = ad_gpo_simple_evaluate_ace(ace, idmap_ctx, user_sid,

+                                          group_sids, group_size,

+                                          &access_granted_status,

+                                          &access_denied_status);

+ 

+         if (ret != EOK) {

+             err = sss_idmap_smb_sid_to_sid(idmap_ctx, &ace->trustee,

+                                            &trustee_dom_sid_str);

+             if (err != IDMAP_SUCCESS) {

+                 DEBUG(SSSDBG_OP_FAILURE,

+                       "sss_idmap_smb_sid_to_sid failed.\n");

+                 return EFAULT;

+             }

+ 

+             DEBUG(SSSDBG_MINOR_FAILURE,

+                   "Could not determine if ACE is applicable; "

+                   " Trustee: %s\n", trustee_dom_sid_str);

+             sss_idmap_free_sid(idmap_ctx, trustee_dom_sid_str);

+             trustee_dom_sid_str = NULL;

+             continue;

+         }

+     }

+ 

      for (i = 0; i < dacl->num_aces; i ++) {

          ace = &dacl->aces[i];

  

-         ace_status = ad_gpo_evaluate_ace(ace, idmap_ctx, user_sid,

-                                          group_sids, group_size);

+         err = sss_idmap_smb_sid_to_sid(idmap_ctx, &ace->trustee,

+                                        &trustee_dom_sid_str);

+         if (err != IDMAP_SUCCESS) {

+             DEBUG(SSSDBG_OP_FAILURE, "sss_idmap_smb_sid_to_sid failed.\n");

+             return EFAULT;

+         }

+ 

+         ace_status = ad_gpo_evaluate_agp_ace(ace, idmap_ctx, user_sid,

+                                              group_sids, group_size);

  

          switch (ace_status) {

          case AD_GPO_ACE_NEUTRAL:

-             continue;

+             break;

          case AD_GPO_ACE_ALLOWED:

-             *_dacl_access_allowed = true;

-             return EOK;

+             if (access_granted_status & SEC_ADS_READ_PROP) {

+                 *_dacl_access_allowed = true;

+                 sss_idmap_free_sid(idmap_ctx, trustee_dom_sid_str);

+                 return EOK;

+             } else {

+                 DEBUG(SSSDBG_TRACE_FUNC,

+                       "GPO read properties access denied (security); "

+                       " Trustee: %s\n", trustee_dom_sid_str);

+                 break;

+             }

          case AD_GPO_ACE_DENIED:

-             *_dacl_access_allowed = false;

-             return EOK;

+             if (access_granted_status & SEC_ADS_READ_PROP) {

+                 DEBUG(SSSDBG_TRACE_FUNC,

+                       "GPO denied (security); "

+                       " Trustee: %s\n", trustee_dom_sid_str);

+                 sss_idmap_free_sid(idmap_ctx, trustee_dom_sid_str);

+                 *_dacl_access_allowed = false;

+                 return EOK;

+             } else {

+                 DEBUG(SSSDBG_TRACE_FUNC,

+                       "GPO read properties access denied (security); "

+                       " Trustee: %s\n", trustee_dom_sid_str);

+                 break;

+             }

          }

+         sss_idmap_free_sid(idmap_ctx, trustee_dom_sid_str);

+         trustee_dom_sid_str = NULL;

+     }

+ 

+     if (access_granted_status & SEC_ADS_READ_PROP) {

+         DEBUG(SSSDBG_TRACE_FUNC,

+               "GPO apply group policy access denied (security)\n");

      }

  

      *_dacl_access_allowed = false;
@@ -887,12 +1037,12 @@ 

          access_allowed = false;

          candidate_gpo = candidate_gpos[i];

  

-         DEBUG(SSSDBG_TRACE_ALL, "examining dacl candidate_gpo_guid:%s\n",

-                                 candidate_gpo->gpo_guid);

+         DEBUG(SSSDBG_TRACE_FUNC, "examining dacl candidate_gpo_guid:%s\n",

+               candidate_gpo->gpo_guid);

  

          /* gpo_func_version must be set to version 2 */

          if (candidate_gpo->gpo_func_version != 2) {

-             DEBUG(SSSDBG_TRACE_ALL,

+             DEBUG(SSSDBG_TRACE_FUNC,

                    "GPO not applicable to target per security filtering: "

                    "gPCFunctionalityVersion is not 2\n");

              continue;
@@ -900,7 +1050,7 @@ 

  

          sd = candidate_gpo->gpo_sd;

          if (sd == NULL) {

-             DEBUG(SSSDBG_TRACE_ALL, "Security descriptor is missing\n");

+             DEBUG(SSSDBG_MINOR_FAILURE, "Security descriptor is missing\n");

              ret = EINVAL;

              goto done;

          }
@@ -909,39 +1059,39 @@ 

  

          /* gpo_flags value of 2 means that GPO's computer portion is disabled */

          if (candidate_gpo->gpo_flags == 2) {

-             DEBUG(SSSDBG_TRACE_ALL,

+             DEBUG(SSSDBG_TRACE_FUNC,

                    "GPO not applicable to target per security filtering: "

                    "GPO's computer portion is disabled\n");

              continue;

          }

  

-         /*

-          * [MS-ADTS] 5.1.3.3.4:

-          * If the security descriptor has no DACL or its "DACL Present" bit

-          * is not set, then grant requester the requested control access right.

-          */

+         if (((sd->type & SEC_DESC_DACL_PRESENT)) && (dacl != NULL)) {

+             ret = ad_gpo_evaluate_dacl(dacl, idmap_ctx, user_sid, group_sids,

+                                        group_size, &access_allowed);

+             if (ret != EOK) {

+                 DEBUG(SSSDBG_MINOR_FAILURE,

+                       "Could not determine if GPO is applicable\n");

+                 continue;

+             }

+         } else {

+             /*

+              * [MS-ADTS] 5.1.3.3.4:

+              * If the security descriptor has no DACL or its "DACL Present" bit

+              * is not set, then grant requester the requested control access right.

+              */

  

-         if ((!(sd->type & SEC_DESC_DACL_PRESENT)) || (dacl == NULL)) {

              DEBUG(SSSDBG_TRACE_ALL, "DACL is not present\n");

              access_allowed = true;

-             break;

-         }

- 

-         ret = ad_gpo_evaluate_dacl(dacl, idmap_ctx, user_sid, group_sids,

-                                    group_size, &access_allowed);

-         if (ret != EOK) {

-             DEBUG(SSSDBG_MINOR_FAILURE, "Could not determine if GPO is applicable\n");

-             continue;

          }

  

          if (access_allowed) {

-             DEBUG(SSSDBG_TRACE_ALL,

+             DEBUG(SSSDBG_TRACE_FUNC,

                    "GPO applicable to target per security filtering\n");

              dacl_filtered_gpos[gpo_dn_idx] = talloc_steal(dacl_filtered_gpos,

                                                            candidate_gpo);

              gpo_dn_idx++;

          } else {

-             DEBUG(SSSDBG_TRACE_ALL,

+             DEBUG(SSSDBG_TRACE_FUNC,

                    "GPO not applicable to target per security filtering: "

                    "result of DACL evaluation\n");

              continue;

@@ -23,6 +23,7 @@ 

  */

  

  #include <sys/types.h>

+ #include <ctype.h>

  #include <unistd.h>

  #include <sys/stat.h>

  #include <popt.h>
@@ -529,7 +530,8 @@ 

                             const char *smb_cse_suffix)

  {

      char *smb_uri = NULL;

-     SMBCFILE *file;

+     char *gpt_main_folder = NULL;

+     SMBCFILE *file = NULL;

      int ret;

      uint8_t *buf = NULL;

      int buflen = 0;
@@ -554,10 +556,38 @@ 

      errno = 0;

      file = smbc_getFunctionOpen(smbc_ctx)(smbc_ctx, smb_uri, O_RDONLY, 0755);

      if (file == NULL) {

-         ret = errno;

-         DEBUG(SSSDBG_CRIT_FAILURE, "smbc_getFunctionOpen failed [%d][%s]\n",

-               ret, strerror(ret));

-         goto done;

+         /*

+          * DCs may use upper case names for the main folder, where GPTs are

+          * stored. libsmbclient does not allow us to request case insensitive

+          * file name lookups on DCs with case sensitive file systems.

+          */

+         gpt_main_folder = strstr(smb_uri, "/Machine/");

+         if (gpt_main_folder == NULL) {

+             /* At this moment we do not use any GPO from user settings,

+              * but it can change in the future so let's keep the following

+              * line around to make this part of the code 'just work' also

+              * with the user GPO settings. */

+             gpt_main_folder = strstr(smb_uri, "/User/");

+         }

+         if (gpt_main_folder != NULL) {

+             ++gpt_main_folder;

+             while (gpt_main_folder != NULL && *gpt_main_folder != '/') {

+                 *gpt_main_folder = toupper(*gpt_main_folder);

+                 ++gpt_main_folder;

+             }

+ 

+             DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);

+ 

+             errno = 0;

+             file = smbc_getFunctionOpen(smbc_ctx)(smbc_ctx, smb_uri, O_RDONLY, 0755);

+         }

+ 

+         if (file == NULL) {

+             ret = errno;

+             DEBUG(SSSDBG_CRIT_FAILURE, "smbc_getFunctionOpen failed [%d][%s]\n",

+                   ret, strerror(ret));

+             goto done;

+         }

      }

  

      buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
@@ -587,6 +617,10 @@ 

      }

  

   done:

+     if (file != NULL) {

+         smbc_getFunctionClose(smbc_ctx)(smbc_ctx, file);

+     }

+ 

      talloc_free(tmp_ctx);

      return ret;

  }

Please find a summary and further information in the first commit, which was a merge commit of the fix branch:
Commit b2cdf58 [PATCH 0/6] Fix group policy security filtering and download.

That's quite huge change to the GPO code,
It will take some time to review changes and MSDN documentation.

Firstly I will try to run some regression tests.
Anyway thank you very much for patches.

@thor thank you indeed would you also mind creating an issue? The commit message explanation in the merge commit is excellent, so just copying the message there should be good enough.

(I can create the issue as well if you're busy, just tell me your preference)

@jhrozek you will find the issue under ID #3324

rebased onto 2c99273c9d068594d4b85f7d4fb43385a56ad3e7

6 years ago

rebased onto 7f4b48077bc4f4b51e130b7001e300629f7b39c9

6 years ago

rebased onto e08f76eabd95d1867aac768ed556c242bb67b8e1

6 years ago

rebased onto ad6048910455c23daa90961983c72fd1dd733d66

6 years ago

Rebase of the patches and update according to Michals review fetched back from https://github.com/SSSD/sssd/pull/412.

Squashed following minor updates into the patches based on my review and test:
Commit 9df1c37 GPO: Support group policy file main folders with upper case name
Line 33 furute -> future

Commit 8fd1ba6 MAN: Provide minimum information on GPO access control
Line 54 'function data' -> 'trace functions'
Line 56 <refentrytitle>sss_debuglevel</refentrytitle> -> <refentrytitle>sssctl</refentrytitle>

rebased onto 3d04a9135114da1ad758ade93415259b00c78a37

6 years ago

I have rebased the patches on top of the current master.

@thor thank you very much for rebased paches. And I would like to apologize for longer review.

You found problems with Samba AD server (4.3.11) at least based on ticket #3324. I assume you did everything from command line therefore it should not be difficult to provide steps how to reproduce these failures.

Because I would like to write regressions tests + also confirm that it really fixed your issue. And we have samba-ad-dc-4.7 in f27 so it would be trivial for us :-)

1 new commit added

  • GPO: Do not crash during group policy evaluation
6 years ago

7 new commits added

  • GPO: Do not crash during group policy evaluation
  • MAN: Provide minimum information on GPO access control
  • GPO: Improve logging of GPO security filtering
  • GPO: Group policy access evaluation not in line with [MS-ADTS]
  • GPO: Close group policy file after copying
  • GPO: Support group policy file main folders with upper case name
  • GPO: Grant access if DACL is not present
6 years ago

@lslebodn: Thank you for taking care of regression testing.

For my tests I created simple GPOs and modified allow/deny ACEs as well as read access to the GPO itself on a Windows machine. Then I went to the Linux server and tried to login.
Would this be helpful for you?

New patch added:

When evavluating some GPOs for lslebodn, SSSD crashed (free(): invalid pointer). It seems that the free() function that came in during Michals review causes this. Replaced free() by sss_idmap_free_sid() to free the mapped trustee SID.

7 new commits added

  • GPO: Do not crash during group policy evaluation
  • MAN: Provide minimum information on GPO access control
  • GPO: Improve logging of GPO security filtering
  • GPO: Group policy access evaluation not in line with [MS-ADTS]
  • GPO: Close group policy file after copying
  • GPO: Support group policy file main folders with upper case name
  • GPO: Grant access if DACL is not present
6 years ago

7 new commits added

  • GPO: Do not crash during group policy evaluation
  • MAN: Provide minimum information on GPO access control
  • GPO: Improve logging of GPO security filtering
  • GPO: Group policy access evaluation not in line with [MS-ADTS]
  • GPO: Close group policy file after copying
  • GPO: Support group policy file main folders with upper case name
  • GPO: Grant access if DACL is not present
6 years ago

@thor: Hi Thomas,

good that it crashed for you. I clearly added wrong 'free' function there but somehow managed to pass the evaluations without crashing.

Could you please squash the new patch to the patch that introduced the wrong free's? These patches are not yet in master so it does not make much sense to add new patch to fix an issue that is in another unpushed patch. It is better to fix the wrong patch.

You can use 'git rebase -i' then change the order of the last patch to follow the patch that introduced the wrong frees and use the 'f' for 'fixup' the new patch.

Thanks.
Michal

On (24/11/17 01:35), Thomas Reim wrote:

thor commented on the pull-request: Fix group policy security filtering and download that you are following:
``
@lslebodn: Thank you for taking care of regression testing.

For my tests I created simple GPOs and modified allow/deny ACEs as well as read access to the GPO itself on a Windows machine. Then I went to the Linux server and tried to login.
Would this be helpful for you?

It would be good to provide a little bit more details about GPOs.
Ideally with exact commands which you used when creating them with samba-ad-dc.

It will be helpful twice
verifying that it really fixed you bug :-)
and prevent any regressions/bugs in future.

Because bugs in GPO would be considered as security bugs due to deny feature.
ad_gpo_map_deny.

LS

6 new commits added

  • MAN: Provide minimum information on GPO access control
  • GPO: Improve logging of GPO security filtering
  • GPO: Group policy access evaluation not in line with [MS-ADTS]
  • GPO: Close group policy file after copying
  • GPO: Support group policy file main folders with upper case name
  • GPO: Grant access if DACL is not present
6 years ago

Could you please squash the new patch to the patch that introduced the wrong free's? These patches are not yet in master so it does not make much sense to add new patch to fix an issue that is in another unpushed patch. It is better to fix the wrong patch.

Commit GPO: Do not crash during group policy evaluation sqashed into commit a53f4fbda as mzidek suggested. Thank you for the request, looks much better now. :wink:

Please, replace "follwing" with "following".

Please, replace "i. e." with "i.e.".

"is" should be replaced with "are", shouldn't it?

I believe that 'is' is correct here, as we are talking about the debug level, which has a plural name 'Trace messages for operation functions'.

Shall I rebase the PR and push the corrected commits again?

Thank you for explanation. I was not aware of that.

From my point of view (not an SSSD developer) it would be good to rebase and push the corrected commits.

rebased onto 7f3fdf82eb3381833b55cf3d396db6f53299c3f3

6 years ago

From my point of view (not an SSSD developer) it would be good to rebase and push the corrected commits.

Your comments have been integrated and updated PR has been rebased.

Test cases will follow in cw52 (hopefully!)

It would be good to provide a little bit more details about GPOs.
Ideally with exact commands which you used when creating them with samba-ad-dc.
It will be helpful twice
verifying that it really fixed you bug :-)
and prevent any regressions/bugs in future.
Because bugs in GPO would be considered as security bugs due to deny feature.
ad_gpo_map_deny.

@lslebodn:
Sorry for my late reply. It's pretty complicated to configure GPOs using Samba 4 command-line tools. But finally I was able to setup the required regression test framework and succesfully perform the specified tests on the servers here.

You will find a detailed specification including GPO templates and some configuration files as attachment to my comment for issue #3324 that is related to this PR.

Currently, I'm investigating on a simple way to modify GPO ACLs. As soon as I have found a solution, I will add two or three further regression tests.

In parallel I would appreciate your confirming me the feasibility of implementing the specified regression tests in your environment.

rebased onto 1c5dc5c54e88d23d6b7e6a61fd1a7359cdda2529

6 years ago

6 new commits added

  • MAN: Provide minimum information on GPO access control
  • GPO: Improve logging of GPO security filtering
  • GPO: Group policy access evaluation not in line with [MS-ADTS]
  • GPO: Close group policy file after copying
  • GPO: Support group policy file main folders with upper case name
  • GPO: Grant access if DACL is not present
6 years ago

Now that 1.16.0 is out, can this go in?

This PR depends on having clean downstream test results which is something @mzidek is working on.

rebased onto a4b181bc97d17a361533b6eec9a6cb6729dc7323

6 years ago

rebased onto 1532aa4b27057b4e6a3c1668dfa7d51f1572db1f

6 years ago

rebased onto 185a5776d3334b5693632d0d1a9b659543d385a1

5 years ago

rebased onto 4dc9dc1

5 years ago

Got clean test results yet?

Nitpick: perhaps this should go after done: so close will be called if the memory allocation or reading the file has failed

bigpick: why is this still not pulled into sssd?

Correct, Reading the GPO file's content might fail and in this case we would leave the file open. This would prevent Samba client lib from closing the context.
I will update the code, but first need to fix some issues which prevent me from rebasing the PR to the latest SSSD baseline. It's now more than a year that the PR has not found its way into the main stream. :-)

Update on 02.10.2018:
Patch has been rebased and updated as requested.

bigpick: why is this still not pulled into sssd?

The issue seems to have low priority :-(

The issue seems to have low priority :-(

Hi,

I have this on my todo list for a very long time. The issue was that I was too afraid to push these patches without having green test results from our internal downstream tests and there are some issues preventing me smoothly debugging the results from those tests (and tests that use Windows machines are particularly problematic). I always ended up switching to a different task after I spent some time with this. Now, some of those issues with our tests were fixed and some failures were identified as false positives, so I hope that after the most immediate deadlines with higher priority I will get to this task for the last time.

My apologies to everyone who is waiting for these patches to be merged and especially @thor (the contributor who wrote the patches).

Michal

rebased onto 156b7d46a9dcf06b30676ba83409c79576686d85

5 years ago

rebased onto b563091

5 years ago

6 new commits added

  • MAN: Provide minimum information on GPO access control
  • GPO: Improve logging of GPO security filtering
  • GPO: Group policy access evaluation not in line with [MS-ADTS]
  • GPO: Close group policy file after copying
  • GPO: Support group policy file main folders with upper case name
  • GPO: Grant access if DACL is not present
5 years ago

@mzidek , this has really taken way too long.

@jockesssd I know and it will actually have to wait at least another week :/ But given how long it is here already it is not that much time I guess...

@mzidek been a very long week now ...

rebased onto 9252d81

4 years ago

rebased onto 4f3aef7

4 years ago

rebased onto 376d3bc

4 years ago

@jockesssd I do not feel comfortable giving any predictions or promises regarding this anymore, but this task is in my current sprint, so I hope it will be done within the time allocated for it (three weeks).

@thor Hi, sorry for the delay in response. As I will not have much time to work on SSSD in the future, I am trying to bring some things from the past to conclusion.

I spent some time with this recently, but unfortunately as of now (or at least two days ago) I still was not able to use the AD/GPO test suite and get green results (there are some issues with the testing infrastructure, that are being worked on, they do have nothing to do with these patches).

I think there are two things that can be done here.
1. Still block these patches until the tests are available again and then push if they are green
2. rely on just manual testing and push these patches

I really wanted to do 1, but now I think it was probably a mistake. The patches were reviewed long time ago and the issues in tests still can be fixed before the releases even if these patches break something (especially now as major downstream releases are relatively far away) and most importantly these patches do fix some issues.

In both cases I think it would be good to move this PR to Github as most activity happens there now and there are also CI tests (even though they do not test AD/GPO code much).

@thor , I know this PR takes ridiculously long time to process, but could you open the PR again on Github and close it here? It would make things more visible and I think it may speed the process up.

If not then I think the best would be for some SSSD developer to cherry pick patches from this PR and review/push them individually as I think they do have a value.

@mzidek , you should at least assign this PR to one of the other sssd devs, not just dump it.

Metadata Update from @pbrezina:
- Request assigned

4 years ago

I'm very sorry this took so long, it certainly sheds a bad light on us. I assigned this PR to Pawel and he will prioritize it so hopefully we will merge this soon. Thank you for your patience.

Any chance it will be in next release?

No promises, but unless there will be issues found with the patches I think it will be included.

This pull request has been rebased to latest master and moved to upstream repo on GitHub:
https://github.com/SSSD/sssd/pull/1022

@pbrezina Should we close it here now or wait for uptream PR to be merged first?
@thor I am very sorry for delay in handling this PR, in case of any comments please feel free to ad them here, on GitHub or email me directly.

Since the PR was moved to Github and thor already contributed there, I'm closing this PR. Thank you.

Pull-Request has been closed by pbrezina

4 years ago