kevoreilly / capemon

capemon: CAPE's monitor
GNU General Public License v3.0
102 stars 49 forks source link

Crashed in get_full_keyvalue_pathUS #54

Closed oalieno closed 1 year ago

oalieno commented 1 year ago

My sandbox is running at Windows 10. The crashed happened when NtQueryValueKey hooked function is trigger from PrivateRegQueryValueExT (I guess) in advapi32.dll.

https://github.com/kevoreilly/capemon/blob/64d21309ad498819a678e640324c71c5feeba93b/hook_reg_native.c#L195-L196

In loq function, it will handle the "k" : "FullName", KeyHandle, ValueName

https://github.com/kevoreilly/capemon/blob/64d21309ad498819a678e640324c71c5feeba93b/log.c#L711

Then the PUNICODE_STRING s, which is the ValueName, is passed into get_full_keyvalue_pathUS

https://github.com/kevoreilly/capemon/blob/64d21309ad498819a678e640324c71c5feeba93b/misc.c#L1195-L1201

Inside of get_full_keyvalue_pathUS. When accessing buf[i], it crashed because buf is null.

https://github.com/kevoreilly/capemon/blob/64d21309ad498819a678e640324c71c5feeba93b/misc.c#L1153-L1154

For unknown reason, the ValueName passed to NtQueryValueKey has Length > 0 and Buffer null. That's the root cause of the crash.


Fix is simple, just check in->Buffer before doing anything.

diff --git a/misc.c b/misc.c
index 5bab94a..2df90cb 100644
--- a/misc.c
+++ b/misc.c
@@ -1192,7 +1192,7 @@ wchar_t *get_full_keyvalue_pathW(HKEY registry, const wchar_t *in, PKEY_NAME_INF
 wchar_t *get_full_keyvalue_pathUS(HKEY registry, const PUNICODE_STRING in, PKEY_NAME_INFORMATION keybuf, unsigned int len)
 {
        wchar_t *ret;
-       if (in && in->Length) {
+       if (in && in->Buffer && in->Length) {
                unsigned int newlen = get_encoded_unicode_string_len(in->Buffer, in->Length);
                wchar_t *incpy = malloc(newlen + (1 * sizeof(wchar_t)));
                copy_encoded_unicode_string(incpy, in->Buffer, in->Length, newlen);
kevoreilly commented 1 year ago

Hi oalieno, thanks for this, I will push the fix today. I'll check there aren't similar issues in other similar hooks.

kevoreilly commented 1 year ago

I checked for any other parameters passed as PUNICODE_STRING and dereferenced - there are no others thankfully.