From 811082daf82763bbb50aa6e5db0cb9a166ffd710 Mon Sep 17 00:00:00 2001 From: Shawn Hardwick Date: Tue, 30 Mar 2021 13:57:35 -0400 Subject: [PATCH 1/4] Remove conversion of user rights identities when setting user rights resource; Unresolvable SIDs found on local machine would cause resource to fail. During a configure operation (secedit.exe /configure), secedit will happily accept either style in the .inf file. It will also accept and de-duplicate security principals that are specified as an account name and SID in the same line item under the [Privileges] section. --- .../MSFT_UserRightsAssignment.psm1 | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source/DSCResources/MSFT_UserRightsAssignment/MSFT_UserRightsAssignment.psm1 b/source/DSCResources/MSFT_UserRightsAssignment/MSFT_UserRightsAssignment.psm1 index eace8b0..edf9ff5 100644 --- a/source/DSCResources/MSFT_UserRightsAssignment/MSFT_UserRightsAssignment.psm1 +++ b/source/DSCResources/MSFT_UserRightsAssignment/MSFT_UserRightsAssignment.psm1 @@ -218,7 +218,12 @@ function Set-TargetResource { if ($id -notin $accounts) { - $accounts += ConvertTo-LocalFriendlyName -Identity $id -Policy $Policy -Scope 'Set' + # SID entries start with asterisk in user rights INF + if ($id -match '^(S-[0-9-]{3,})') + { + $id = "*$id" + } + $accounts += $id } } } From 38883c17b954f0d848225321bcf873a608db33d1 Mon Sep 17 00:00:00 2001 From: Shawn Hardwick Date: Tue, 30 Mar 2021 17:27:26 -0400 Subject: [PATCH 2/4] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a478bc3..b8aec8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,9 @@ - Ensure `Get` method returns the specified `Name` property. - Fix applying Account_lockout_duration to zero [Issue #140](https://github.com/dsccommunity/SecurityPolicyDsc/issues/140). +- UserRightsAssignment: + - Allow unresolvable SIDs found in local security policy + [Issue #158](https://github.com/dsccommunity/SecurityPolicyDsc/issues/158) ## [2.10.0.0] - 2019-09-19 From 59aef67c452eeaebf501ded74ff09fd5b10e153a Mon Sep 17 00:00:00 2001 From: Shawn Hardwick Date: Tue, 30 Mar 2021 17:56:18 -0400 Subject: [PATCH 3/4] Add unit tests --- .../Unit/MSFT_UserRightsAssignment.tests.ps1 | 50 +++++++++++++++---- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 b/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 index e6626f4..a0c63fc 100644 --- a/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 +++ b/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 @@ -60,10 +60,15 @@ try Identity = $null } + $mockUnresolvableSID = [PSObject] @{ + Policy = 'SeBatchLogonRight' + Identity = '*S-1-5-21-577511119-1435111626-1914111595-3711104' + } + #endregion #region Function Get-TargetResource - Describe "Get-TargetResource" { + Describe "Get-TargetResource" { Context 'Identity should match on Policy' { Mock -CommandName Get-UserRightPolicy -MockWith {return @($testParameters)} Mock -CommandName Test-TargetResource -MockWith {$false} @@ -119,7 +124,7 @@ try It 'Should call expected mocks' { Assert-MockCalled -CommandName Get-UserRightPolicy -Exactly 1 - } + } } Context 'Identity is NULL and should be' { @@ -137,7 +142,7 @@ try } Context 'Tests for when Identity is a local account or SID' { - $mockGetUSRPolicyResult = $mockGetUSRPolicyResult.Clone() + $mockGetUSRPolicyResult = $mockGetUSRPolicyResult.Clone() It 'Should return True when a SID is used for Identity' { @@ -154,15 +159,15 @@ try Context 'Identity does not exist but should' { Mock -CommandName Invoke-Secedit -MockWith {} Mock -CommandName Test-TargetResource -MockWith {$true} - Mock -CommandName Get-Content -ParameterFilter {$Path -match "Secedit-OutPut.txt"} -MockWith {"Tasked Failed"} + Mock -CommandName Get-Content -ParameterFilter {$Path -match "Secedit-OutPut.txt"} -MockWith {"Tasked Failed"} Mock -CommandName ConvertTo-LocalFriendlyName -MockWith {'contoso\testuser1'} - - It 'Should not throw' { + + It 'Should not throw' { {Set-TargetResource @testParameters} | Should Not Throw } It 'Should throw when set fails' { - Mock Test-TargetResource -MockWith {$false} + Mock Test-TargetResource -MockWith {$false} {Set-TargetResource @testParameters} | Should Throw $script:localizedData.TaskFail } @@ -172,20 +177,43 @@ try } } + Context 'Unresolvable SID exists' { + $mockUnresolvableSID = @{ + Policy = 'SeBatchLogonRight' + Identity = '*S-1-5-21-577511119-1435111626-1914111595-3711104' + } + $setParameters = @{ + Policy = 'Log_on_as_a_batch_job' + Identity = 'contoso\TestUser1' + } + Mock -CommandName Get-UserRightPolicy -MockWith {$mockUnresolvableSID} + Mock -CommandName Invoke-Secedit -MockWith {} + + It 'Should not throw' { + Mock -CommandName Test-TargetResource -MockWith {$true} + {Set-TargetResource @setParameters} | Should Not Throw + } + + It 'Should call expected mocks' { + Assert-MockCalled -CommandName Invoke-Secedit -Exactly 2 + Assert-MockCalled -CommandName Test-TargetResource -Exactly 2 + } + } + Context 'Identity is NULL' { It 'Should not throw' { Mock -CommandName Invoke-Secedit -MockWith {} - Mock -CommandName Test-TargetResource -MockWith {$true} + Mock -CommandName Test-TargetResource -MockWith {$true} $setParameters = @{ Policy = 'Access_Credential_Manager_as_a_trusted_caller' Identity = "" - } + } {Set-TargetResource @setParameters} | Should Not Throw } It 'Should call expected mocks' { Assert-MockCalled -CommandName Invoke-Secedit - Assert-MockCalled -CommandName Test-TargetResource + Assert-MockCalled -CommandName Test-TargetResource } } } @@ -219,7 +247,7 @@ try $constant | Should Be 'SeTrustedCredManAccessPrivilege' } } - #endregion + #endregion } #endregion } From 52b8200b5142384371aa54763a2a31c8ec3761f5 Mon Sep 17 00:00:00 2001 From: Shawn Hardwick Date: Tue, 30 Mar 2021 20:07:30 -0400 Subject: [PATCH 4/4] Fix unit test for unresolvable SIDs --- Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 b/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 index a0c63fc..c22cb2a 100644 --- a/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 +++ b/Tests/Unit/MSFT_UserRightsAssignment.tests.ps1 @@ -188,16 +188,12 @@ try } Mock -CommandName Get-UserRightPolicy -MockWith {$mockUnresolvableSID} Mock -CommandName Invoke-Secedit -MockWith {} + Mock -CommandName ConvertTo-LocalFriendlyName -MockWith {'contoso\testUser1'} It 'Should not throw' { Mock -CommandName Test-TargetResource -MockWith {$true} {Set-TargetResource @setParameters} | Should Not Throw } - - It 'Should call expected mocks' { - Assert-MockCalled -CommandName Invoke-Secedit -Exactly 2 - Assert-MockCalled -CommandName Test-TargetResource -Exactly 2 - } } Context 'Identity is NULL' {