From 352238f8e718f2b132627acc32e99ba0a0b3d478 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Mon, 8 Dec 2025 16:19:23 -0700 Subject: [PATCH 1/9] write to compStatus for CA lookups --- .../Processors/CertAbuseProcessor.cs | 61 ++++++++++++++++++- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index 86513f5a..7c352b43 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -23,7 +23,6 @@ public class CertAbuseProcessor public delegate Task ComputerStatusDelegate(CSVComputerStatus status); public event ComputerStatusDelegate ComputerStatusEvent; - public CertAbuseProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; _log = log ?? Logging.LogProvider.CreateLogger("CAProc"); @@ -47,9 +46,23 @@ public async Task ProcessRegistryEnrollmentPermissions(str data.Collected = aceData.Collected; if (!aceData.Collected) { + await SendComputerStatus(new CSVComputerStatus { + Status = aceData.FailureReason, + Task = nameof(ProcessRegistryEnrollmentPermissions), + ComputerName = caName, + ObjectId = computerObjectId, + }); + data.FailureReason = aceData.FailureReason; return data; } + + await SendComputerStatus(new CSVComputerStatus { + Status = CSVComputerStatus.StatusSuccess, + Task = nameof(ProcessRegistryEnrollmentPermissions), + ComputerName = caName, + ObjectId = computerObjectId, + }); if (aceData.Value == null) { @@ -167,9 +180,23 @@ public async Task ProcessEAPermissions(string ret.Collected = regData.Collected; if (!ret.Collected) { + await SendComputerStatus(new CSVComputerStatus { + Status = regData.FailureReason, + Task = nameof(ProcessEAPermissions), + ComputerName = caName, + ObjectId = computerObjectId, + }); + ret.FailureReason = regData.FailureReason; return ret; } + + await SendComputerStatus(new CSVComputerStatus { + Status = CSVComputerStatus.StatusSuccess, + Task = nameof(ProcessEAPermissions), + ComputerName = caName, + ObjectId = computerObjectId, + }); if (regData.Value == null) { @@ -251,7 +278,7 @@ private RegistryResult GetEnrollmentAgentRights(string target, string caName) /// /// [ExcludeFromCodeCoverage] - public BoolRegistryAPIResult IsUserSpecifiesSanEnabled(string target, string caName) + public async Task IsUserSpecifiesSanEnabled(string target, string caName, string hostSid) { var ret = new BoolRegistryAPIResult(); var subKey = @@ -262,9 +289,23 @@ public BoolRegistryAPIResult IsUserSpecifiesSanEnabled(string target, string caN ret.Collected = data.Collected; if (!data.Collected) { + await SendComputerStatus(new CSVComputerStatus { + Status = data.FailureReason, + Task = nameof(IsUserSpecifiesSanEnabled), + ComputerName = caName, + ObjectId = hostSid + }); + ret.FailureReason = data.FailureReason; return ret; } + + await SendComputerStatus(new CSVComputerStatus { + Status = CSVComputerStatus.StatusSuccess, + Task = nameof(IsUserSpecifiesSanEnabled), + ComputerName = caName, + ObjectId = hostSid + }); if (data.Value == null) { @@ -287,7 +328,7 @@ public BoolRegistryAPIResult IsUserSpecifiesSanEnabled(string target, string caN /// /// [ExcludeFromCodeCoverage] - public BoolRegistryAPIResult RoleSeparationEnabled(string target, string caName) + public async Task RoleSeparationEnabled(string target, string caName, string hostSid) { var ret = new BoolRegistryAPIResult(); var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; @@ -297,9 +338,23 @@ public BoolRegistryAPIResult RoleSeparationEnabled(string target, string caName) ret.Collected = data.Collected; if (!data.Collected) { + await SendComputerStatus(new CSVComputerStatus { + Status = data.FailureReason, + Task = nameof(RoleSeparationEnabled), + ComputerName = caName, + ObjectId = hostSid + }); + ret.FailureReason = data.FailureReason; return ret; } + + await SendComputerStatus(new CSVComputerStatus { + Status = CSVComputerStatus.StatusSuccess, + Task = nameof(RoleSeparationEnabled), + ComputerName = caName, + ObjectId = hostSid + }); if (data.Value == null) { From 0c72806cc65fe190ec4f20f6a9f24c0142c510e9 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Wed, 10 Dec 2025 06:53:44 -0700 Subject: [PATCH 2/9] move remote registry dependencies out of static helpers --- src/CommonLib/Helpers.cs | 12 +- src/CommonLib/IRegistryAccessor.cs | 76 ++++++++++ src/CommonLib/IRegistryKey.cs | 4 +- .../Processors/CertAbuseProcessor.cs | 22 ++- test/unit/CertAbuseProcessorTest.cs | 137 ++++++++++++++++-- 5 files changed, 227 insertions(+), 24 deletions(-) create mode 100644 src/CommonLib/IRegistryAccessor.cs diff --git a/src/CommonLib/Helpers.cs b/src/CommonLib/Helpers.cs index 6b89ae9c..0033fa2a 100644 --- a/src/CommonLib/Helpers.cs +++ b/src/CommonLib/Helpers.cs @@ -1,17 +1,17 @@ using System; using System.Collections.Generic; using System.Globalization; +using System.IO; using System.Linq; +using System.Security; using System.Security.Principal; using System.Text; using System.Text.RegularExpressions; using SharpHoundCommonLib.Enums; using Microsoft.Extensions.Logging; -using System.IO; -using System.Security; -using SharpHoundCommonLib.Processors; -using Microsoft.Win32; using System.Threading.Tasks; +using Microsoft.Win32; +using SharpHoundCommonLib.Processors; namespace SharpHoundCommonLib { public static class Helpers { @@ -151,7 +151,7 @@ public static string DistinguishedNameToDomain(string distinguishedName) { } /// - /// Converts a domain name to a distinguished name using simple string substitution + /// Converts a domain name to a distinguished name using simple string substitution /// /// /// @@ -258,6 +258,7 @@ public static bool IsSidFiltered(string sid) { return false; } + //TODO: replace with IRegistryAccessor public static RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue, ILogger log) { var data = new RegistryResult(); @@ -292,6 +293,7 @@ public static RegistryResult GetRegistryKeyData(string target, string subkey, st return data; } + //TODO: replace with IRegistryAccessor public static IRegistryKey OpenRemoteRegistry(string target) { return SHRegistryKey.Connect(RegistryHive.LocalMachine, target).GetAwaiter().GetResult(); } diff --git a/src/CommonLib/IRegistryAccessor.cs b/src/CommonLib/IRegistryAccessor.cs new file mode 100644 index 00000000..d6ceb5fb --- /dev/null +++ b/src/CommonLib/IRegistryAccessor.cs @@ -0,0 +1,76 @@ +using System; +using System.IO; +using System.Security; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Win32; +using SharpHoundCommonLib.Processors; + +namespace SharpHoundCommonLib { + public interface IRegistryAccessor { + public RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue, ILogger log); + public IRegistryKey OpenRemoteRegistry(string target); + public Task Connect(RegistryHive hive, string machineName); + } + + public class RegistryAccessor : IRegistryAccessor { + private static readonly AdaptiveTimeout _adaptiveTimeout = + new AdaptiveTimeout(maxTimeout: TimeSpan.FromSeconds(10), Logging.LogProvider.CreateLogger(nameof(SHRegistryKey))); + + public RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue, ILogger log) { + var data = new RegistryResult(); + + try { + var baseKey = OpenRemoteRegistry(target); + var value = baseKey.GetValue(subkey, subvalue); + data.Value = value; + data.Collected = true; + } + catch (IOException e) { + log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + target, subkey, subvalue); + data.FailureReason = "Target machine was not found or not connectable"; + } + catch (SecurityException e) { + log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + target, subkey, subvalue); + data.FailureReason = "User does not have the proper permissions to perform this operation"; + } + catch (UnauthorizedAccessException e) { + log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + target, subkey, subvalue); + data.FailureReason = "User does not have the necessary registry rights"; + } + catch (Exception e) { + log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + target, subkey, subvalue); + data.FailureReason = e.Message; + } + + return data; + } + + public IRegistryKey OpenRemoteRegistry(string target) { + return Connect(RegistryHive.LocalMachine, target).GetAwaiter().GetResult(); + } + + /// + /// Gets a handle to a remote registry. + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + public async Task Connect(RegistryHive hive, string machineName) { + var remoteKey = await _adaptiveTimeout.ExecuteWithTimeout((_) => RegistryKey.OpenRemoteBaseKey(hive, machineName)); + if (remoteKey.IsSuccess) + return new SHRegistryKey(remoteKey.Value); + throw new TimeoutException($"Failed to connect to registry on {machineName}: {remoteKey.Error}"); + } + } +} \ No newline at end of file diff --git a/src/CommonLib/IRegistryKey.cs b/src/CommonLib/IRegistryKey.cs index fd266265..43c3a0e9 100644 --- a/src/CommonLib/IRegistryKey.cs +++ b/src/CommonLib/IRegistryKey.cs @@ -12,7 +12,7 @@ public class SHRegistryKey : IRegistryKey, IDisposable { private readonly RegistryKey _currentKey; private static readonly AdaptiveTimeout _adaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromSeconds(10), Logging.LogProvider.CreateLogger(nameof(SHRegistryKey))); - private SHRegistryKey(RegistryKey registryKey) { + public SHRegistryKey(RegistryKey registryKey) { _currentKey = registryKey; } @@ -35,6 +35,8 @@ public object GetValue(string subkey, string name) { /// /// /// + /// + //TODO: replace with IRegistryAccessor public static async Task Connect(RegistryHive hive, string machineName) { var remoteKey = await _adaptiveTimeout.ExecuteWithTimeout((_) => RegistryKey.OpenRemoteBaseKey(hive, machineName)); if (remoteKey.IsSuccess) diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index 7c352b43..e6f0d6d3 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -20,11 +20,14 @@ public class CertAbuseProcessor private readonly ILdapUtils _utils; private readonly AdaptiveTimeout _getMachineSidAdaptiveTimeout; private readonly AdaptiveTimeout _openSamServerAdaptiveTimeout; + private readonly IRegistryAccessor _registryAccessor; + public delegate Task ComputerStatusDelegate(CSVComputerStatus status); public event ComputerStatusDelegate ComputerStatusEvent; - public CertAbuseProcessor(ILdapUtils utils, ILogger log = null) { + public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, ILogger log = null) { _utils = utils; + _registryAccessor = registryAccessor; _log = log ?? Logging.LogProvider.CreateLogger("CAProc"); _getMachineSidAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(ISAMServer.GetMachineSid))); _openSamServerAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(SAMServer.OpenServer))); @@ -251,7 +254,8 @@ private RegistryResult GetCASecurity(string target, string caName) var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; const string regValue = "Security"; - return Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); + return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + // return Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); } /// @@ -266,7 +270,8 @@ private RegistryResult GetEnrollmentAgentRights(string target, string caName) var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; var regValue = "EnrollmentAgentRights"; - return Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); + return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + // return Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); } /// @@ -281,10 +286,11 @@ private RegistryResult GetEnrollmentAgentRights(string target, string caName) public async Task IsUserSpecifiesSanEnabled(string target, string caName, string hostSid) { var ret = new BoolRegistryAPIResult(); - var subKey = + var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; - const string subValue = "EditFlags"; - var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); + const string regValue = "EditFlags"; + var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + // var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); ret.Collected = data.Collected; if (!data.Collected) @@ -333,7 +339,8 @@ public async Task RoleSeparationEnabled(string target, st var ret = new BoolRegistryAPIResult(); var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; const string regValue = "RoleSeparationEnabled"; - var data = Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + // var data = Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); ret.Collected = data.Collected; if (!data.Collected) @@ -518,7 +525,6 @@ private async Task SendComputerStatus(CSVComputerStatus status) { if (ComputerStatusEvent is not null) await ComputerStatusEvent(status); } - } public class EnrollmentAgentRestriction diff --git a/test/unit/CertAbuseProcessorTest.cs b/test/unit/CertAbuseProcessorTest.cs index 17efe86f..ceee6183 100644 --- a/test/unit/CertAbuseProcessorTest.cs +++ b/test/unit/CertAbuseProcessorTest.cs @@ -1,5 +1,6 @@ using System; using System.DirectoryServices; +using System.Runtime.Versioning; using System.Threading.Tasks; using CommonLibTest.Facades; using Moq; @@ -8,20 +9,98 @@ using SharpHoundCommonLib.Processors; using Xunit; using Xunit.Abstractions; +using FluentAssertions.Events; +using Microsoft.Extensions.Logging; -namespace CommonLibTest { - //TODO: Make these tests work - public class CertAbuseProcessorTest : IDisposable { - private const string CASecurityFixture = - "AQAUhCABAAAwAQAAFAAAAEQAAAACADAAAgAAAALAFAD//wAAAQEAAAAAAAEAAAAAAsAUAP//AAABAQAAAAAABQcAAAACANwABwAAAAADGAABAAAAAQIAAAAAAAUgAAAAIAIAAAADGAACAAAAAQIAAAAAAAUgAAAAIAIAAAADJAABAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQAAIAAAADJAACAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQAAIAAAADJAABAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQBwIAAAADJAACAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQBwIAAAADFAAAAgAAAQEAAAAAAAULAAAAAQIAAAAAAAUgAAAAIAIAAAECAAAAAAAFIAAAACACAAA="; +namespace CommonLibTest +{ + public class CertAbuseProcessorTest + { + // private const string CASecurityFixture = + // "AQAUhCABAAAwAQAAFAAAAEQAAAACADAAAgAAAALAFAD//wAAAQEAAAAAAAEAAAAAAsAUAP//AAABAQAAAAAABQcAAAACANwABwAAAAADGAABAAAAAQIAAAAAAAUgAAAAIAIAAAADGAACAAAAAQIAAAAAAAUgAAAAIAIAAAADJAABAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQAAIAAAADJAACAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQAAIAAAADJAABAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQBwIAAAADJAACAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQBwIAAAADFAAAAgAAAQEAAAAAAAULAAAAAQIAAAAAAAUgAAAAIAIAAAECAAAAAAAFIAAAACACAAA="; + + [Theory] + [InlineData(0x00040000, true)] + [InlineData(0x00020000, false)] + public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_ChecksKey(int editFlags, bool expectedResult) { + const string target = "target machine name"; + const string caName = "TEST-CA"; + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; + const string subValue = "EditFlags"; + const string hostSid = "S-1-5-21-ENTERPRISE-CA"; + + var mockRegistryAccessor = new Mock(); + mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + target, + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = true, Value = editFlags }); + + var processor = new CertAbuseProcessor(new MockLdapUtils(), mockRegistryAccessor.Object); + + CSVComputerStatus capturedStatus = null; + processor.ComputerStatusEvent += status => + { + capturedStatus = status; + return Task.CompletedTask; + }; - private readonly ITestOutputHelper _testOutputHelper; + var results = await processor.IsUserSpecifiesSanEnabled(target, caName, hostSid); - public CertAbuseProcessorTest(ITestOutputHelper testOutputHelper) { - _testOutputHelper = testOutputHelper; + //Validate result + Assert.Null(results.FailureReason); + Assert.True(results.Collected); + Assert.Equal(expectedResult, results.Value); + + //Validate CompStatus Log + Assert.Equal(caName, capturedStatus.ComputerName); + Assert.Equal(nameof(processor.IsUserSpecifiesSanEnabled), capturedStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, capturedStatus.Status); + Assert.Equal(hostSid, capturedStatus.ObjectId); } + + [Fact] + public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_FailsLookup() { + const string target = "target machine name"; + const string caName = "TEST-CA"; + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; + const string subValue = "EditFlags"; + const string hostSid = "S-1-5-21-ENTERPRISE-CA"; + + + const string failureReason = "Registry Lookup Failure"; + + var mockRegistryAccessor = new Mock(); + mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + target, + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = false, FailureReason = failureReason }); + + var processor = new CertAbuseProcessor(new MockLdapUtils(), mockRegistryAccessor.Object); + + CSVComputerStatus capturedStatus = null; + processor.ComputerStatusEvent += status => + { + capturedStatus = status; + return Task.CompletedTask; + }; + + var results = await processor.IsUserSpecifiesSanEnabled(target, caName, hostSid); - public void Dispose() { + //Validate result + Assert.Equal(failureReason, results.FailureReason); + Assert.False(results.Collected); + + //Validate CompStatus Log + Assert.Equal(caName, capturedStatus.ComputerName); + Assert.Equal(nameof(processor.IsUserSpecifiesSanEnabled), capturedStatus.Task); + Assert.Equal(failureReason, capturedStatus.Status); + Assert.Equal(hostSid, capturedStatus.ObjectId); } // [Fact] @@ -89,12 +168,25 @@ public void Dispose() { // public async Task CertAbuseProcessor_ProcessCAPermissions_NullSecurity_ReturnsNull() // { // var processor = new CertAbuseProcessor(new MockLdapUtils()); + // + // CSVComputerStatus capturedStatus = null; + // + // processor.ComputerStatusEvent += status => + // { + // capturedStatus = status; + // return Task.CompletedTask; + // }; // // var results = await processor.ProcessRegistryEnrollmentPermissions(null, "DUMPSTER.FIRE", null, "test"); // // Assert.Equal("Value cannot be null. (Parameter 'machineName')", results.FailureReason); // Assert.False(results.Collected); - // Assert.Null(results.Data); + // Assert.Empty(results.Data); + // + // Assert.Equal(null, capturedStatus.ComputerName); + // Assert.Equal("test", capturedStatus.ObjectId); + // Assert.Equal("Value cannot be null. (Parameter 'machineName')", capturedStatus.Status); + // Assert.Equal(nameof(processor.ProcessRegistryEnrollmentPermissions), capturedStatus.Task); // } // [WindowsOnlyFact] @@ -137,5 +229,30 @@ public void Dispose() { // x.PrincipalSID == "S-1-5-21-3130019616-2776909439-2417379446-519" && // !x.IsInherited); // } + + // [Fact] + // public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_HandlesFailure() + // { + // var processor = new CertAbuseProcessor(new MockLdapUtils()); + // + // CSVComputerStatus capturedStatus = null; + // + // processor.ComputerStatusEvent += status => + // { + // capturedStatus = status; + // return Task.CompletedTask; + // }; + // + // var results = await processor.IsUserSpecifiesSanEnabled("target", "DUMPSTER.FIRE", "sid"); + // + // Assert.Equal("Target machine was not found or not connectable", results.FailureReason); + // Assert.False(results.Collected); + // Assert.False(results.Value); + // + // Assert.Equal("DUMPSTER.FIRE", capturedStatus.ComputerName); + // Assert.Equal("sid", capturedStatus.ObjectId); + // Assert.Equal("Target machine was not found or not connectable", capturedStatus.Status); + // Assert.Equal(nameof(processor.IsUserSpecifiesSanEnabled), capturedStatus.Task); + // } } } \ No newline at end of file From 39c87b656ae923bad06e59db2069c3f160e2bcc1 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Wed, 10 Dec 2025 14:08:21 -0700 Subject: [PATCH 3/9] add certAbuseProc tests --- .../Processors/CertAbuseProcessor.cs | 4 - test/unit/CertAbuseProcessorTest.cs | 395 ++++++++++-------- test/unit/Facades/MockLdapUtils.cs | 9 +- test/unit/Utils.cs | 8 + 4 files changed, 245 insertions(+), 171 deletions(-) diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index e6f0d6d3..b723eac6 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -248,7 +248,6 @@ await SendComputerStatus(new CSVComputerStatus { /// /// /// - [ExcludeFromCodeCoverage] private RegistryResult GetCASecurity(string target, string caName) { var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; @@ -264,7 +263,6 @@ private RegistryResult GetCASecurity(string target, string caName) /// /// /// - [ExcludeFromCodeCoverage] private RegistryResult GetEnrollmentAgentRights(string target, string caName) { var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; @@ -282,7 +280,6 @@ private RegistryResult GetEnrollmentAgentRights(string target, string caName) /// /// /// - [ExcludeFromCodeCoverage] public async Task IsUserSpecifiesSanEnabled(string target, string caName, string hostSid) { var ret = new BoolRegistryAPIResult(); @@ -333,7 +330,6 @@ await SendComputerStatus(new CSVComputerStatus { /// /// /// - [ExcludeFromCodeCoverage] public async Task RoleSeparationEnabled(string target, string caName, string hostSid) { var ret = new BoolRegistryAPIResult(); diff --git a/test/unit/CertAbuseProcessorTest.cs b/test/unit/CertAbuseProcessorTest.cs index ceee6183..6768b74a 100644 --- a/test/unit/CertAbuseProcessorTest.cs +++ b/test/unit/CertAbuseProcessorTest.cs @@ -1,107 +1,286 @@ -using System; -using System.DirectoryServices; -using System.Runtime.Versioning; +using System.Security.Principal; using System.Threading.Tasks; using CommonLibTest.Facades; using Moq; -using Newtonsoft.Json; using SharpHoundCommonLib; using SharpHoundCommonLib.Processors; using Xunit; -using Xunit.Abstractions; -using FluentAssertions.Events; using Microsoft.Extensions.Logging; +using SharpHoundCommonLib.Enums; +using SharpHoundCommonLib.OutputTypes; namespace CommonLibTest { public class CertAbuseProcessorTest { - // private const string CASecurityFixture = - // "AQAUhCABAAAwAQAAFAAAAEQAAAACADAAAgAAAALAFAD//wAAAQEAAAAAAAEAAAAAAsAUAP//AAABAQAAAAAABQcAAAACANwABwAAAAADGAABAAAAAQIAAAAAAAUgAAAAIAIAAAADGAACAAAAAQIAAAAAAAUgAAAAIAIAAAADJAABAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQAAIAAAADJAACAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQAAIAAAADJAABAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQBwIAAAADJAACAAAAAQUAAAAAAAUVAAAAIE+Qun9GhKV2SBaQBwIAAAADFAAAAgAAAQEAAAAAAAULAAAAAQIAAAAAAAUgAAAAIAIAAAECAAAAAAAFIAAAACACAAA="; + private readonly Mock _mockRegistryAccessor; + private readonly CertAbuseProcessor _certAbuseProcessor; + + private const string Target = "target.test.local"; + private const string CAName = "TEST-CA"; + private const string DomainName = "TEST.LOCAL"; + private const string HostSid = "S-1-5-21-TEST-ENTERPRISE-CA"; + private const string FailureReason = "Registry Lookup Failure"; + + private CSVComputerStatus _receivedCompStatus; + + public CertAbuseProcessorTest() { + _mockRegistryAccessor = new Mock(); + _certAbuseProcessor = new CertAbuseProcessor(new MockLdapUtils(), _mockRegistryAccessor.Object); + + _certAbuseProcessor.ComputerStatusEvent += status => + { + _receivedCompStatus = status; + return Task.CompletedTask; + }; + } [Theory] [InlineData(0x00040000, true)] [InlineData(0x00020000, false)] public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_ChecksKey(int editFlags, bool expectedResult) { - const string target = "target machine name"; - const string caName = "TEST-CA"; - const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; const string subValue = "EditFlags"; - const string hostSid = "S-1-5-21-ENTERPRISE-CA"; - var mockRegistryAccessor = new Mock(); - mockRegistryAccessor + _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - target, + Target, subKey, subValue, It.IsAny())) .Returns(new RegistryResult { Collected = true, Value = editFlags }); - - var processor = new CertAbuseProcessor(new MockLdapUtils(), mockRegistryAccessor.Object); - - CSVComputerStatus capturedStatus = null; - processor.ComputerStatusEvent += status => - { - capturedStatus = status; - return Task.CompletedTask; - }; - var results = await processor.IsUserSpecifiesSanEnabled(target, caName, hostSid); + var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(Target, CAName, HostSid); //Validate result - Assert.Null(results.FailureReason); Assert.True(results.Collected); Assert.Equal(expectedResult, results.Value); + Assert.Null(results.FailureReason); //Validate CompStatus Log - Assert.Equal(caName, capturedStatus.ComputerName); - Assert.Equal(nameof(processor.IsUserSpecifiesSanEnabled), capturedStatus.Task); - Assert.Equal(CSVComputerStatus.StatusSuccess, capturedStatus.Status); - Assert.Equal(hostSid, capturedStatus.ObjectId); + Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.IsUserSpecifiesSanEnabled), _receivedCompStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + Assert.Equal(HostSid, _receivedCompStatus.ObjectId); } [Fact] public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_FailsLookup() { - const string target = "target machine name"; - const string caName = "TEST-CA"; - const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; const string subValue = "EditFlags"; - const string hostSid = "S-1-5-21-ENTERPRISE-CA"; + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + Target, + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); + + var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(Target, CAName, HostSid); + + //Validate result + Assert.False(results.Collected); + Assert.Equal(FailureReason, results.FailureReason); + + //Validate CompStatus Log + Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(_certAbuseProcessor.IsUserSpecifiesSanEnabled), _receivedCompStatus.Task); + Assert.Equal(FailureReason, _receivedCompStatus.Status); + Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + } + + [Theory] + [InlineData(1, true)] + [InlineData(0, false)] + public async Task CertAbuseProcessor_RoleSeparationEnabled_ChecksKey(int regValue, bool expectedResult) { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "RoleSeparationEnabled"; - const string failureReason = "Registry Lookup Failure"; + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + Target, + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = true, Value = regValue }); + + var results = await _certAbuseProcessor.RoleSeparationEnabled(Target, CAName, HostSid); + + //Validate result + Assert.True(results.Collected); + Assert.Equal(expectedResult, results.Value); + Assert.Null(results.FailureReason); + + //Validate CompStatus Log + Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.RoleSeparationEnabled), _receivedCompStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + } + + [Fact] + public async Task CertAbuseProcessor_RoleSeparationEnabled_FailsLookup() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "RoleSeparationEnabled"; - var mockRegistryAccessor = new Mock(); - mockRegistryAccessor + _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - target, + Target, subKey, subValue, It.IsAny())) - .Returns(new RegistryResult { Collected = false, FailureReason = failureReason }); + .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); + + var results = await _certAbuseProcessor.RoleSeparationEnabled(Target, CAName, HostSid); + + //Validate result + Assert.False(results.Collected); + Assert.Equal(FailureReason, results.FailureReason); + + //Validate CompStatus Log + Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(_certAbuseProcessor.RoleSeparationEnabled), _receivedCompStatus.Task); + Assert.Equal(FailureReason, _receivedCompStatus.Status); + Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + } + + //TODO: test happy path + // [Fact] + // public async Task CertAbuseProcessor_ProcessEAPermissions_ChecksKey() { + // const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + // const string subValue = "EnrollmentAgentRights"; + // + // _mockRegistryAccessor + // .Setup(ra => ra.GetRegistryKeyData( + // Target, + // subKey, + // subValue, + // It.IsAny())) + // .Returns(new RegistryResult { Collected = true, Value = "regValue" }); + // + // var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, Target, HostSid); + // + // //Validate result + // Assert.True(results.Collected); + // Assert.Equal(new EnrollmentAgentRestriction[0], results.Restrictions); + // Assert.Null(results.FailureReason); + // + // //Validate CompStatus Log + // Assert.Equal(CAName, _receivedCompStatus.ComputerName); + // Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); + // Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + // Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + // } + + [Fact] + public async Task CertAbuseProcessor_ProcessEAPermissions_FailsLookup() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "EnrollmentAgentRights"; - var processor = new CertAbuseProcessor(new MockLdapUtils(), mockRegistryAccessor.Object); + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + Target, + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); + + var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, Target, HostSid); + + //Validate result + Assert.False(results.Collected); + Assert.Equal(FailureReason, results.FailureReason); + + //Validate CompStatus Log + Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(_certAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); + Assert.Equal(FailureReason, _receivedCompStatus.Status); + Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + } + + //TODO: test happy path + // [Fact] + // public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ChecksKey() { + // const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + // const string subValue = "Security"; + // + // _mockRegistryAccessor + // .Setup(ra => ra.GetRegistryKeyData( + // Target, + // subKey, + // subValue, + // It.IsAny())) + // .Returns(new RegistryResult { Collected = true, Value = "regValue" }); + // + // var results = await _certAbuseProcessor.ProcessRegistryEnrollmentPermissions(CAName, DomainName, Target, HostSid); + // + // //Validate result + // Assert.True(results.Collected); + // Assert.Equal(new ACE[0], results.Data); + // Assert.Null(results.FailureReason); + // + // //Validate CompStatus Log + // Assert.Equal(CAName, _receivedCompStatus.ComputerName); + // Assert.Equal(nameof(CertAbuseProcessor.ProcessRegistryEnrollmentPermissions), _receivedCompStatus.Task); + // Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + // Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + // } + + [Fact] + public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_FailsLookup() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "Security"; - CSVComputerStatus capturedStatus = null; - processor.ComputerStatusEvent += status => - { - capturedStatus = status; - return Task.CompletedTask; - }; + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + Target, + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); - var results = await processor.IsUserSpecifiesSanEnabled(target, caName, hostSid); + var results = await _certAbuseProcessor.ProcessRegistryEnrollmentPermissions(CAName, DomainName, Target, HostSid); //Validate result - Assert.Equal(failureReason, results.FailureReason); Assert.False(results.Collected); + Assert.Equal(FailureReason, results.FailureReason); //Validate CompStatus Log - Assert.Equal(caName, capturedStatus.ComputerName); - Assert.Equal(nameof(processor.IsUserSpecifiesSanEnabled), capturedStatus.Task); - Assert.Equal(failureReason, capturedStatus.Status); - Assert.Equal(hostSid, capturedStatus.ObjectId); + Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(_certAbuseProcessor.ProcessRegistryEnrollmentPermissions), _receivedCompStatus.Task); + Assert.Equal(FailureReason, _receivedCompStatus.Status); + Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + } + + [Fact] + public async Task CertAbuseProcessor_ProcessCertTemplates_ReturnResolvedAndUnresolvedTemplates() { + const string validCN = "ValidCN"; + const string invalidCN = "InvalidCN"; + + var results = await _certAbuseProcessor.ProcessCertTemplates([validCN, invalidCN], DomainName); + + var expectedTemplate = new TypedPrincipal("guid", Label.CertTemplate); + Assert.Single(results.resolvedTemplates); + Assert.Contains(expectedTemplate, results.resolvedTemplates); + Assert.Single(results.unresolvedTemplates); + Assert.Contains(invalidCN, results.unresolvedTemplates); + } + + [WindowsOnlyTheory] + [InlineData("S-1-5-80")] + [InlineData("S-1-5-82")] + [InlineData("S-1-5-90")] + [InlineData("S-1-5-96")] + public async Task CertAbuseProcessor_GetRegistryPrincipal_ReturnsFalseForFilteredSID(string sidValue) { + var sid = new SecurityIdentifier(sidValue); + + var results = await _certAbuseProcessor.GetRegistryPrincipal(sid, DomainName, Target, true, "compId", sid); + + Assert.Equal((false, null), results); } + + //TODO finish testing GetRegistryPrincipal // [Fact] // public void CertAbuseProcessor_GetCASecurity_HappyPath() @@ -118,52 +297,6 @@ public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_FailsLookup() { // Assert.True(results.Collected); // } - // [Fact] - // public void CertAbuseProcessor_GetTrustedCerts_EmptyForNonRoot() - // { - // var mockUtils = new Mock(); - // mockUtils.Setup(x => x.IsForestRoot(It.IsAny())).Returns(false); - // var processor = new CertAbuseProcessor(mockUtils.Object); - // - // var results = processor.GetTrustedCerts("testlab.local"); - // Assert.Empty(results); - // } - // - // [Fact] - // public void CertAbuseProcessor_GetTrustedCerts_NullConfigPath_ReturnsEmpty() - // { - // var mockUtils = new Mock(); - // mockUtils.Setup(x => x.IsForestRoot(It.IsAny())).Returns(true); - // mockUtils.Setup(x => x.GetConfigurationPath(It.IsAny())).Returns((string)null); - // var processor = new CertAbuseProcessor(mockUtils.Object); - // - // var results = processor.GetTrustedCerts("testlab.local"); - // Assert.Empty(results); - // } - // - // [Fact] - // public void CertAbuseProcessor_GetRootCAs_EmptyForNonRoot() - // { - // var mockUtils = new Mock(); - // mockUtils.Setup(x => x.IsForestRoot(It.IsAny())).Returns(false); - // var processor = new CertAbuseProcessor(mockUtils.Object); - // - // var results = processor.GetRootCAs("testlab.local"); - // Assert.Empty(results); - // } - // - // [Fact] - // public void CertAbuseProcessor_GetRootCAs_NullConfigPath_ReturnsEmpty() - // { - // var mockUtils = new Mock(); - // mockUtils.Setup(x => x.IsForestRoot(It.IsAny())).Returns(true); - // mockUtils.Setup(x => x.GetConfigurationPath(It.IsAny())).Returns((string)null); - // var processor = new CertAbuseProcessor(mockUtils.Object); - // - // var results = processor.GetRootCAs("testlab.local"); - // Assert.Empty(results); - // } - // [Fact] // public async Task CertAbuseProcessor_ProcessCAPermissions_NullSecurity_ReturnsNull() // { @@ -188,71 +321,5 @@ public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_FailsLookup() { // Assert.Equal("Value cannot be null. (Parameter 'machineName')", capturedStatus.Status); // Assert.Equal(nameof(processor.ProcessRegistryEnrollmentPermissions), capturedStatus.Task); // } - - // [WindowsOnlyFact] - // public void CertAbuseProcessor_ProcessCAPermissions_ReturnsCorrectValues() - // { - // var mockUtils = new Mock(); - // var sd = new ActiveDirectorySecurityDescriptor(new ActiveDirectorySecurity()); - // mockUtils.Setup(x => x.MakeSecurityDescriptor()).Returns(sd); - // var processor = new CertAbuseProcessor(mockUtils.Object); - // var bytes = Helpers.B64ToBytes(CASecurityFixture); - // - // var results = processor.ProcessCAPermissions(bytes, "TESTLAB.LOCAL", "test", false); - // _testOutputHelper.WriteLine(JsonConvert.SerializeObject(results, Formatting.Indented)); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.Owns && x.PrincipalSID == "TESTLAB.LOCAL-S-1-5-32-544" && - // x.PrincipalType == Label.Group && !x.IsInherited); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.Enroll && x.PrincipalSID == "TESTLAB.LOCAL-S-1-5-11" && - // !x.IsInherited); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.ManageCA && x.PrincipalSID == "TESTLAB.LOCAL-S-1-5-32-544" && - // !x.IsInherited); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.ManageCertificates && x.PrincipalSID == "TESTLAB.LOCAL-S-1-5-32-544" && - // !x.IsInherited); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.ManageCA && - // x.PrincipalSID == "S-1-5-21-3130019616-2776909439-2417379446-512" && - // !x.IsInherited); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.ManageCertificates && - // x.PrincipalSID == "S-1-5-21-3130019616-2776909439-2417379446-512" && - // !x.IsInherited); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.ManageCA && - // x.PrincipalSID == "S-1-5-21-3130019616-2776909439-2417379446-519" && - // !x.IsInherited); - // Assert.Contains(results, - // x => x.RightName == EdgeNames.ManageCertificates && - // x.PrincipalSID == "S-1-5-21-3130019616-2776909439-2417379446-519" && - // !x.IsInherited); - // } - - // [Fact] - // public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_HandlesFailure() - // { - // var processor = new CertAbuseProcessor(new MockLdapUtils()); - // - // CSVComputerStatus capturedStatus = null; - // - // processor.ComputerStatusEvent += status => - // { - // capturedStatus = status; - // return Task.CompletedTask; - // }; - // - // var results = await processor.IsUserSpecifiesSanEnabled("target", "DUMPSTER.FIRE", "sid"); - // - // Assert.Equal("Target machine was not found or not connectable", results.FailureReason); - // Assert.False(results.Collected); - // Assert.False(results.Value); - // - // Assert.Equal("DUMPSTER.FIRE", capturedStatus.ComputerName); - // Assert.Equal("sid", capturedStatus.ObjectId); - // Assert.Equal("Target machine was not found or not connectable", capturedStatus.Status); - // Assert.Equal(nameof(processor.IsUserSpecifiesSanEnabled), capturedStatus.Task); - // } } } \ No newline at end of file diff --git a/test/unit/Facades/MockLdapUtils.cs b/test/unit/Facades/MockLdapUtils.cs index 8efd0eb0..be31ba44 100644 --- a/test/unit/Facades/MockLdapUtils.cs +++ b/test/unit/Facades/MockLdapUtils.cs @@ -745,9 +745,12 @@ public bool GetDomain(out Domain domain) { return (!results.IsNullOrEmpty(), results); } - public Task<(bool Success, TypedPrincipal Principal)> ResolveCertTemplateByProperty(string propValue, string propName, string domainName) { - throw new NotImplementedException(); - } + public async Task<(bool Success, TypedPrincipal Principal)> ResolveCertTemplateByProperty(string propValue, string propName, string domainName) => + propValue switch + { + "ValidCN" => (true, new TypedPrincipal("guid", Label.CertTemplate)), + _ => (false, null), + }; public async Task ConvertWellKnownPrincipal(string sid, string domain) { diff --git a/test/unit/Utils.cs b/test/unit/Utils.cs index 10e9d90a..fe525518 100644 --- a/test/unit/Utils.cs +++ b/test/unit/Utils.cs @@ -90,4 +90,12 @@ public WindowsOnlyFact() if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) Skip = "Ignore on non-Windows platforms"; } } + + public sealed class WindowsOnlyTheory : TheoryAttribute + { + public WindowsOnlyTheory() + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) Skip = "Ignore on non-Windows platforms"; + } + } } \ No newline at end of file From b20b2dd6f3046190381f5a5be8dad19b3b459fe6 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Wed, 10 Dec 2025 16:59:21 -0700 Subject: [PATCH 4/9] replace registry lookup functions in Helpers/IRegistryKey with RegistryAccessor --- src/CommonLib/Helpers.cs | 40 --------------- src/CommonLib/IRegistryKey.cs | 49 +++++-------------- .../Processors/CertAbuseProcessor.cs | 4 -- .../Processors/ComputerSessionProcessor.cs | 4 +- .../Processors/DCRegistryProcessor.cs | 10 ++-- 5 files changed, 23 insertions(+), 84 deletions(-) diff --git a/src/CommonLib/Helpers.cs b/src/CommonLib/Helpers.cs index 0033fa2a..839a0725 100644 --- a/src/CommonLib/Helpers.cs +++ b/src/CommonLib/Helpers.cs @@ -258,46 +258,6 @@ public static bool IsSidFiltered(string sid) { return false; } - //TODO: replace with IRegistryAccessor - public static RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue, ILogger log) { - var data = new RegistryResult(); - - try { - var baseKey = OpenRemoteRegistry(target); - var value = baseKey.GetValue(subkey, subvalue); - data.Value = value; - - data.Collected = true; - } - catch (IOException e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", - target, subkey, subvalue); - data.FailureReason = "Target machine was not found or not connectable"; - } - catch (SecurityException e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", - target, subkey, subvalue); - data.FailureReason = "User does not have the proper permissions to perform this operation"; - } - catch (UnauthorizedAccessException e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", - target, subkey, subvalue); - data.FailureReason = "User does not have the necessary registry rights"; - } - catch (Exception e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", - target, subkey, subvalue); - data.FailureReason = e.Message; - } - - return data; - } - - //TODO: replace with IRegistryAccessor - public static IRegistryKey OpenRemoteRegistry(string target) { - return SHRegistryKey.Connect(RegistryHive.LocalMachine, target).GetAwaiter().GetResult(); - } - public static string[] AuthenticationOIDs = new string[] { CommonOids.ClientAuthentication, CommonOids.PKINITClientAuthentication, diff --git a/src/CommonLib/IRegistryKey.cs b/src/CommonLib/IRegistryKey.cs index 43c3a0e9..fdf77f69 100644 --- a/src/CommonLib/IRegistryKey.cs +++ b/src/CommonLib/IRegistryKey.cs @@ -1,17 +1,15 @@ using System; -using System.Threading.Tasks; using Microsoft.Win32; namespace SharpHoundCommonLib { - public interface IRegistryKey { + public interface IRegistryKey: IDisposable { public object GetValue(string subkey, string name); public string[] GetSubKeyNames(); } - public class SHRegistryKey : IRegistryKey, IDisposable { + public class SHRegistryKey : IRegistryKey { private readonly RegistryKey _currentKey; - private static readonly AdaptiveTimeout _adaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromSeconds(10), Logging.LogProvider.CreateLogger(nameof(SHRegistryKey))); - + public SHRegistryKey(RegistryKey registryKey) { _currentKey = registryKey; } @@ -23,40 +21,19 @@ public object GetValue(string subkey, string name) { public string[] GetSubKeyNames() => _currentKey.GetSubKeyNames(); - /// - /// Gets a handle to a remote registry. - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - //TODO: replace with IRegistryAccessor - public static async Task Connect(RegistryHive hive, string machineName) { - var remoteKey = await _adaptiveTimeout.ExecuteWithTimeout((_) => RegistryKey.OpenRemoteBaseKey(hive, machineName)); - if (remoteKey.IsSuccess) - return new SHRegistryKey(remoteKey.Value); - throw new TimeoutException($"Failed to connect to registry on {machineName}: {remoteKey.Error}"); - } - public void Dispose() { _currentKey.Dispose(); } } - public class MockRegistryKey : IRegistryKey { - public virtual object GetValue(string subkey, string name) { - //Unimplemented - return default; - } - - public virtual string[] GetSubKeyNames() { - throw new NotImplementedException(); - } - } + // public class MockRegistryKey : IRegistryKey { + // public virtual object GetValue(string subkey, string name) { + // //Unimplemented + // return default; + // } + // + // public virtual string[] GetSubKeyNames() { + // throw new NotImplementedException(); + // } + // } } \ No newline at end of file diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index b723eac6..dc799ac6 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -254,7 +254,6 @@ private RegistryResult GetCASecurity(string target, string caName) const string regValue = "Security"; return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); - // return Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); } /// @@ -269,7 +268,6 @@ private RegistryResult GetEnrollmentAgentRights(string target, string caName) var regValue = "EnrollmentAgentRights"; return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); - // return Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); } /// @@ -287,7 +285,6 @@ public async Task IsUserSpecifiesSanEnabled(string target $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; const string regValue = "EditFlags"; var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); - // var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); ret.Collected = data.Collected; if (!data.Collected) @@ -336,7 +333,6 @@ public async Task RoleSeparationEnabled(string target, st var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; const string regValue = "RoleSeparationEnabled"; var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); - // var data = Helpers.GetRegistryKeyData(target, regSubKey, regValue, _log); ret.Collected = data.Collected; if (!data.Collected) diff --git a/src/CommonLib/Processors/ComputerSessionProcessor.cs b/src/CommonLib/Processors/ComputerSessionProcessor.cs index 729df182..52c2a8c1 100644 --- a/src/CommonLib/Processors/ComputerSessionProcessor.cs +++ b/src/CommonLib/Processors/ComputerSessionProcessor.cs @@ -24,6 +24,7 @@ public class ComputerSessionProcessor { private readonly string _localAdminPassword; private readonly AdaptiveTimeout _readUserSessionsAdaptiveTimeout; private readonly AdaptiveTimeout _readUserSessionsPriviledgedAdaptiveTimeout; + private readonly IRegistryAccessor _registryAccessor; public ComputerSessionProcessor(ILdapUtils utils, NativeMethods nativeMethods = null, ILogger log = null, string currentUserName = null, @@ -38,6 +39,7 @@ public ComputerSessionProcessor(ILdapUtils utils, _localAdminPassword = localAdminPassword; _readUserSessionsAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(ReadUserSessions))); _readUserSessionsPriviledgedAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(ReadUserSessionsPrivileged))); + _registryAccessor = new RegistryAccessor(); } public event ComputerStatusDelegate ComputerStatusEvent; @@ -293,7 +295,7 @@ public async Task ReadUserSessionsRegistry(string computerName _log.LogDebug("Running RegSessionEnum for {ObjectName}", computerName); try { - using (var key = await SHRegistryKey.Connect(RegistryHive.Users, computerName)) { + using (var key = await _registryAccessor.Connect(RegistryHive.Users, computerName)) { ret.Collected = true; await SendComputerStatus(new CSVComputerStatus { Status = CSVComputerStatus.StatusSuccess, diff --git a/src/CommonLib/Processors/DCRegistryProcessor.cs b/src/CommonLib/Processors/DCRegistryProcessor.cs index 4c229d02..febc08ec 100644 --- a/src/CommonLib/Processors/DCRegistryProcessor.cs +++ b/src/CommonLib/Processors/DCRegistryProcessor.cs @@ -9,12 +9,16 @@ namespace SharpHoundCommonLib.Processors public class DCRegistryProcessor { private readonly ILogger _log; + private readonly IRegistryAccessor _registryAccessor; + public readonly ILdapUtils _utils; public delegate Task ComputerStatusDelegate(CSVComputerStatus status); public DCRegistryProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; + //TODO: inject dependency and mock for Unit Tests + _registryAccessor = new RegistryAccessor(); _log = log ?? Logging.LogProvider.CreateLogger("DCRegProc"); } @@ -30,7 +34,7 @@ public IntRegistryAPIResult GetCertificateMappingMethods(string target) var ret = new IntRegistryAPIResult(); const string subKey = @"SYSTEM\CurrentControlSet\Control\SecurityProviders\Schannel"; const string subValue = "CertificateMappingMethods"; - var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue, _log); ret.Collected = data.Collected; if (!data.Collected) @@ -62,7 +66,7 @@ public IntRegistryAPIResult GetStrongCertificateBindingEnforcement(string target var ret = new IntRegistryAPIResult(); const string subKey = @"SYSTEM\CurrentControlSet\Services\Kdc"; const string subValue = "StrongCertificateBindingEnforcement"; - var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue, _log); ret.Collected = data.Collected; if (!data.Collected) @@ -94,7 +98,7 @@ public StrRegistryAPIResult GetVulnerableNetlogonSecurityDescriptor(string targe var ret = new StrRegistryAPIResult(); const string subKey = @"SYSTEM\CurrentControlSet\Services\Netlogon\Parameters"; const string subValue = "VulnerableChannelAllowList"; - var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue, _log); ret.Collected = data.Collected; if (!data.Collected) From 4d3636a08ccb18e0119b6b2439259b5f97936885 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Fri, 12 Dec 2025 08:08:04 -0700 Subject: [PATCH 5/9] test cleanup --- src/CommonLib/Helpers.cs | 4 - .../Processors/CertAbuseProcessor.cs | 41 +++---- test/unit/CertAbuseProcessorTest.cs | 113 +++++++++++------- 3 files changed, 93 insertions(+), 65 deletions(-) diff --git a/src/CommonLib/Helpers.cs b/src/CommonLib/Helpers.cs index 839a0725..19a51a37 100644 --- a/src/CommonLib/Helpers.cs +++ b/src/CommonLib/Helpers.cs @@ -1,17 +1,13 @@ using System; using System.Collections.Generic; using System.Globalization; -using System.IO; using System.Linq; -using System.Security; using System.Security.Principal; using System.Text; using System.Text.RegularExpressions; using SharpHoundCommonLib.Enums; using Microsoft.Extensions.Logging; using System.Threading.Tasks; -using Microsoft.Win32; -using SharpHoundCommonLib.Processors; namespace SharpHoundCommonLib { public static class Helpers { diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index dc799ac6..4b0baadb 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; using System.Security.AccessControl; using System.Security.Principal; using System.Text; @@ -8,7 +7,6 @@ using Microsoft.Extensions.Logging; using SharpHoundCommonLib.Enums; using SharpHoundCommonLib.OutputTypes; -using SharpHoundRPC; using SharpHoundRPC.Wrappers; using Encoder = Microsoft.Security.Application.Encoder; @@ -37,9 +35,10 @@ public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, /// This function should be called with the security data fetched from . /// The resulting ACEs will contain the owner of the CA as well as Management rights. /// - /// + /// /// /// + /// /// public async Task ProcessRegistryEnrollmentPermissions(string caName, string objectDomain, string computerName, string computerObjectId) { @@ -52,7 +51,7 @@ public async Task ProcessRegistryEnrollmentPermissions(str await SendComputerStatus(new CSVComputerStatus { Status = aceData.FailureReason, Task = nameof(ProcessRegistryEnrollmentPermissions), - ComputerName = caName, + ComputerName = computerName, ObjectId = computerObjectId, }); @@ -63,7 +62,7 @@ await SendComputerStatus(new CSVComputerStatus { await SendComputerStatus(new CSVComputerStatus { Status = CSVComputerStatus.StatusSuccess, Task = nameof(ProcessRegistryEnrollmentPermissions), - ComputerName = caName, + ComputerName = computerName, ObjectId = computerObjectId, }); @@ -186,7 +185,7 @@ public async Task ProcessEAPermissions(string await SendComputerStatus(new CSVComputerStatus { Status = regData.FailureReason, Task = nameof(ProcessEAPermissions), - ComputerName = caName, + ComputerName = computerName, ObjectId = computerObjectId, }); @@ -197,7 +196,7 @@ await SendComputerStatus(new CSVComputerStatus { await SendComputerStatus(new CSVComputerStatus { Status = CSVComputerStatus.StatusSuccess, Task = nameof(ProcessEAPermissions), - ComputerName = caName, + ComputerName = computerName, ObjectId = computerObjectId, }); @@ -277,8 +276,9 @@ private RegistryResult GetEnrollmentAgentRights(string target, string caName) /// https://blog.keyfactor.com/hidden-dangers-certificate-subject-alternative-names-sans /// /// + /// /// - public async Task IsUserSpecifiesSanEnabled(string target, string caName, string hostSid) + public async Task IsUserSpecifiesSanEnabled(string target, string caName, string computerObjectId) { var ret = new BoolRegistryAPIResult(); var regSubKey = @@ -292,8 +292,8 @@ public async Task IsUserSpecifiesSanEnabled(string target await SendComputerStatus(new CSVComputerStatus { Status = data.FailureReason, Task = nameof(IsUserSpecifiesSanEnabled), - ComputerName = caName, - ObjectId = hostSid + ComputerName = target, + ObjectId = computerObjectId }); ret.FailureReason = data.FailureReason; @@ -303,8 +303,8 @@ await SendComputerStatus(new CSVComputerStatus { await SendComputerStatus(new CSVComputerStatus { Status = CSVComputerStatus.StatusSuccess, Task = nameof(IsUserSpecifiesSanEnabled), - ComputerName = caName, - ObjectId = hostSid + ComputerName = target, + ObjectId = computerObjectId }); if (data.Value == null) @@ -319,15 +319,16 @@ await SendComputerStatus(new CSVComputerStatus { } /// - /// This function checks a registry setting on the target host for the specified CA to see if role seperation is enabled. + /// This function checks a registry setting on the target host for the specified CA to see if role separation is enabled. /// If enabled, you cannot perform any CA actions if you have both ManageCA and ManageCertificates permissions. Only CA admins can modify the setting. /// /// https://www.itprotoday.com/security/q-how-can-i-make-sure-given-windows-account-assigned-only-single-certification-authority-ca /// /// + /// /// /// - public async Task RoleSeparationEnabled(string target, string caName, string hostSid) + public async Task IsRoleSeparationEnabled(string target, string caName, string computerObjectId) { var ret = new BoolRegistryAPIResult(); var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; @@ -339,9 +340,9 @@ public async Task RoleSeparationEnabled(string target, st { await SendComputerStatus(new CSVComputerStatus { Status = data.FailureReason, - Task = nameof(RoleSeparationEnabled), - ComputerName = caName, - ObjectId = hostSid + Task = nameof(IsRoleSeparationEnabled), + ComputerName = target, + ObjectId = computerObjectId }); ret.FailureReason = data.FailureReason; @@ -350,9 +351,9 @@ await SendComputerStatus(new CSVComputerStatus { await SendComputerStatus(new CSVComputerStatus { Status = CSVComputerStatus.StatusSuccess, - Task = nameof(RoleSeparationEnabled), - ComputerName = caName, - ObjectId = hostSid + Task = nameof(IsRoleSeparationEnabled), + ComputerName = target, + ObjectId = computerObjectId }); if (data.Value == null) diff --git a/test/unit/CertAbuseProcessorTest.cs b/test/unit/CertAbuseProcessorTest.cs index 6768b74a..6c2ed7a8 100644 --- a/test/unit/CertAbuseProcessorTest.cs +++ b/test/unit/CertAbuseProcessorTest.cs @@ -13,18 +13,20 @@ namespace CommonLibTest { public class CertAbuseProcessorTest { + private readonly Mock _mockLdapUtils; private readonly Mock _mockRegistryAccessor; private readonly CertAbuseProcessor _certAbuseProcessor; - private const string Target = "target.test.local"; - private const string CAName = "TEST-CA"; private const string DomainName = "TEST.LOCAL"; - private const string HostSid = "S-1-5-21-TEST-ENTERPRISE-CA"; + private const string CAName = "TEST-CA"; + private const string TargetName = "target.test.local"; + private const string TargetDomainSid = "S-1-5-21-123456789-123456789-123456789"; private const string FailureReason = "Registry Lookup Failure"; private CSVComputerStatus _receivedCompStatus; public CertAbuseProcessorTest() { + _mockLdapUtils = new Mock(); _mockRegistryAccessor = new Mock(); _certAbuseProcessor = new CertAbuseProcessor(new MockLdapUtils(), _mockRegistryAccessor.Object); @@ -37,20 +39,20 @@ public CertAbuseProcessorTest() { [Theory] [InlineData(0x00040000, true)] - [InlineData(0x00020000, false)] - public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_ChecksKey(int editFlags, bool expectedResult) { + [InlineData(0x00000000, false)] + public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_ReturnsResult(int editFlags, bool expectedResult) { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; const string subValue = "EditFlags"; _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - Target, + TargetName, subKey, subValue, It.IsAny())) .Returns(new RegistryResult { Collected = true, Value = editFlags }); - var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(Target, CAName, HostSid); + var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(TargetName, CAName, TargetDomainSid); //Validate result Assert.True(results.Collected); @@ -58,54 +60,54 @@ public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_ChecksKey(int edi Assert.Null(results.FailureReason); //Validate CompStatus Log - Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(TargetName, _receivedCompStatus.ComputerName); Assert.Equal(nameof(CertAbuseProcessor.IsUserSpecifiesSanEnabled), _receivedCompStatus.Task); Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); - Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } [Fact] - public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_FailsLookup() { + public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_HandlesFailedLookup() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; const string subValue = "EditFlags"; _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - Target, + TargetName, subKey, subValue, It.IsAny())) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); - var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(Target, CAName, HostSid); + var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(TargetName, CAName, TargetDomainSid); //Validate result Assert.False(results.Collected); Assert.Equal(FailureReason, results.FailureReason); //Validate CompStatus Log - Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(TargetName, _receivedCompStatus.ComputerName); Assert.Equal(nameof(_certAbuseProcessor.IsUserSpecifiesSanEnabled), _receivedCompStatus.Task); Assert.Equal(FailureReason, _receivedCompStatus.Status); - Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } [Theory] [InlineData(1, true)] [InlineData(0, false)] - public async Task CertAbuseProcessor_RoleSeparationEnabled_ChecksKey(int regValue, bool expectedResult) { + public async Task CertAbuseProcessor_IsRoleSeparationEnabled_ReturnsResult(int roleSeparationEnabled, bool expectedResult) { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; const string subValue = "RoleSeparationEnabled"; _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - Target, + TargetName, subKey, subValue, It.IsAny())) - .Returns(new RegistryResult { Collected = true, Value = regValue }); + .Returns(new RegistryResult { Collected = true, Value = roleSeparationEnabled }); - var results = await _certAbuseProcessor.RoleSeparationEnabled(Target, CAName, HostSid); + var results = await _certAbuseProcessor.IsRoleSeparationEnabled(TargetName, CAName, TargetDomainSid); //Validate result Assert.True(results.Collected); @@ -113,41 +115,41 @@ public async Task CertAbuseProcessor_RoleSeparationEnabled_ChecksKey(int regValu Assert.Null(results.FailureReason); //Validate CompStatus Log - Assert.Equal(CAName, _receivedCompStatus.ComputerName); - Assert.Equal(nameof(CertAbuseProcessor.RoleSeparationEnabled), _receivedCompStatus.Task); + Assert.Equal(TargetName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.IsRoleSeparationEnabled), _receivedCompStatus.Task); Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); - Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } [Fact] - public async Task CertAbuseProcessor_RoleSeparationEnabled_FailsLookup() { + public async Task CertAbuseProcessor_IsRoleSeparationEnabled_HandlesFailedLookup() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; const string subValue = "RoleSeparationEnabled"; _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - Target, + TargetName, subKey, subValue, It.IsAny())) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); - var results = await _certAbuseProcessor.RoleSeparationEnabled(Target, CAName, HostSid); + var results = await _certAbuseProcessor.IsRoleSeparationEnabled(TargetName, CAName, TargetDomainSid); //Validate result Assert.False(results.Collected); Assert.Equal(FailureReason, results.FailureReason); //Validate CompStatus Log - Assert.Equal(CAName, _receivedCompStatus.ComputerName); - Assert.Equal(nameof(_certAbuseProcessor.RoleSeparationEnabled), _receivedCompStatus.Task); + Assert.Equal(TargetName, _receivedCompStatus.ComputerName); + Assert.Equal(nameof(_certAbuseProcessor.IsRoleSeparationEnabled), _receivedCompStatus.Task); Assert.Equal(FailureReason, _receivedCompStatus.Status); - Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } //TODO: test happy path // [Fact] - // public async Task CertAbuseProcessor_ProcessEAPermissions_ChecksKey() { + // public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsResult() { // const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; // const string subValue = "EnrollmentAgentRights"; // @@ -174,34 +176,34 @@ public async Task CertAbuseProcessor_RoleSeparationEnabled_FailsLookup() { // } [Fact] - public async Task CertAbuseProcessor_ProcessEAPermissions_FailsLookup() { + public async Task CertAbuseProcessor_ProcessEAPermissions_HandlesFailedLookup() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; const string subValue = "EnrollmentAgentRights"; _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - Target, + TargetName, subKey, subValue, It.IsAny())) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); - var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, Target, HostSid); + var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, TargetName, TargetDomainSid); //Validate result Assert.False(results.Collected); Assert.Equal(FailureReason, results.FailureReason); //Validate CompStatus Log - Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(TargetName, _receivedCompStatus.ComputerName); Assert.Equal(nameof(_certAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); Assert.Equal(FailureReason, _receivedCompStatus.Status); - Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } //TODO: test happy path // [Fact] - // public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ChecksKey() { + // public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ReturnsResult() { // const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; // const string subValue = "Security"; // @@ -228,33 +230,33 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_FailsLookup() { // } [Fact] - public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_FailsLookup() { + public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_HandlesFailedLookup() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; const string subValue = "Security"; _mockRegistryAccessor .Setup(ra => ra.GetRegistryKeyData( - Target, + TargetName, subKey, subValue, It.IsAny())) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); - var results = await _certAbuseProcessor.ProcessRegistryEnrollmentPermissions(CAName, DomainName, Target, HostSid); + var results = await _certAbuseProcessor.ProcessRegistryEnrollmentPermissions(CAName, DomainName, TargetName, TargetDomainSid); //Validate result Assert.False(results.Collected); Assert.Equal(FailureReason, results.FailureReason); //Validate CompStatus Log - Assert.Equal(CAName, _receivedCompStatus.ComputerName); + Assert.Equal(TargetName, _receivedCompStatus.ComputerName); Assert.Equal(nameof(_certAbuseProcessor.ProcessRegistryEnrollmentPermissions), _receivedCompStatus.Task); Assert.Equal(FailureReason, _receivedCompStatus.Status); - Assert.Equal(HostSid, _receivedCompStatus.ObjectId); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } [Fact] - public async Task CertAbuseProcessor_ProcessCertTemplates_ReturnResolvedAndUnresolvedTemplates() { + public async Task CertAbuseProcessor_ProcessCertTemplates_ReturnsResolvedAndUnresolvedTemplates() { const string validCN = "ValidCN"; const string invalidCN = "InvalidCN"; @@ -275,11 +277,40 @@ public async Task CertAbuseProcessor_ProcessCertTemplates_ReturnResolvedAndUnres public async Task CertAbuseProcessor_GetRegistryPrincipal_ReturnsFalseForFilteredSID(string sidValue) { var sid = new SecurityIdentifier(sidValue); - var results = await _certAbuseProcessor.GetRegistryPrincipal(sid, DomainName, Target, true, "compId", sid); + //TODO: check inputs + var results = await _certAbuseProcessor.GetRegistryPrincipal( + sid, + DomainName, + TargetName, + true, + TargetDomainSid, + new SecurityIdentifier(TargetDomainSid) + ); Assert.Equal((false, null), results); } + [WindowsOnlyFact] + public async Task CertAbuseProcessor_GetRegistryPrincipal_ReturnsTrueForDomainController() { + var expectedPrincipalType = Label.Group; + var expectedPrincipalSID = "S-1-5-21-3130019616-2776909439-2417379446-512"; + var domainSid = new SecurityIdentifier(expectedPrincipalSID); + + _mockLdapUtils.Setup(x => x.ResolveIDAndType(It.IsAny(), It.IsAny())) + .ReturnsAsync((true, new TypedPrincipal(expectedPrincipalSID, expectedPrincipalType))); + + var results = await _certAbuseProcessor.GetRegistryPrincipal( + domainSid, + DomainName, + TargetName, + true, + TargetDomainSid, + new SecurityIdentifier("S-1-5-21-3130019616-2776909439-2417379446-1104") + ); + + Assert.Equal((true, new TypedPrincipal(expectedPrincipalSID, expectedPrincipalType)), results); + } + //TODO finish testing GetRegistryPrincipal // [Fact] From 69da9fb2b3ae6cb9865ac23268c72aaaafc2edc7 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Mon, 15 Dec 2025 09:00:44 -0700 Subject: [PATCH 6/9] add tests for successful lookup in ProcessEAPermissions --- .../Processors/CertAbuseProcessor.cs | 4 + test/unit/CertAbuseProcessorTest.cs | 203 ++++++++++++++---- test/unit/Facades/MockLdapUtils.cs | 11 +- 3 files changed, 175 insertions(+), 43 deletions(-) diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index 4b0baadb..afc82e1b 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -460,6 +460,10 @@ await SendComputerStatus(new CSVComputerStatus computerObjectId, machineSid); var opaque = ace.GetOpaque(); + + if(opaque is null) + return (false, default); + var sidCount = BitConverter.ToUInt32(opaque, 0); index += 4; diff --git a/test/unit/CertAbuseProcessorTest.cs b/test/unit/CertAbuseProcessorTest.cs index 6c2ed7a8..d4fa4823 100644 --- a/test/unit/CertAbuseProcessorTest.cs +++ b/test/unit/CertAbuseProcessorTest.cs @@ -1,6 +1,6 @@ -using System.Security.Principal; +using System.Security.AccessControl; +using System.Security.Principal; using System.Threading.Tasks; -using CommonLibTest.Facades; using Moq; using SharpHoundCommonLib; using SharpHoundCommonLib.Processors; @@ -28,7 +28,7 @@ public class CertAbuseProcessorTest public CertAbuseProcessorTest() { _mockLdapUtils = new Mock(); _mockRegistryAccessor = new Mock(); - _certAbuseProcessor = new CertAbuseProcessor(new MockLdapUtils(), _mockRegistryAccessor.Object); + _certAbuseProcessor = new CertAbuseProcessor(_mockLdapUtils.Object, _mockRegistryAccessor.Object); _certAbuseProcessor.ComputerStatusEvent += status => { @@ -147,33 +147,157 @@ public async Task CertAbuseProcessor_IsRoleSeparationEnabled_HandlesFailedLookup Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } - //TODO: test happy path - // [Fact] - // public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsResult() { - // const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; - // const string subValue = "EnrollmentAgentRights"; - // - // _mockRegistryAccessor - // .Setup(ra => ra.GetRegistryKeyData( - // Target, - // subKey, - // subValue, - // It.IsAny())) - // .Returns(new RegistryResult { Collected = true, Value = "regValue" }); - // - // var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, Target, HostSid); - // - // //Validate result - // Assert.True(results.Collected); - // Assert.Equal(new EnrollmentAgentRestriction[0], results.Restrictions); - // Assert.Null(results.FailureReason); - // - // //Validate CompStatus Log - // Assert.Equal(CAName, _receivedCompStatus.ComputerName); - // Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); - // Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); - // Assert.Equal(HostSid, _receivedCompStatus.ObjectId); - // } + //TODO: mock SAM server, for sid lookups instead of fetching from localhost + [WindowsOnlyFact] + public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenDaclIsEmpty() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "EnrollmentAgentRights"; + + //setup binary security descriptor as registry value + var dacl = new RawAcl(2, 0); //dacl is empty + + var descriptor = new RawSecurityDescriptor( + ControlFlags.DiscretionaryAclPresent, + null, + null, + null, + dacl); + + byte[] regValue = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(regValue, 0); + + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + "localhost", + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = true, Value = regValue }); + + _mockLdapUtils.Setup(x => x.IsDomainController(TargetDomainSid, DomainName)) + .ReturnsAsync(true); + + var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); + + //Validate result + Assert.True(results.Collected); + Assert.Empty(results.Restrictions); + Assert.Null(results.FailureReason); + + //Validate CompStatus Log + Assert.Equal("localhost", _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); + } + + //TODO: mock SAM server, for sid lookups instead of fetching from localhost + [WindowsOnlyFact] + public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqueIsNull() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "EnrollmentAgentRights"; + + //setup binary security descriptor as registry value + var ace = new CommonAce( + AceFlags.None, + AceQualifier.AccessAllowed, + 0x00020089, + new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null), + false, + null //is callback false, null opaque + ); + + var dacl = new RawAcl(2, 1); + dacl.InsertAce(0, ace); + + var descriptor = new RawSecurityDescriptor( + ControlFlags.DiscretionaryAclPresent, + null, + null, + null, + dacl); + + byte[] regValue = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(regValue, 0); + + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + "localhost", + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = true, Value = regValue }); + + _mockLdapUtils.Setup(x => x.IsDomainController(TargetDomainSid, DomainName)) + .ReturnsAsync(true); + + var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); + + //Validate result + Assert.True(results.Collected); + Assert.Empty(results.Restrictions); + Assert.Null(results.FailureReason); + + //Validate CompStatus Log + Assert.Equal("localhost", _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); + } + + //TODO: mock SAM server, for sid lookups instead of fetching from localhost + [WindowsOnlyFact] + public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqueIsEmpty() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "EnrollmentAgentRights"; + + //setup binary security descriptor as registry value + var ace = new CommonAce( + AceFlags.None, + AceQualifier.AccessAllowed, + 0x00020089, + new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null), + true, + new byte[] { 0, 0, 0, 0 } //is callback true, empty opaque + ); + + var dacl = new RawAcl(2, 1); + dacl.InsertAce(0, ace); + + var descriptor = new RawSecurityDescriptor( + ControlFlags.DiscretionaryAclPresent, + null, + null, + null, + dacl); + + byte[] regValue = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(regValue, 0); + + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + "localhost", + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = true, Value = regValue }); + + _mockLdapUtils.Setup(x => x.IsDomainController(TargetDomainSid, DomainName)) + .ReturnsAsync(true); + + var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); + + //Validate result + Assert.True(results.Collected); + Assert.Empty(results.Restrictions); + Assert.Null(results.FailureReason); + + //Validate CompStatus Log + Assert.Equal("localhost", _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); + } [Fact] public async Task CertAbuseProcessor_ProcessEAPermissions_HandlesFailedLookup() { @@ -260,9 +384,16 @@ public async Task CertAbuseProcessor_ProcessCertTemplates_ReturnsResolvedAndUnre const string validCN = "ValidCN"; const string invalidCN = "InvalidCN"; + _mockLdapUtils + .Setup(x => x.ResolveCertTemplateByProperty(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync((string cn, string _, string _) => + cn == validCN + ? (true, new TypedPrincipal("test guid", Label.CertTemplate)) + : (false, null)); + var results = await _certAbuseProcessor.ProcessCertTemplates([validCN, invalidCN], DomainName); - var expectedTemplate = new TypedPrincipal("guid", Label.CertTemplate); + var expectedTemplate = new TypedPrincipal("test guid", Label.CertTemplate); Assert.Single(results.resolvedTemplates); Assert.Contains(expectedTemplate, results.resolvedTemplates); Assert.Single(results.unresolvedTemplates); @@ -284,28 +415,28 @@ public async Task CertAbuseProcessor_GetRegistryPrincipal_ReturnsFalseForFiltere TargetName, true, TargetDomainSid, - new SecurityIdentifier(TargetDomainSid) + new SecurityIdentifier("S-1-5-18") ); Assert.Equal((false, null), results); } [WindowsOnlyFact] - public async Task CertAbuseProcessor_GetRegistryPrincipal_ReturnsTrueForDomainController() { + public async Task CertAbuseProcessor_GetRegistryPrincipal_ResolvedDomainController_ReturnsTrue() { var expectedPrincipalType = Label.Group; var expectedPrincipalSID = "S-1-5-21-3130019616-2776909439-2417379446-512"; - var domainSid = new SecurityIdentifier(expectedPrincipalSID); + var sid = new SecurityIdentifier(expectedPrincipalSID); _mockLdapUtils.Setup(x => x.ResolveIDAndType(It.IsAny(), It.IsAny())) .ReturnsAsync((true, new TypedPrincipal(expectedPrincipalSID, expectedPrincipalType))); var results = await _certAbuseProcessor.GetRegistryPrincipal( - domainSid, + sid, DomainName, TargetName, true, TargetDomainSid, - new SecurityIdentifier("S-1-5-21-3130019616-2776909439-2417379446-1104") + new SecurityIdentifier("S-1-5-18") ); Assert.Equal((true, new TypedPrincipal(expectedPrincipalSID, expectedPrincipalType)), results); diff --git a/test/unit/Facades/MockLdapUtils.cs b/test/unit/Facades/MockLdapUtils.cs index be31ba44..1e5f1194 100644 --- a/test/unit/Facades/MockLdapUtils.cs +++ b/test/unit/Facades/MockLdapUtils.cs @@ -744,13 +744,10 @@ public bool GetDomain(out Domain domain) { return (!results.IsNullOrEmpty(), results); } - - public async Task<(bool Success, TypedPrincipal Principal)> ResolveCertTemplateByProperty(string propValue, string propName, string domainName) => - propValue switch - { - "ValidCN" => (true, new TypedPrincipal("guid", Label.CertTemplate)), - _ => (false, null), - }; + + public Task<(bool Success, TypedPrincipal Principal)> ResolveCertTemplateByProperty(string propValue, string propName, string domainName) { + throw new NotImplementedException(); + } public async Task ConvertWellKnownPrincipal(string sid, string domain) { From 1590a841579e3fda56738b3546efa05459d7a48d Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Mon, 15 Dec 2025 10:05:02 -0700 Subject: [PATCH 7/9] test successful lookup for ProcessRegistryEnrollmentPermissions --- .../Processors/CertAbuseProcessor.cs | 6 + test/unit/CertAbuseProcessorTest.cs | 128 ++++++++++++------ 2 files changed, 94 insertions(+), 40 deletions(-) diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index afc82e1b..7431206f 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -208,6 +208,12 @@ await SendComputerStatus(new CSVComputerStatus { var isDomainController = await _utils.IsDomainController(computerObjectId, objectDomain); var machineSid = await GetMachineSid(computerName, computerObjectId); var descriptor = new RawSecurityDescriptor(regData.Value as byte[], 0); + + if (descriptor.DiscretionaryAcl is null) + { + return ret; + } + var enrollmentAgentRestrictions = new List(); foreach (var genericAce in descriptor.DiscretionaryAcl) { diff --git a/test/unit/CertAbuseProcessorTest.cs b/test/unit/CertAbuseProcessorTest.cs index d4fa4823..9c183a52 100644 --- a/test/unit/CertAbuseProcessorTest.cs +++ b/test/unit/CertAbuseProcessorTest.cs @@ -1,4 +1,5 @@ -using System.Security.AccessControl; +using System; +using System.Security.AccessControl; using System.Security.Principal; using System.Threading.Tasks; using Moq; @@ -147,7 +148,46 @@ public async Task CertAbuseProcessor_IsRoleSeparationEnabled_HandlesFailedLookup Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } - //TODO: mock SAM server, for sid lookups instead of fetching from localhost + //TODO: mock SAM server for sid lookups instead of fetching from localhost + [WindowsOnlyFact] + public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenDaclIsNull() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "EnrollmentAgentRights"; + + //setup binary security descriptor as registry value + var descriptor = new RawSecurityDescriptor( + ControlFlags.None, + null, + null, + null, + null); + + var regValue = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(regValue, 0); + + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + "localhost", + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = true, Value = regValue }); + + var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); + + //Validate result + Assert.True(results.Collected); + Assert.Empty(results.Restrictions); + Assert.Null(results.FailureReason); + + //Validate CompStatus Log + Assert.Equal("localhost", _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); + } + + //TODO: mock SAM server for sid lookups instead of fetching from localhost [WindowsOnlyFact] public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenDaclIsEmpty() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; @@ -173,9 +213,6 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenDaclI subValue, It.IsAny())) .Returns(new RegistryResult { Collected = true, Value = regValue }); - - _mockLdapUtils.Setup(x => x.IsDomainController(TargetDomainSid, DomainName)) - .ReturnsAsync(true); var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); @@ -191,7 +228,7 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenDaclI Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } - //TODO: mock SAM server, for sid lookups instead of fetching from localhost + //TODO: mock SAM server for sid lookups instead of fetching from localhost [WindowsOnlyFact] public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqueIsNull() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; @@ -227,9 +264,6 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqu subValue, It.IsAny())) .Returns(new RegistryResult { Collected = true, Value = regValue }); - - _mockLdapUtils.Setup(x => x.IsDomainController(TargetDomainSid, DomainName)) - .ReturnsAsync(true); var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); @@ -245,7 +279,7 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqu Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } - //TODO: mock SAM server, for sid lookups instead of fetching from localhost + //TODO: mock SAM server for sid lookups instead of fetching from localhost [WindowsOnlyFact] public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqueIsEmpty() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; @@ -281,9 +315,6 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqu subValue, It.IsAny())) .Returns(new RegistryResult { Collected = true, Value = regValue }); - - _mockLdapUtils.Setup(x => x.IsDomainController(TargetDomainSid, DomainName)) - .ReturnsAsync(true); var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); @@ -325,33 +356,50 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_HandlesFailedLookup() Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } - //TODO: test happy path - // [Fact] - // public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ReturnsResult() { - // const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; - // const string subValue = "Security"; - // - // _mockRegistryAccessor - // .Setup(ra => ra.GetRegistryKeyData( - // Target, - // subKey, - // subValue, - // It.IsAny())) - // .Returns(new RegistryResult { Collected = true, Value = "regValue" }); - // - // var results = await _certAbuseProcessor.ProcessRegistryEnrollmentPermissions(CAName, DomainName, Target, HostSid); - // - // //Validate result - // Assert.True(results.Collected); - // Assert.Equal(new ACE[0], results.Data); - // Assert.Null(results.FailureReason); - // - // //Validate CompStatus Log - // Assert.Equal(CAName, _receivedCompStatus.ComputerName); - // Assert.Equal(nameof(CertAbuseProcessor.ProcessRegistryEnrollmentPermissions), _receivedCompStatus.Task); - // Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); - // Assert.Equal(HostSid, _receivedCompStatus.ObjectId); - // } + //TODO: mock SAM server for sid lookups instead of fetching from localhost + [WindowsOnlyFact] + public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ReturnsEmpty_WhenNoOwnerSidNoRules() { + const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; + const string subValue = "Security"; + + //setup binary security descriptor as registry value + var descriptor = new RawSecurityDescriptor( + ControlFlags.DiscretionaryAclPresent, + null, //owner is null + null, + null, + null); + + byte[] regValue = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(regValue, 0); + + _mockRegistryAccessor + .Setup(ra => ra.GetRegistryKeyData( + "localhost", + subKey, + subValue, + It.IsAny())) + .Returns(new RegistryResult { Collected = true, Value = regValue}); + + //get access rules returns empty + var mockSecurityDescriptor = new Mock(MockBehavior.Loose, null); + mockSecurityDescriptor.Setup(m => m.GetAccessRules(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns([]); + _mockLdapUtils.Setup(x => x.MakeSecurityDescriptor()).Returns(mockSecurityDescriptor.Object); + + var results = await _certAbuseProcessor.ProcessRegistryEnrollmentPermissions(CAName, DomainName, "localhost", TargetDomainSid); + + //Validate result + Assert.True(results.Collected); + Assert.Empty(results.Data); + Assert.Null(results.FailureReason); + + //Validate CompStatus Log + Assert.Equal("localhost", _receivedCompStatus.ComputerName); + Assert.Equal(nameof(CertAbuseProcessor.ProcessRegistryEnrollmentPermissions), _receivedCompStatus.Task); + Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); + Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); + } [Fact] public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_HandlesFailedLookup() { From 41b4f255ed52080c91087a040445d352f9513fd4 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Mon, 15 Dec 2025 10:23:39 -0700 Subject: [PATCH 8/9] consolidate ProcessEAPermissions Tests --- test/unit/CertAbuseProcessorTest.cs | 169 +++++----------------------- 1 file changed, 31 insertions(+), 138 deletions(-) diff --git a/test/unit/CertAbuseProcessorTest.cs b/test/unit/CertAbuseProcessorTest.cs index 9c183a52..247a2b63 100644 --- a/test/unit/CertAbuseProcessorTest.cs +++ b/test/unit/CertAbuseProcessorTest.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Security.AccessControl; using System.Security.Principal; using System.Threading.Tasks; @@ -147,165 +148,57 @@ public async Task CertAbuseProcessor_IsRoleSeparationEnabled_HandlesFailedLookup Assert.Equal(FailureReason, _receivedCompStatus.Status); Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } - - //TODO: mock SAM server for sid lookups instead of fetching from localhost - [WindowsOnlyFact] - public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenDaclIsNull() { - const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; - const string subValue = "EnrollmentAgentRights"; - - //setup binary security descriptor as registry value - var descriptor = new RawSecurityDescriptor( - ControlFlags.None, - null, - null, - null, - null); - - var regValue = new byte[descriptor.BinaryLength]; - descriptor.GetBinaryForm(regValue, 0); - - _mockRegistryAccessor - .Setup(ra => ra.GetRegistryKeyData( - "localhost", - subKey, - subValue, - It.IsAny())) - .Returns(new RegistryResult { Collected = true, Value = regValue }); - - var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); - - //Validate result - Assert.True(results.Collected); - Assert.Empty(results.Restrictions); - Assert.Null(results.FailureReason); - - //Validate CompStatus Log - Assert.Equal("localhost", _receivedCompStatus.ComputerName); - Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); - Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); - Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); - } - - //TODO: mock SAM server for sid lookups instead of fetching from localhost - [WindowsOnlyFact] - public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenDaclIsEmpty() { - const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; - const string subValue = "EnrollmentAgentRights"; - - //setup binary security descriptor as registry value - var dacl = new RawAcl(2, 0); //dacl is empty - - var descriptor = new RawSecurityDescriptor( - ControlFlags.DiscretionaryAclPresent, - null, - null, - null, - dacl); - - byte[] regValue = new byte[descriptor.BinaryLength]; - descriptor.GetBinaryForm(regValue, 0); - _mockRegistryAccessor - .Setup(ra => ra.GetRegistryKeyData( - "localhost", - subKey, - subValue, - It.IsAny())) - .Returns(new RegistryResult { Collected = true, Value = regValue }); - - var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); - - //Validate result - Assert.True(results.Collected); - Assert.Empty(results.Restrictions); - Assert.Null(results.FailureReason); - - //Validate CompStatus Log - Assert.Equal("localhost", _receivedCompStatus.ComputerName); - Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); - Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); - Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); - } - - //TODO: mock SAM server for sid lookups instead of fetching from localhost - [WindowsOnlyFact] - public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqueIsNull() { - const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; - const string subValue = "EnrollmentAgentRights"; - - //setup binary security descriptor as registry value - var ace = new CommonAce( + public static IEnumerable ProcessEAPermissionsTestData() { + var nullOpaqueAce = new CommonAce( AceFlags.None, AceQualifier.AccessAllowed, - 0x00020089, + 0x0000, new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null), false, - null //is callback false, null opaque + null ); - var dacl = new RawAcl(2, 1); - dacl.InsertAce(0, ace); + var daclWithNullOpaque = new RawAcl(2, 1); + daclWithNullOpaque.InsertAce(0, nullOpaqueAce); - var descriptor = new RawSecurityDescriptor( - ControlFlags.DiscretionaryAclPresent, - null, - null, - null, - dacl); - - byte[] regValue = new byte[descriptor.BinaryLength]; - descriptor.GetBinaryForm(regValue, 0); + var emptyOpaqueAce = new CommonAce( + AceFlags.None, + AceQualifier.AccessAllowed, + 0x0000, + new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null), + true, + new byte[] { 0, 0, 0, 0 } + ); - _mockRegistryAccessor - .Setup(ra => ra.GetRegistryKeyData( - "localhost", - subKey, - subValue, - It.IsAny())) - .Returns(new RegistryResult { Collected = true, Value = regValue }); - - var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); - - //Validate result - Assert.True(results.Collected); - Assert.Empty(results.Restrictions); - Assert.Null(results.FailureReason); - - //Validate CompStatus Log - Assert.Equal("localhost", _receivedCompStatus.ComputerName); - Assert.Equal(nameof(CertAbuseProcessor.ProcessEAPermissions), _receivedCompStatus.Task); - Assert.Equal(CSVComputerStatus.StatusSuccess, _receivedCompStatus.Status); - Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); + var daclWithEmptyOpaque = new RawAcl(2, 1); + daclWithEmptyOpaque.InsertAce(0, emptyOpaqueAce); + + return new List + { + new object[] { null }, //null dacl + new object[] { new RawAcl(2, 0) }, //empty dacl + new object[] { daclWithNullOpaque }, //dacl is callback false, null opaque + new object[] { daclWithEmptyOpaque }, //dacl is callback true, empty opaque + }; } //TODO: mock SAM server for sid lookups instead of fetching from localhost - [WindowsOnlyFact] - public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty_WhenOpaqueIsEmpty() { + [WindowsOnlyTheory] + [MemberData(nameof(ProcessEAPermissionsTestData))] + public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty(RawAcl dacl) { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; const string subValue = "EnrollmentAgentRights"; //setup binary security descriptor as registry value - var ace = new CommonAce( - AceFlags.None, - AceQualifier.AccessAllowed, - 0x00020089, - new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null), - true, - new byte[] { 0, 0, 0, 0 } //is callback true, empty opaque - ); - - var dacl = new RawAcl(2, 1); - dacl.InsertAce(0, ace); - var descriptor = new RawSecurityDescriptor( ControlFlags.DiscretionaryAclPresent, null, null, null, dacl); - - byte[] regValue = new byte[descriptor.BinaryLength]; + + var regValue = new byte[descriptor.BinaryLength]; descriptor.GetBinaryForm(regValue, 0); _mockRegistryAccessor @@ -358,7 +251,7 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_HandlesFailedLookup() //TODO: mock SAM server for sid lookups instead of fetching from localhost [WindowsOnlyFact] - public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ReturnsEmpty_WhenNoOwnerSidNoRules() { + public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ReturnsEmpty_WhenNoOwnerAndNoRules() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; const string subValue = "Security"; From 434738e1910359809efce0b8f3f65d9421e42223 Mon Sep 17 00:00:00 2001 From: Lucas Falslev Date: Mon, 15 Dec 2025 17:01:10 -0700 Subject: [PATCH 9/9] resolve pr comments --- src/CommonLib/IRegistryAccessor.cs | 30 ++++---- src/CommonLib/IRegistryKey.cs | 11 --- .../Processors/CertAbuseProcessor.cs | 8 +-- .../Processors/DCRegistryProcessor.cs | 10 ++- test/unit/CertAbuseProcessorTest.cs | 69 +++---------------- 5 files changed, 34 insertions(+), 94 deletions(-) diff --git a/src/CommonLib/IRegistryAccessor.cs b/src/CommonLib/IRegistryAccessor.cs index d6ceb5fb..0a80174f 100644 --- a/src/CommonLib/IRegistryAccessor.cs +++ b/src/CommonLib/IRegistryAccessor.cs @@ -8,41 +8,47 @@ namespace SharpHoundCommonLib { public interface IRegistryAccessor { - public RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue, ILogger log); + public RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue); public IRegistryKey OpenRemoteRegistry(string target); public Task Connect(RegistryHive hive, string machineName); } public class RegistryAccessor : IRegistryAccessor { - private static readonly AdaptiveTimeout _adaptiveTimeout = - new AdaptiveTimeout(maxTimeout: TimeSpan.FromSeconds(10), Logging.LogProvider.CreateLogger(nameof(SHRegistryKey))); + private readonly ILogger _log; + private readonly AdaptiveTimeout _adaptiveTimeout; + + public RegistryAccessor(ILogger log = null) { + _log = log ?? Logging.LogProvider.CreateLogger(nameof(RegistryAccessor)); + _adaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromSeconds(10), _log); + } - public RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue, ILogger log) { + public RegistryResult GetRegistryKeyData(string target, string subkey, string subvalue) { var data = new RegistryResult(); try { - var baseKey = OpenRemoteRegistry(target); - var value = baseKey.GetValue(subkey, subvalue); - data.Value = value; - data.Collected = true; + using (var baseKey = OpenRemoteRegistry(target)) { + var value = baseKey.GetValue(subkey, subvalue); + data.Value = value; + data.Collected = true; + } } catch (IOException e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + _log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", target, subkey, subvalue); data.FailureReason = "Target machine was not found or not connectable"; } catch (SecurityException e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + _log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", target, subkey, subvalue); data.FailureReason = "User does not have the proper permissions to perform this operation"; } catch (UnauthorizedAccessException e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + _log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", target, subkey, subvalue); data.FailureReason = "User does not have the necessary registry rights"; } catch (Exception e) { - log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", + _log.LogDebug(e, "Error getting data from registry for {Target}: {RegSubKey}:{RegValue}", target, subkey, subvalue); data.FailureReason = e.Message; } diff --git a/src/CommonLib/IRegistryKey.cs b/src/CommonLib/IRegistryKey.cs index fdf77f69..510efe32 100644 --- a/src/CommonLib/IRegistryKey.cs +++ b/src/CommonLib/IRegistryKey.cs @@ -25,15 +25,4 @@ public void Dispose() { _currentKey.Dispose(); } } - - // public class MockRegistryKey : IRegistryKey { - // public virtual object GetValue(string subkey, string name) { - // //Unimplemented - // return default; - // } - // - // public virtual string[] GetSubKeyNames() { - // throw new NotImplementedException(); - // } - // } } \ No newline at end of file diff --git a/src/CommonLib/Processors/CertAbuseProcessor.cs b/src/CommonLib/Processors/CertAbuseProcessor.cs index 7431206f..d9e4286e 100644 --- a/src/CommonLib/Processors/CertAbuseProcessor.cs +++ b/src/CommonLib/Processors/CertAbuseProcessor.cs @@ -258,7 +258,7 @@ private RegistryResult GetCASecurity(string target, string caName) var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; const string regValue = "Security"; - return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue); } /// @@ -272,7 +272,7 @@ private RegistryResult GetEnrollmentAgentRights(string target, string caName) var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; var regValue = "EnrollmentAgentRights"; - return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + return _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue); } /// @@ -290,7 +290,7 @@ public async Task IsUserSpecifiesSanEnabled(string target var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy"; const string regValue = "EditFlags"; - var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue); ret.Collected = data.Collected; if (!data.Collected) @@ -339,7 +339,7 @@ public async Task IsRoleSeparationEnabled(string target, var ret = new BoolRegistryAPIResult(); var regSubKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}"; const string regValue = "RoleSeparationEnabled"; - var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, regSubKey, regValue); ret.Collected = data.Collected; if (!data.Collected) diff --git a/src/CommonLib/Processors/DCRegistryProcessor.cs b/src/CommonLib/Processors/DCRegistryProcessor.cs index febc08ec..2726a45a 100644 --- a/src/CommonLib/Processors/DCRegistryProcessor.cs +++ b/src/CommonLib/Processors/DCRegistryProcessor.cs @@ -1,5 +1,4 @@ using SharpHoundCommonLib.OutputTypes; -using System; using System.Diagnostics.CodeAnalysis; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -17,8 +16,7 @@ public class DCRegistryProcessor public DCRegistryProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; - //TODO: inject dependency and mock for Unit Tests - _registryAccessor = new RegistryAccessor(); + _registryAccessor = new RegistryAccessor(log); _log = log ?? Logging.LogProvider.CreateLogger("DCRegProc"); } @@ -34,7 +32,7 @@ public IntRegistryAPIResult GetCertificateMappingMethods(string target) var ret = new IntRegistryAPIResult(); const string subKey = @"SYSTEM\CurrentControlSet\Control\SecurityProviders\Schannel"; const string subValue = "CertificateMappingMethods"; - var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue); ret.Collected = data.Collected; if (!data.Collected) @@ -66,7 +64,7 @@ public IntRegistryAPIResult GetStrongCertificateBindingEnforcement(string target var ret = new IntRegistryAPIResult(); const string subKey = @"SYSTEM\CurrentControlSet\Services\Kdc"; const string subValue = "StrongCertificateBindingEnforcement"; - var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue); ret.Collected = data.Collected; if (!data.Collected) @@ -98,7 +96,7 @@ public StrRegistryAPIResult GetVulnerableNetlogonSecurityDescriptor(string targe var ret = new StrRegistryAPIResult(); const string subKey = @"SYSTEM\CurrentControlSet\Services\Netlogon\Parameters"; const string subValue = "VulnerableChannelAllowList"; - var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue, _log); + var data = _registryAccessor.GetRegistryKeyData(target, subKey, subValue); ret.Collected = data.Collected; if (!data.Collected) diff --git a/test/unit/CertAbuseProcessorTest.cs b/test/unit/CertAbuseProcessorTest.cs index 247a2b63..663ece98 100644 --- a/test/unit/CertAbuseProcessorTest.cs +++ b/test/unit/CertAbuseProcessorTest.cs @@ -50,8 +50,7 @@ public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_ReturnsResult(int .Setup(ra => ra.GetRegistryKeyData( TargetName, subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = true, Value = editFlags }); var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(TargetName, CAName, TargetDomainSid); @@ -77,8 +76,7 @@ public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_HandlesFailedLook .Setup(ra => ra.GetRegistryKeyData( TargetName, subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); var results = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(TargetName, CAName, TargetDomainSid); @@ -105,8 +103,7 @@ public async Task CertAbuseProcessor_IsRoleSeparationEnabled_ReturnsResult(int r .Setup(ra => ra.GetRegistryKeyData( TargetName, subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = true, Value = roleSeparationEnabled }); var results = await _certAbuseProcessor.IsRoleSeparationEnabled(TargetName, CAName, TargetDomainSid); @@ -132,8 +129,7 @@ public async Task CertAbuseProcessor_IsRoleSeparationEnabled_HandlesFailedLookup .Setup(ra => ra.GetRegistryKeyData( TargetName, subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); var results = await _certAbuseProcessor.IsRoleSeparationEnabled(TargetName, CAName, TargetDomainSid); @@ -183,7 +179,6 @@ public static IEnumerable ProcessEAPermissionsTestData() { }; } - //TODO: mock SAM server for sid lookups instead of fetching from localhost [WindowsOnlyTheory] [MemberData(nameof(ProcessEAPermissionsTestData))] public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty(RawAcl dacl) { @@ -205,8 +200,7 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_ReturnsEmpty(RawAcl da .Setup(ra => ra.GetRegistryKeyData( "localhost", subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = true, Value = regValue }); var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, "localhost", TargetDomainSid); @@ -232,8 +226,7 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_HandlesFailedLookup() .Setup(ra => ra.GetRegistryKeyData( TargetName, subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); var results = await _certAbuseProcessor.ProcessEAPermissions(CAName, DomainName, TargetName, TargetDomainSid); @@ -249,7 +242,6 @@ public async Task CertAbuseProcessor_ProcessEAPermissions_HandlesFailedLookup() Assert.Equal(TargetDomainSid, _receivedCompStatus.ObjectId); } - //TODO: mock SAM server for sid lookups instead of fetching from localhost [WindowsOnlyFact] public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_ReturnsEmpty_WhenNoOwnerAndNoRules() { const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}"; @@ -270,8 +262,7 @@ public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_Return .Setup(ra => ra.GetRegistryKeyData( "localhost", subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = true, Value = regValue}); //get access rules returns empty @@ -303,8 +294,7 @@ public async Task CertAbuseProcessor_ProcessRegistryEnrollmentPermissions_Handle .Setup(ra => ra.GetRegistryKeyData( TargetName, subKey, - subValue, - It.IsAny())) + subValue)) .Returns(new RegistryResult { Collected = false, FailureReason = FailureReason }); var results = await _certAbuseProcessor.ProcessRegistryEnrollmentPermissions(CAName, DomainName, TargetName, TargetDomainSid); @@ -349,7 +339,6 @@ public async Task CertAbuseProcessor_ProcessCertTemplates_ReturnsResolvedAndUnre public async Task CertAbuseProcessor_GetRegistryPrincipal_ReturnsFalseForFilteredSID(string sidValue) { var sid = new SecurityIdentifier(sidValue); - //TODO: check inputs var results = await _certAbuseProcessor.GetRegistryPrincipal( sid, DomainName, @@ -382,47 +371,5 @@ public async Task CertAbuseProcessor_GetRegistryPrincipal_ResolvedDomainControll Assert.Equal((true, new TypedPrincipal(expectedPrincipalSID, expectedPrincipalType)), results); } - - //TODO finish testing GetRegistryPrincipal - - // [Fact] - // public void CertAbuseProcessor_GetCASecurity_HappyPath() - // { - // var mockProcessor = new Mock(new MockLDAPUtils(), null); - // - // var mockRegistryKey = new Mock(); - // mockRegistryKey.Setup(x => x.GetValue(It.IsAny(), It.IsAny())) - // .Returns(new byte[] { 0x20, 0x20 }); - // mockProcessor.Setup(x => x.OpenRemoteRegistry(It.IsAny())).Returns(mockRegistryKey.Object); - // - // var processor = mockProcessor.Object; - // var results = processor.GetCASecurity("testlab.local", "blah"); - // Assert.True(results.Collected); - // } - - // [Fact] - // public async Task CertAbuseProcessor_ProcessCAPermissions_NullSecurity_ReturnsNull() - // { - // var processor = new CertAbuseProcessor(new MockLdapUtils()); - // - // CSVComputerStatus capturedStatus = null; - // - // processor.ComputerStatusEvent += status => - // { - // capturedStatus = status; - // return Task.CompletedTask; - // }; - // - // var results = await processor.ProcessRegistryEnrollmentPermissions(null, "DUMPSTER.FIRE", null, "test"); - // - // Assert.Equal("Value cannot be null. (Parameter 'machineName')", results.FailureReason); - // Assert.False(results.Collected); - // Assert.Empty(results.Data); - // - // Assert.Equal(null, capturedStatus.ComputerName); - // Assert.Equal("test", capturedStatus.ObjectId); - // Assert.Equal("Value cannot be null. (Parameter 'machineName')", capturedStatus.Status); - // Assert.Equal(nameof(processor.ProcessRegistryEnrollmentPermissions), capturedStatus.Task); - // } } } \ No newline at end of file