WIP: NTLM Implementation #177
Conversation
|
|
||
| public async Task EnsureRequiresAuth(bool enableSigning, bool? useBadChannelBindings) | ||
| { | ||
| if (Url == null) |
There was a problem hiding this comment.
It looks like the Url property is only used on this method while being passed in to our other methods, might be worth moving this to a required argument rather than a property
|
|
||
| public class HttpTransport : INtlmTransport | ||
| { | ||
| private readonly ILogger _logger; |
There was a problem hiding this comment.
_logger doesn't seem to be used, might not be needed, or potentially was forgot to be used
src/CommonLib/Ntlm/HttpTransport.cs
Outdated
| throw new InvalidOperationException("No WWW-Authenticate header found in response"); | ||
| } | ||
|
|
||
| var authHeaders = response.Headers.WwwAuthenticate.Where(a => a.Scheme == _authScheme); |
There was a problem hiding this comment.
Slight performance nit: we can convert this to a list to avoid enumerating the elements again, probably not a large enough collection to make a large difference
| // { | ||
| // Marshal.FreeHGlobal(responseBerval.bv_val); | ||
| // } | ||
| // Marshal.FreeHGlobal(response); |
There was a problem hiding this comment.
I think that the memory is cleaned up at the end of the PInvoke call since, which could potentially be causing the crash
|
|
||
| // NEGOTIATE | ||
| var negotiateMsgBytes = context.Step(); | ||
| //_logger.LogDebug($"NTLM-NEGOTIATE message: {Convert.ToBase64String(negotiateMsgBytes)}"); |
There was a problem hiding this comment.
Not sure if intentionally left out, but _logger is no longer used if intentional
We also don't use _host or NtlmAuthPackageName
| if (evnt.Id != 4624) | ||
| throw new ArgumentException("Not a logon event"); | ||
|
|
||
| //var subjectUserSid = eventDetail.Properties[0].Value.ToString(); |
| public override string ToString() | ||
| { | ||
| var targetUser = AccountDomain + "\\" + AccountName; | ||
| var source = (SourceIp != null || SourcePort != null) ? $"{SourceIp}:{SourcePort}" : ""; |
| string packageName | ||
| ) | ||
| { | ||
| public string Host { get; set; } = host; // Hostname the sessions occurred on |
|
|
||
| public static NtlmSession FromLogonEvent(string host, EventRecord evnt) | ||
| { | ||
| if (evnt.Id != 4624) |
There was a problem hiding this comment.
Should these magic numbers be moved to an enum? Maybe a constant for this
| if (buffers[2].cbBuffer > 0) | ||
| { | ||
| Buffer.BlockCopy(padding, 0, wrapped, offset, (int)buffers[2].cbBuffer); | ||
| offset += (int)buffers[2].cbBuffer; |
There was a problem hiding this comment.
We can remove since we no longer use the offset after this if block, might have been kept for future changes, though?
| internal static partial class Helpers | ||
| { | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| public struct SEC_CHANNEL_BINDINGS |
There was a problem hiding this comment.
Pretty sure these are the names from the shared library, but we should follow our managed code's styling
|
|
||
| internal static class SSPI | ||
| { | ||
| private const Int32 SEC_I_CONTINUE_NEEDED = 0x00090312; |
| [Flags] | ||
| internal enum InitiatorContextRequestFlags : uint | ||
| { | ||
| ISC_REQ_DELEGATE = 0x00000001, |
Description
This is the initial implementation of the new NTLM modelling, courtesy of @leechristensen .
Motivation and Context
https://specterops.atlassian.net/browse/BED-5113
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: