Adding safety checks, initialization and error handling

[cccs-mog]

CAPE/CAPE.c PVOID GetAllocationBase(PVOID Address) was found to fail silently since the erroroutput have been commented out. At VirtualQuery(Address, &MemInfo, sizeof(MEMORY_BASIC_INFORMATION), getting the last error can be really helpful to troubleshoot and the try catch mechanism could be a good catch point.

A missing TrackedRegion give error in the logging so the if statement have been added to prevent that from happening and appropriate logging have been added to make sure the lack of it is not silently passed.

In SIZE_T ScanForAccess(PVOID Buffer, SIZE_T Size), the char c is not initialized if the try catch fail and therefore the debugoutput could be erronous.

hook_misc.c The usage of swprintf_s is prefered over swprintf for the added security feature into it.

hook_process.c Error handling propagation to one more hook.

hook_reg.c Adding safety with wcscpy_s and strcpy_s as good practice.

hook_special.c Adding more CLSID for BITS and initializint the CLSID to empty value.

hooking.c Prevent teb empty value on NtCurrentTeb failure if possible.

[kevoreilly]

Wow thanks! ❤️

[kevoreilly]

I am interested in the wrapping of VirtualQuery and CreateProcess calls in try/except blocks - have you observed circumstances where exceptions are thrown by these APIs (and could you provide details)?

[cccs-mog]

Yes, so both of them are related to calls exceptions around https://github.com/kevoreilly/capemon/issues/63, even if the analysis doesn't crash per say, debugging was showing Access Denied error. I think however that regardless of those cases, error handling should be applied around such calls as it is good for debugging,could give more context for end users and it could increase the chance of recovery. Could work on standarizing such error handling accross a whole range of APIs.

[kevoreilly]

I think some caution is required here - firstly the issue you mentioned is confusing, I have yet to be convinced there is an issue in capemon as I have no problem detonating those samples without exeptions. More importantly, exceptions being raised that prevent a sample that would otherwise detonate are presumably bugs in the hooks that need fixing, not just wrapping. Exceptions that are raised irrespective of the hooks (i.e. due to the malware itself) should be 'captured' in terms of information but not caught.

[cccs-mog]

I agree that exceptions being raised that prevent a sample from detonating need fixing, which is why comments were added for easier identification. Wrapping is just so that such error in a hook is non fatal when it doesn't need to be and would encompass the whole hook call or the critical part not (only) the old_API. The way it is written is indeed not the best in cases where exceptions are raised due to the malware itself, but unless I am missing something this is by wrapping that it could be more descriptively captured other than by the function ret value. When I mentionned that issue #63, it was for context of why those wrapping were put on in the first place, not a fix per say, as like you mentionned there is no detonation issue. I can modify the pull request to reflect the idea of 'capture' and easier identification or I can remove it those two part altogether.

[kevoreilly]

Currently if there is an exception caused by the malware (or monitor) it is captured by the RtlDispatchException hook.

I would like to observe something that causes such an exception in GetAllocationBase() or AddTrackedRegion() and is handled by the proposed except block. Is there an example or sample you can show or share with me, or was it more of a theoretical idea?

[kevoreilly]

This is an example of a bug in capemon shown in the behavior log by the RtlDispatchException hook.

[cccs-mog]

Theorical idea for both, was following the functions logic and VirtualQuery in GetAllocationBase could fail and crash without wrapping but don't have any relevant sample to share for it. It seem that in GetAllocationBase case the sample bf57c2fa6f6c71786b18a43c2e0979c305d21a7dba7b206d6eca29dbe3c49799 happen to trigger the logging with WinError 87 there not the except portion however. Regarding AddTrackedRegion, assuming TrackingRegion could stay NULL, the debugoutput would fail if debug_comments are enabled and/or the later VirtualQuery would as well. Interesting, was not aware of this error handling being logged the way it is and handled. Probably explain the problem with other debuggers not able to follow the exception routine of RtlDispatchException for kernel-mode exception. What about user-mode ? I don't see KeUserExceptionDispatcher/KiUserExceptionDispatcher hooked outside WOW64 or is it handled by regular SEH from wrapping only ?

[kevoreilly]

I've removed the try/except bits in order to merge the other stuff - thanks very much for the contribution.

Just a note that the ErrorOutput() function (Output.c) calls GetLastError() internally so no need to call that explicitly I must have commented out the ErrorOutput call in GetAllocationBase() because it was too noisy but it should produce the desired output if uncommented.