avoid race condition in the extension callback on windows beacons

[DominicBreuker]

Card

Beacons on Windows can crash if you run too many extensions at the same time.
I believe this is because of a race condition in WindowsExtenions.Call, which stores the function onFinish referred to in the callback on the struct.
The change is to use a closure and don't reference the struct.

Details

I've noticed the following behaviour: Write yourself an extension or install one from the armory. For example armory install sa-whoami. Get a Windows HTTP beacon and call sa-whoami slowly, only once per check in. Everything will be fine (well, the first time there is no output, but I don't yet know why). Now run sa-whoami lots of times so that they all execute on the next check in. There is a good chance your implant will die with exit code 2, probably a panic.

In the code I saw the following:

• beaconMain launches a Goroutine for each task, with an anonymous function for results processing: https://github.com/BishopFox/sliver/blob/master/implant/sliver/sliver.go#L412
• When you call an extension this handler should call Run on the extension, again with an anonymous callback function wrapping the one from above: https://github.com/BishopFox/sliver/blob/master/implant/sliver/handlers/handlers_windows.go#L718
• Eventually we get from Run to Call in the extension and it stores the callback on the extension struct: https://github.com/BishopFox/sliver/blob/master/implant/sliver/extension/extension_windows.go#L104
• The DLL then gets an extensionCallback passed to it which is related to the original callback only by referring to the callback on the struct.
• The actual call to the DLL is protected by a mutex here: https://github.com/BishopFox/sliver/blob/master/implant/sliver/extension/extension_windows.go#L122

Now my thinking is that because all calls to extensions are created in their own Goroutine they may overwrite the onFinish callback stored on the struct, which is stored before a lock is acquired. Moreover, in beacon mode it seems that each call to the extension also loads the extension. Not sure about it but I wonder if the entire extension struct may be garbage-collected by the time the DLL calls back.

The change is to decouple the DLL callback from the extension struct. This should ensure the reference to the callback survives until it is called.

In my experiments the beacon did not crash a single time anymore after this change no matter the number of extension calls. Still, you probably want to give it a try yourself before merging this!!! I only tried with sa-whoami and an extension I wrote myself, with HTTP baecon.

[rkervella]

Thanks @DominicBreuker, that's a good analysis, and indeed, it looks like we're overwriting the callback between each call. I'll test this PR when I get some time, but it looks good after a first look at the code.

[github-advanced-security]

You have successfully added a new CodeQL configuration .github/workflows/codeql-scanning.yml:CodeQL-Build. As part of the setup process, we have scanned this repository and found 20 existing alerts. Please check the repository Security tab to see all alerts.