Dangerously hijack file handles to close them

[BinToss]

Status: Working in theory, but untested

Todo:

• refactor to NativeMethods class
• remove unused code
• add inline exception docs
• polish
• documentation

Mostly based on the following projects:

• ProcessHacker (https://github.com/processhacker/processhacker)
• Hijack File Handle by x86Mattew (https://www.x86matthew.com/view_post?id=hijack_file_handle)
• Handle Monitor by Adam Kramer (https://github.com/adamkramer/handle_monitor)

Documentation referenced:
https://www.geoffchappell.com/
https://docs.microsoft.com/en-us/windows/win32/
https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/_kernel/
https://docs.rs/ntapi/latest/ntapi/

I may add Microsoft/CsWin32 and/or dotnet/PInvoke for interop assistance and fewer definitions.

[BinToss]

It's been a while since I've shown any significant changes. This is mainly because most of this time was spent brainstorming, prototyping, restarting, etc.
My changes to NativeMethods.cs are nearly complete, but I'm going to refactor it again so I can commit changes in a review-friendly way.

I decided to commit my working tree to a temporary branch for your you to take a look at while I refactor the new methods, structs, enums, extern function, and classes to sub-classes for better organization and PR-friendly commits.

https://github.com/BinToss/deadlock-dotnet-sdk/blob/d37d7ad2de1e46aed339764f361b8f7eec840aa6/deadlock-dotnet-sdk/Domain/NativeMethods.cs

[BinToss]

NativeMethods backend is mostly done. Still need some xml documentation and thorough testing.
Next up: LockedHandle.cs and methods in the DeadLock.cs

[CodeDead]

Looking good so far @BinToss !

[BinToss]

Jus tabout everything has been tentavtively implemented.
Next up is the set of methods to be implemented in class deadlock-dotnet-sdk.DeadLock.

I'm unsure if I should modify the existing methods–add an argument to switch to file handle logic–or implement separate methods for handles.

[CodeDead]

@BinToss Separate methods would be the way to go. Since this is quite dangerous, it would be beneficial to add warnings for the developers as well as to use it only when absolutely necessary.

[BinToss]

Good point.
I've been concerned about that and a few other potential issues. Because this P/Invokes sensitive, undocumented kernel functions, an unsigned binary may be blocked or quarantined by security software. If a developer does not use the handle-unlocking features, member-level IL trimming should remove any potentially offensive code. Older SDKs that support assembly-level trimming, but not member-level trimming may include the offensive code even if it is unused.
If this poses a problem, then this pull request may need to be moved to a separate project.

Another potential problem to how to check if any of this actually works. I'd need to build a CLI project that can catch and print any and all exceptions. Luckily, I thought of exception reporting ahead of time—each SafeHandleEx object has a collection of exceptions.

Thirdly, some data queries or functions may fail if the thread or process has unsufficient privileges. One such failure will result in a handle's object type remaining unidentified. As such, the method highest in the native methods call chain can allow optionally filters out non-file and unidentified handles.

EDIT: I misremembered the Filter flag. The default filter for results is "only handles whose object are confirmed to be 'File'". The other flags enable including non-file handles and handles whose object type queries failed.

[CodeDead]

@BinToss
Being detected as malware is not a concern. Antimalware solutions will need to be clever enough to realize that sometimes, what the developer is trying to do should be permitted behavior. If they don't, the users of these antimalware solutions will eventually complain of certain software not working as intended and the problem will fix itself. I've been dealing with software getting false positives left and right for years and am willing to deal with it for much longer.

For the problem of requiring more privileges, that should be documented in the methods. If a developer still continues to use a function without the proper privileges, the consequences are theirs to carry. E.g. Throwing exceptions in such cases would probably be the safest option.

[BinToss]

Ready for a CLI project to test functionality and discover unhandled exceptions.
Except for Win32Exceptions thrown by (new SafeFileHandleEx()).UnlockSystemFile(), all exceptions should be handled gracefully and presented as a list of exceptions per handle. I had in mind a GDI+ list view when implementing SafeFileHandleEx. That grid-like presentation should transfer over to a CLI somewhat cleanly.

[BinToss]

Started the CLI project. Only had to spend two days troubleshooting a user error in VSCode.
I forgot I had OmniSharp select deadlock-dotnet-sdk.csproj for workspace analysis instead of the solution file. Until I remembered to select the solution file, Intellisense wasn't working properly in the CLI project.

It's a start. Even though it's an exception, I'm still relieved to see it got that far.

[BinToss]

I'm having trouble deciding how to inform users about something.
System.Diagnostics.Process.EnterDebugMode() grants us the SeDebugMode permission to access system and service processes. By default, the SeDebugMode permission can only be granted to a process if the process is elevated (run as Admin).
Currently, I'm informing users via an exception that can be suppressed with rethrowExceptions , but I'd like to treat this as a Warning rather than an Error.

[CodeDead]

@BinToss I think if the DebugMode elevation is absolutely required in order for something to work, it should be an error. If it's not required for the functionality to work properly, you could add something to the method docs for the developers to be aware of instead, but I'll leave it up to you as to what you think is best.

[BinToss]

Update.
Nearly done with a working product. Debug builds are slow. Very slow. It's untenable.
I'd recorded a performance trace with dotnet-trace and viewed it with speedscope.
>99% of CPU time is spent on unmanaged P/Invoke functions.
Additionally, the lack of output (namely, thrown exception counters) to the debugger's console make me think the process is in a "suspended" state—as if it hit a breakpoint that wasn't acknowledged by the debugger. It once ran for more than three hours. During that period, I had to use the debugger several times to break into and continue the process.

Major performance issues aside, simply achieving a buggy, but working dry run is nigh impossible.
Last night, I'd discovered and diagnosed a different issue.
When I expected only one SafeFileHandleEx added to FileLockerEx.Lockers given specified search parameters, I was receiving over 100. I inspected a few of the SafeFileHandleEx instances for their process ID and handle value and looked for the processes' handles in System Informer. It wasn't until hours later, when I was getting ready for bed, that I realized the cause.
I'd forgotten to duplicate handles when they aren't owned by the current process.
When you pass a handle value to a function that doesn't take a process ID or handle, that function will only return results for handles owned by the current process. DuplicateHandle allows the current process partial ownership over another process's handles.

[CodeDead]

Thanks for all the hard work @BinToss . I wish I could offer some more feedback but you're surfing in a lot of uncharted waters when it comes to those Windows API's.

Either way, all I can say is that it is much appreciated.

[BinToss]

I keep running into issues that can't be solved without loading a kernel-mode driver (i.e. the code-signed KProcessHacker.sys from Process Hacker 2 would work). NtQueryObject, GetFileInformationByHandleEx, and a many other functions will hang indefinitely when certain handles or handles types are passed to them. Other functions return error codes with no documented explanation for the given parameters. Additionally, our elevated process is frequently denied permission to duplicate other process's handles. Any and all handle-based queries require a duplicated handle when operating on handles not owned by the current process.

Loading a kernel-mode driver–even if it's signed–may be undesirable for API callers. But it's starting to seem like the only way to solve many of the issues I'm encountering.

edit: GetFileInformationByHandleEx shouldn't hang because it communicates with drivers, but I've observed it hanging anyway.