-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error handling of SID translation and manual translation for Domain Controllers #74
base: dev
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #74 +/- ##
=================================
- Coverage 81% 80% -1%
=================================
Files 5 5
Lines 417 420 +3
=================================
Hits 338 338
- Misses 79 82 +3 |
This needs to be merged...serious problem, causing DSC application to fail, and is referenced in other issues. I would suggest perhaps resolving other SIDs locally as per https://github.com/PowerShellMafia/PowerSploit/blob/master/Recon/PowerView.ps1#L918 e.g. (version below includes more mappings)
|
Just ran into this issue. This happened for me when resolving a user from a different domain. |
@hackjammer is right. Looks like it's failing for me on resolving Built-In Account Sids like the "Administrators Group" Here is the code that iterates through all of the privilege rights Switch($Area)
{
"USER_RIGHTS"
{
$returnValue = @{}
$privilegeRights = $policyConfiguration.'Privilege Rights'
foreach ($key in $privilegeRights.keys )
{
$identity = ConvertTo-LocalFriendlyName -Identity $($privilegeRights[$key] -split ",").Trim()
$returnValue.Add( $key,$identity )
}
continue
}
Default
{
$returnValue = $policyConfiguration
}
} And here is the example output of Sids that will get translated
|
Reviewed 1 of 1 files at r1. DSCResources/SecurityPolicyResourceHelper/SecurityPolicyResourceHelper.psm1, line 341 at r1 (raw file):
Running this I get the expected result without this change. In what scenario(s) does this fail to return the correct value?
Comments from Reviewable |
Note: This PR continues the work from PR #72 by @rdbartram. PR #72 was closed in favor of this PR. |
This PR should probably be abandoned because the scenario when a machine cannot resolve "Window Manager\Window Manager Group" is when the group does not exist, such as Sever Core. On a server OS with a desktop it can translate the identity as expected. There are issues with identities that cannot be translated but I don't think this fits in that scenario. |
I suggest to keep this issue open until the problem has been resolved, either through this PR or through another PR. |
My PR #72 was not to do with nano server rather SIDs belonging to objects that no longer exist in AD. This extends also to SIDs that can't be resolved because the resolve is executed under an account that doesn't have access to resolve said SID. Think local user against domain, foreign domain, one way trust etc |
@rdbartram I think that is discussed in issue #78? If not, please make sure that is part of that discussion. 🙂 |
Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again. |
@hackjammer can you give me an update on this problem? Could you describe what you do to reproduce this issue? |
To my knowledge the issue with SID translation has been fixed with PR #97. This PR was opened to handle the absence of "Window Manager\Window Manager Group" which doesn't exist unless the Server has a desktop. |
Running the DC GPO setting against a DC will prompt you with errors regarding translating security ID.
Troubleshooting turns out that a DC can't resolve S-1-5-90-0 ("Window Manager\Window Manager Group"). This piece of code introduce error handling plus a manual translation for the SID for "Window Manager\Window Manager Group".
This change is