Potential memory leak in SharpHound/ResolutionHelpers

[JohnLaTwC]

I think there is a problem with this coding pattern :

var netWkstaTask = Task.Run(() => NetWkstaGetInfo(hostname, 100, out wkstaData));
if (await Task.WhenAny(Task.Delay(5000), netWkstaTask) != netWkstaTask)
                return (false, new WorkstationInfo100());

If I understand what it is doing, it waits 5 secords or when the native API completes, whichever comes first. If the timeout came first, it exits early from the function. The question is what happened to the task making the native API call? and in particular the out param wkstaData. It would seem the timeout case always leaks the memory because nothing calls NetApiBufferFree on it. I am not sure there is a safe way to call these kind of APIs where the caller is expected to do the memory management.

        private static async Task<(bool success, WorkstationInfo100 info)> CallNetWkstaGetInfo(string hostname)
        {
            if (!Helpers.CheckPort(hostname, 445))
                return (false, new WorkstationInfo100());

            var wkstaData = IntPtr.Zero;
            var netWkstaTask = Task.Run(() => NetWkstaGetInfo(hostname, 100, out wkstaData));
            if (await Task.WhenAny(Task.Delay(5000), netWkstaTask) != netWkstaTask)
                return (false, new WorkstationInfo100());
! Does this leak wkstaData?

            if (netWkstaTask.Result != 0)
                return (false, new WorkstationInfo100());

            try
            {
                var wkstaInfo = Marshal.PtrToStructure<WorkstationInfo100>(wkstaData);
                return (true, wkstaInfo);
            }
            finally
            {
                if (wkstaData != IntPtr.Zero)
                    NetApiBufferFree(wkstaData);
            }
        }

SharpHound3/SharpHound3/ResolutionHelpers.cs

Line 542 in 1ba6ff2

return (false, new WorkstationInfo100());

Issue 2

Here is another case. If the timeout happens, it returns early. The finally block runs and it calls NetApiBufferFree(ptrInfo) but is there any guarantee that the NetSessionEnum task has completed when this finally block runs?

        private static async Task<List<Session>> GetNetSessions(Computer computer)
        {
            var resumeHandle = IntPtr.Zero;
            var sessionInfoType = typeof(SESSION_INFO_10);

            var entriesRead = 0;
            var ptrInfo = IntPtr.Zero;

            var sessionList = new List<Session>();

            try
            {
                var task = Task.Run(() => NetSessionEnum(computer.APIName, null, null, 10,
                    out ptrInfo, -1, out entriesRead, out _, ref resumeHandle));

                //10 second timeout
                if (await Task.WhenAny(task, Task.Delay(10000)) != task)
                {
                    if (Options.Instance.DumpComputerStatus)
                        OutputTasks.AddComputerStatus(new ComputerStatus
                        {
                            ComputerName = computer.DisplayName,
                            Status = "Timeout",
                            Task = "NetSessionEnum"
                        });
                    return sessionList;
! Early return triggers finally block
                }
...

                return sessionList;
            }
            finally
            {
                if (ptrInfo != IntPtr.Zero)
                    NetApiBufferFree(ptrInfo);
! Is it ensured that the NetSessionEnum task completes before this check? 
! Otherwise the task could complete later and nothing will free the memory.
            }
        }

SharpHound3/SharpHound3/Tasks/NetSessionTasks.cs

Line 59 in f0cfadf

if (await Task.WhenAny(task, Task.Delay(10000)) != task)

I am not an expert in these async tasks constructs when calling unmanaged code but I am not sure how this is safe.

[rvazarkar]

So this is a bit tricky. In our experience, if the API call doesn't finish in 5 seconds, its never going to finish at all. That's just a general thing though and not a hard fact by any means. We ran into scenarios where it would take upwards of a minute for the API call to fail which was massively slowing down enumeration.

I'm not sure if there's a clean way to actually do this with the constraints we have. I'm going to do a bit more research into this problem and see if its a known pattern, but for now I'm a bit stumped.

[rvazarkar]

After doing a bit more research, it appears that the task that was awaited will still run to completion even if we take the timeout. The program just wont wait for it to complete. The task should still wrap up gracefully in the background whenever the API call does return

[JohnLaTwC]

When I had filed this bug, I wrote a test program to see if the behavior repro'd and this is what I found:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Runtime.InteropServices;
using System.ComponentModel;

namespace TaskTest
{
    class Program
    {
        public static async Task Main(string[] args)
        {

            string hostname = Environment.GetEnvironmentVariable("COMPUTERNAME");

            if (args.Length == 1)
                hostname = args[0];


            //Console.WriteLine("looking up info for {0}", hostname);
            while (true) //for (uint i = 0; i < 10000; i++)
            {
                var wkstaData = IntPtr.Zero;
                var timeoutTask = Task.Delay(0);

                var netWkstaTask = Task.Run(() => NativeMethods.NetWkstaGetInfo(hostname, 100, out wkstaData));
                List<Task> tasks = new List<Task>() { timeoutTask, netWkstaTask };

                if (await Task.WhenAny(tasks) == timeoutTask)
                {
                    Console.WriteLine("*");
                }
                else
                {
                    try
                    {
                        var wkstaInfo = Marshal.PtrToStructure<NativeMethods.WorkstationInfo100>(wkstaData);
                    }
                    finally
                    {
                        if (wkstaData != IntPtr.Zero)
                        {
                            NativeMethods.NetApiBufferFree(wkstaData);
                            Console.WriteLine("F");
                        }
                    }
                }
            }
        }
    }

    public class NativeMethods
    {
        [DllImport("netapi32.dll", SetLastError = true)]
        public static extern int NetWkstaGetInfo(
            [MarshalAs(UnmanagedType.LPWStr)] string serverName,
            uint level,
            out IntPtr bufPtr);

        public struct WorkstationInfo100
        {

            public int platform_id;
            [MarshalAs(UnmanagedType.LPWStr)]
            public string computer_name;
            [MarshalAs(UnmanagedType.LPWStr)]
            public string lan_group;
            public int ver_major;
            public int ver_minor;
        }

        [DllImport("Netapi32.dll", SetLastError = true)]
        public static extern int NetApiBufferFree(IntPtr Buffer);
    }
}

I saw a steady increase in memory:

[rvazarkar]

Completely reworked this flow in vNext:

https://github.com/BloodHoundAD/SharpHoundCommon/blob/master/src/CommonLib/Processors/SAMRPCServer.cs

No more tasks, so everything should be disposed correctly