[Daclread] New module option - Filter interesting-only permissions#973
[Daclread] New module option - Filter interesting-only permissions#973njutn95 wants to merge 1 commit into
Conversation
njutn95
requested review from
Marshall-Hallenbeck,
NeffIsBack,
mpgn and
zblurx
as code owners
|
Thanks for the PR! I will take a look at it when i have reviewed the pile of PRs that have accumulated. |
| ACE_TYPE The type of ACE to read (Allowed or Denied) | ||
| RIGHTS An interesting right to filter on ('FullControl', 'ResetPassword', 'WriteMembers', 'DCSync') | ||
| RIGHTS_GUID A right GUID that specify a particular rights to filter on | ||
| INTERESTING_ONLY Whether to exclude common or default permissions and only focus on more specific, significant ones |
There was a problem hiding this comment.
Please reword this, the filter is based on trustee SID (RID ≥ 1000) and access-mask patterns, not on “common permissions"
| for ace in dacl["Data"]: | ||
| parsed_ace = self.parse_ace(ace) | ||
| parsed_dacl.append(parsed_ace) | ||
| if self.interesting_only: |
There was a problem hiding this comment.
Use .get() and continue when keys are missing, or skip unsupported ACEs early
| parsed_dacl.append(parsed_ace) | ||
| if self.interesting_only: | ||
| # only process SIDs > 1000 | ||
| if (re.search(r"\(S-1-5-.*-[1-9]\d{3,}\)$", parsed_ace["Trustee (SID)"]) and |
There was a problem hiding this comment.
Please switch to parsing the canonical SID (e.g. extract RID from ace['Ace']['Sid'] before resolveSID) instead of regex on the formatted "Name (S-1-5-...)" string, more reliable and avoids LDAP lookups for ACEs you’ll drop anyway.
| # only process SIDs > 1000 | ||
| if (re.search(r"\(S-1-5-.*-[1-9]\d{3,}\)$", parsed_ace["Trustee (SID)"]) and | ||
| (re.search(r"GenericAll|Write|Create|Delete|FullControl|Modify|ControlAccess", parsed_ace["Access mask"]) or | ||
| ("ExtendedRight" in parsed_ace["Access mask"] and "ACCESS_ALLOWED" in parsed_ace["ACE Type"])) |
There was a problem hiding this comment.
This will KeyError on standard ACCESS_ALLOWED_ACE / ACCESS_DENIED_ACE because parse_ace() overwrites the dict and drops "ACE Type" (see L416–419). Please either keep "ACE Type" in the standard-ACE branch
| self.rights_guid = module_options["RIGHTS_GUID"] | ||
|
|
||
| if "INTERESTING_ONLY" in module_options: | ||
| self.interesting_only = module_options["INTERESTING_ONLY"].lower() in ["true", "1", "yes"] |
There was a problem hiding this comment.
Please use .get() to avoid relying on key presence and keep it consistent
| # only process SIDs > 1000 | ||
| if (re.search(r"\(S-1-5-.*-[1-9]\d{3,}\)$", parsed_ace["Trustee (SID)"]) and | ||
| (re.search(r"GenericAll|Write|Create|Delete|FullControl|Modify|ControlAccess", parsed_ace["Access mask"]) or | ||
| ("ExtendedRight" in parsed_ace["Access mask"] and "ACCESS_ALLOWED" in parsed_ace["ACE Type"])) |
There was a problem hiding this comment.
Please switch "ExtendedRight in ..." to an explicit match. It accidentally matches AllExtendedRights and never matches the real flag names (ControlAccess, etc.). Use the actual permission names produced by the enums instead of a substring check.
4 participants
Description
Add the
INTERESTING_ONLYboolean option to the daclread module. This allows filtering a response of 50+ ACEs into 1-5 relevant entries. It does so by removing ACE entries that can be performed with users/groups whose SID is below 1000 (default groups that share common permissions), and excluding Read capabilities, which are in 99% considered irrelevant. This allows the user to see permissions such as "John Doe can change the email address of Will Smith" in a matter of a couple of seconds.Type of change
Insert an "x" inside the brackets for relevant items (do not delete options)
Setup guide for the review
Run the command using the
and
whilst targeting a user that can be modified by a non-default user/group.
Screenshots (if appropriate):
Running the command without the flag executes the default module behavior of listing all ACEs.
Running the command with the option flag enabled returns an actionable list of ACEs.
Checklist:
Insert an "x" inside the brackets for completed and relevant items (do not delete options)
poetry run python -m ruff check . --preview, use--fixto automatically fix what it can)tests/e2e_commands.txtfile if necessary (new modules or features are required to be added to the e2e tests)