Skip to content

Commit 464f19d

Browse files
mykeeliummykeelium
authored
BED-5558 - SharpHound warn on inability to collect necessary properties (#258)
* BED-5558 - Add Warning When Cannot Collect UAC Flags * chore: Add SID to Warn Log, move uac prop into condition * chore: Fix Tests * chore: fix Check for Resolve to Sid
1 parent bbf2f26 commit 464f19d

2 files changed

Lines changed: 57 additions & 63 deletions

File tree

‎src/CommonLib/Processors/LdapPropertyProcessor.cs‎

Lines changed: 50 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,11 @@ static LdapPropertyProcessor() {
3636
}
3737

3838
private readonly ILdapUtils _utils;
39+
private readonly ILogger _log;
3940

40-
public LdapPropertyProcessor(ILdapUtils utils) {
41+
public LdapPropertyProcessor(ILdapUtils utils, ILogger log = null) {
4142
_utils = utils;
43+
_log = log ?? Logging.LogProvider.CreateLogger(nameof(LdapPropertyProcessor));
4244
}
4345

4446
private static Dictionary<string, object> GetCommonProps(IDirectoryObject entry) {
@@ -233,55 +235,57 @@ public async Task<UserProperties> ReadUserProperties(IDirectoryObject entry, str
233235
var userProps = new UserProperties();
234236
var props = GetCommonProps(entry);
235237

236-
var uacFlags = (UacFlags)0;
237238
if (entry.TryGetLongProperty(LDAPProperties.UserAccountControl, out var uac)) {
238-
uacFlags = (UacFlags)uac;
239-
}
240-
241-
props.Add("sensitive", uacFlags.HasFlag(UacFlags.NotDelegated));
242-
props.Add("dontreqpreauth", uacFlags.HasFlag(UacFlags.DontReqPreauth));
243-
props.Add("passwordnotreqd", uacFlags.HasFlag(UacFlags.PasswordNotRequired));
244-
props.Add("unconstraineddelegation", uacFlags.HasFlag(UacFlags.TrustedForDelegation));
245-
props.Add("pwdneverexpires", uacFlags.HasFlag(UacFlags.DontExpirePassword));
246-
props.Add("enabled", !uacFlags.HasFlag(UacFlags.AccountDisable));
247-
props.Add("trustedtoauth", uacFlags.HasFlag(UacFlags.TrustedToAuthForDelegation));
248-
props.Add("smartcardrequired", uacFlags.HasFlag(UacFlags.SmartcardRequired));
249-
props.Add("encryptedtextpwdallowed", uacFlags.HasFlag(UacFlags.EncryptedTextPwdAllowed));
250-
props.Add("usedeskeyonly", uacFlags.HasFlag(UacFlags.UseDesKeyOnly));
251-
props.Add("logonscriptenabled", uacFlags.HasFlag(UacFlags.Script));
252-
props.Add("lockedout", uacFlags.HasFlag(UacFlags.Lockout));
253-
props.Add("passwordcantchange", uacFlags.HasFlag(UacFlags.PasswordCantChange));
254-
props.Add("passwordexpired", uacFlags.HasFlag(UacFlags.PasswordExpired));
255-
256-
userProps.UnconstrainedDelegation = uacFlags.HasFlag(UacFlags.TrustedForDelegation);
257-
258-
var comps = new List<TypedPrincipal>();
259-
if (uacFlags.HasFlag(UacFlags.TrustedToAuthForDelegation) &&
260-
entry.TryGetArrayProperty(LDAPProperties.AllowedToDelegateTo, out var delegates)) {
261-
props.Add("allowedtodelegate", delegates);
239+
var uacFlags = (UacFlags)uac;
240+
props.Add("sensitive", uacFlags.HasFlag(UacFlags.NotDelegated));
241+
props.Add("dontreqpreauth", uacFlags.HasFlag(UacFlags.DontReqPreauth));
242+
props.Add("passwordnotreqd", uacFlags.HasFlag(UacFlags.PasswordNotRequired));
243+
props.Add("unconstraineddelegation", uacFlags.HasFlag(UacFlags.TrustedForDelegation));
244+
props.Add("pwdneverexpires", uacFlags.HasFlag(UacFlags.DontExpirePassword));
245+
props.Add("enabled", !uacFlags.HasFlag(UacFlags.AccountDisable));
246+
props.Add("trustedtoauth", uacFlags.HasFlag(UacFlags.TrustedToAuthForDelegation));
247+
props.Add("smartcardrequired", uacFlags.HasFlag(UacFlags.SmartcardRequired));
248+
props.Add("encryptedtextpwdallowed", uacFlags.HasFlag(UacFlags.EncryptedTextPwdAllowed));
249+
props.Add("usedeskeyonly", uacFlags.HasFlag(UacFlags.UseDesKeyOnly));
250+
props.Add("logonscriptenabled", uacFlags.HasFlag(UacFlags.Script));
251+
props.Add("lockedout", uacFlags.HasFlag(UacFlags.Lockout));
252+
props.Add("passwordcantchange", uacFlags.HasFlag(UacFlags.PasswordCantChange));
253+
props.Add("passwordexpired", uacFlags.HasFlag(UacFlags.PasswordExpired));
254+
props.Add("useraccountcontrol", uac);
255+
256+
userProps.UnconstrainedDelegation = uacFlags.HasFlag(UacFlags.TrustedForDelegation);
257+
258+
var comps = new List<TypedPrincipal>();
259+
if (uacFlags.HasFlag(UacFlags.TrustedToAuthForDelegation) &&
260+
entry.TryGetArrayProperty(LDAPProperties.AllowedToDelegateTo, out var delegates)) {
261+
props.Add("allowedtodelegate", delegates);
262262

263-
foreach (var d in delegates) {
264-
if (d == null)
265-
continue;
263+
foreach (var d in delegates) {
264+
if (d == null)
265+
continue;
266266

267-
var resolvedHost = await _utils.ResolveHostToSid(d, domain);
268-
if (resolvedHost.Success && resolvedHost.SecurityIdentifier.Contains("S-1"))
269-
{
267+
var resolvedHost = await _utils.ResolveHostToSid(d, domain);
268+
if (!resolvedHost.Success || !resolvedHost.SecurityIdentifier.StartsWith("S-1-5-")) continue;
270269
await SendComputerStatus(new CSVComputerStatus {
271270
Status = CSVComputerStatus.StatusSuccess,
272271
Task = nameof(ReadUserProperties),
273272
ComputerName = Helpers.StripServicePrincipalName(d).ToUpper().TrimEnd('$'),
274273
ObjectId = resolvedHost.SecurityIdentifier,
275274
});
275+
276276
comps.Add(new TypedPrincipal {
277277
ObjectIdentifier = resolvedHost.SecurityIdentifier,
278278
ObjectType = Label.Computer
279279
});
280280
}
281281
}
282-
}
283282

284-
userProps.AllowedToDelegate = comps.Distinct().ToArray();
283+
userProps.AllowedToDelegate = comps.Distinct().ToArray();
284+
}
285+
else {
286+
entry.TryGetSecurityIdentifier(out var sid);
287+
_log.LogWarning("Unable to collect UserAccountControl flags for {SecurityIdentifier}.", sid);
288+
}
285289

286290
if (!entry.TryGetProperty(LDAPProperties.LastLogon, out var lastLogon)) {
287291
lastLogon = null;
@@ -313,7 +317,6 @@ await SendComputerStatus(new CSVComputerStatus {
313317
props.Add("unicodepassword", entry.GetProperty(LDAPProperties.UnicodePassword));
314318
props.Add("sfupassword", entry.GetProperty(LDAPProperties.MsSFU30Password));
315319
props.Add("logonscript", entry.GetProperty(LDAPProperties.ScriptPath));
316-
props.Add("useraccountcontrol", uac);
317320
props.Add("profilepath", entry.GetProperty(LDAPProperties.ProfilePath));
318321

319322
entry.TryGetLongProperty(LDAPProperties.AdminCount, out var ac);
@@ -381,19 +384,17 @@ public async Task<ComputerProperties> ReadComputerProperties(IDirectoryObject en
381384
continue;
382385

383386
var resolvedHost = await _utils.ResolveHostToSid(d, domain);
384-
if (resolvedHost.Success && resolvedHost.SecurityIdentifier.Contains("S-1"))
385-
{
386-
await SendComputerStatus(new CSVComputerStatus {
387-
Status = CSVComputerStatus.StatusSuccess,
388-
Task = nameof(ReadComputerProperties),
389-
ComputerName = d,
390-
ObjectId = resolvedHost.SecurityIdentifier,
391-
});
392-
comps.Add(new TypedPrincipal {
393-
ObjectIdentifier = resolvedHost.SecurityIdentifier,
394-
ObjectType = Label.Computer
395-
});
396-
}
387+
if (!resolvedHost.Success || !resolvedHost.SecurityIdentifier.StartsWith("S-1-5-")) continue;
388+
await SendComputerStatus(new CSVComputerStatus {
389+
Status = CSVComputerStatus.StatusSuccess,
390+
Task = nameof(ReadComputerProperties),
391+
ComputerName = d,
392+
ObjectId = resolvedHost.SecurityIdentifier,
393+
});
394+
comps.Add(new TypedPrincipal {
395+
ObjectIdentifier = resolvedHost.SecurityIdentifier,
396+
ObjectType = Label.Computer
397+
});
397398
}
398399
}
399400

@@ -1036,4 +1037,4 @@ public class IssuancePolicyProperties {
10361037
public Dictionary<string, object> Props { get; set; } = new();
10371038
public TypedPrincipal GroupLink { get; set; } = new TypedPrincipal();
10381039
}
1039-
}
1040+
}

‎test/unit/LdapPropertyTests.cs‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -418,20 +418,13 @@ public async Task LDAPPropertyProcessor_ReadUserProperties_TestBadPaths()
418418
Assert.Empty(props["sidhistory"] as string[]);
419419
Assert.Contains("admincount", keys);
420420
Assert.False((bool)props["admincount"]);
421-
Assert.Contains("sensitive", keys);
422-
Assert.Contains("dontreqpreauth", keys);
423-
Assert.Contains("passwordnotreqd", keys);
424-
Assert.Contains("unconstraineddelegation", keys);
425-
Assert.Contains("pwdneverexpires", keys);
426-
Assert.Contains("enabled", keys);
427-
Assert.Contains("trustedtoauth", keys);
428-
Assert.False((bool)props["trustedtoauth"]);
429-
Assert.False((bool)props["sensitive"]);
430-
Assert.False((bool)props["dontreqpreauth"]);
431-
Assert.False((bool)props["passwordnotreqd"]);
432-
Assert.False((bool)props["unconstraineddelegation"]);
433-
Assert.False((bool)props["pwdneverexpires"]);
434-
Assert.True((bool)props["enabled"]);
421+
Assert.DoesNotContain("sensitive", keys);
422+
Assert.DoesNotContain("dontreqpreauth", keys);
423+
Assert.DoesNotContain("passwordnotreqd", keys);
424+
Assert.DoesNotContain("unconstraineddelegation", keys);
425+
Assert.DoesNotContain("pwdneverexpires", keys);
426+
Assert.DoesNotContain("enabled", keys);
427+
Assert.DoesNotContain("trustedtoauth", keys);
435428
}
436429

437430
[WindowsOnlyFact]

0 commit comments

Comments
 (0)