6
\$\begingroup\$

I have been reading about passwords and hashing algorithms and what not and decided to write a program.

Overview: The user should be prompted to create a password the first time the program is executed. They should enter a key and confirm it. If they have executed the program previously, then they should just enter the password to gain access.

I determine if the user has run the program by checking if key.txt exists. Is there a more preferred method?

I tried to streamline some code with the two bool functions. Any other suggestions for cleaner or more concise code?

#include <iostream>
#include <fstream>
#include <string>
#include "sha256.h"
using namespace std;

bool keyExists() {
    bool keyExists = false;
    ifstream inFile("key.txt");

    if (inFile) {
        keyExists = true;
    }

    return keyExists;
}

bool isMatch(string key, string confirmKey) {

    bool match = false;
    if (key == confirmKey) {
        match = true;
    }

    return match;
}

int main() {

    if (keyExists()) {
        string key;
        string storedKey;
        cout << "Please enter key: ";
        getline(cin, key);

        SHA256 sha256;
        ifstream inFile("key.txt");
        getline(inFile, storedKey);

        if (isMatch(sha256(key), storedKey)) {
            cout << "Acces Granted!\n";
        }
        else {
            cout << "Access Denied!\n";
        }
    }

    else {
        string key;
        string confirmKey;

        cout << "Please create a key: ";
        getline(cin, key);
        cout << "Confirm key: ";
        getline(cin, confirmKey);

        if (isMatch(key, confirmKey)) {

            SHA256 sha256;
            ofstream outFile("key.txt");
            outFile << sha256(key);
        }

        else {
            cout << "Keys do not match!\n";
        }
    }

    return 0;
}

Many thanks to Stephan Brumme for the awesome hashing algorithm code! This was very easy to implement.

\$\endgroup\$
  • 2
    \$\begingroup\$ I realize that this is just a test program to play with cryptography, hashing, and security. As a further exercise, could you think of ways this can be hacked and their mitigation? \$\endgroup\$ – TomG Mar 25 at 7:18
  • \$\begingroup\$ Well, I guess it could be brute forced. To mitigate that I should set a max number of attempts before lockout. There is also a possibility of a collision. However, from my understanding sha256 is fairly robust in this regard. Salting the hash could mitigate many factors. \$\endgroup\$ – okkv1747vm Mar 25 at 16:17
  • \$\begingroup\$ how about if I swap out the key.txt with I file of my own? \$\endgroup\$ – TomG Mar 25 at 18:17
  • \$\begingroup\$ @TomG Wow. Good point. I wouldn't know what to do in that scenario.. \$\endgroup\$ – okkv1747vm Mar 25 at 18:21
  • 1
    \$\begingroup\$ How about adding a increasing delay after each failed attempt. Say 0.25 seconds 0.5 seconds 1 second 2 seconds (double each time). This kind of delay will not affect a human user who knows the password but will severely affect a machine trying to brute force an answer. \$\endgroup\$ – Martin York Mar 26 at 15:24
6
\$\begingroup\$
  1. Using namespace std isn't a good practice in header files, where your program should at least partially reside, because you introduce potential name conflicts. So don't do that, and prefix the names you're importing with std::, you'll get used to it in no time.

  2. Your function keyExists can be improved upon. First, its name is not the best you could have found, because it suggests you're looking for the existence of a particular key, whereas what you really want to know if the program has been launched before. So is_first_use, or is_first_visit as an analogy to a website, would tell more. Besides, it doesn't have to be that verbose:

Original version

bool keyExists() {
    bool keyExists = false; // the homonymy is a bad idea
    ifstream inFile("key.txt");

    if (inFile) { 
        keyExists = true; // 
    }

    return keyExists;
}

first trimming

bool keyExists() {
    bool keyExists = false;
    ifstream inFile("key.txt");

    keyExists = inFile; // because streams can be converted to bool as you know

    return keyExists;
}

second trimming

bool keyExists() {
    ifstream inFile("key.txt");
    return inFile; // you specified the return type, so the conversion will occur
}

third trimming

bool keyExists() {
    return ifstream("key.txt"); // no need for a named object
}

So do you really need a function? It depends. If you want to let your program evolve from here, it's a good idea to keep the function, and implement a more viable test at a later point without disturbing the rest of your code. Otherwise it isn't worth the burden of finding a name for it.

  1. Your function isMatch suffers from the same issue.

original version

bool isMatch(string key, string confirmKey) {

    bool match = false;
    if (key == confirmKey) {
        match = true;
    }

    return match;
}

suggested implementation

bool isMatch(const string& key, const string& confirmKey) { // pass it by reference to avoid a potentially costly copy
    return key == confirmKey; // no need to store that in a `bool` before returning it
}

Then again I'm not sure it needs a function of its own...

  1. What's a valid key?

It might be a design choice, but it's very rare to allow for tabs, spaces and such inside a password. Is this really what you want? Is it compatible with every way to store passwords that you can think of? How would you constrain the choice of a password?

\$\endgroup\$
  • \$\begingroup\$ I'd say it's very rare to prohibit tabs, spaces or any other characters within passwords; did you mistype something there? \$\endgroup\$ – Toby Speight Mar 25 at 12:54
  • \$\begingroup\$ @TobySpeight: not at all, just a honest mistake \$\endgroup\$ – papagaga Mar 25 at 13:02
  • \$\begingroup\$ @papagaga Thanks you for your input! However, I get the following error when when trying to implement you suggestions: "No suitable conversion function from std::ifstream to bool." \$\endgroup\$ – okkv1747vm Mar 25 at 17:52
  • \$\begingroup\$ @okkv1747vm: the conversion to bool is standard since C++11... If your compiler / standard library version is older, you should consider an upgrade if possible \$\endgroup\$ – papagaga Mar 26 at 11:17
1
\$\begingroup\$

Another thing to keep in mind when doing security focused applications is reverse engineering.
Using that, a potential attacker can simply analyze your compiled program and then binary patch it to circumvent any security measures you have put in place. Of course in your case this is rather trivial because the source code is available. However even without the original source it can be possible to do so for example by looking for certain tell-tale strings.

Running your program gives some great hints about program flow via the strings Access Denied! and Access Granted! respectively. If one were to analyze your program it would be easy to find references to above strings to pinpoint the location in your code that needs to be patched.
Ultimately your program comes down to this part:

if (isMatch(sha256(key), storedKey)) {
    cout << "Acces Granted!\n";
}
else {
    cout << "Access Denied!\n";
}

Which when disassembled could look something like this:

access denied

Notice how you can see part of your if statement, namely the condition test and then a jump (JNZ) as well as the message Access Denied!. if you follow the jump you will get to this part:

access granted

Which just happens to be the "protected" part of your program. The attacker now only has to "patch" the jump to always jump to the right location of your program regardless of user input.

In order to protect against this you can read up on obfuscating programs and making them harder to "crack".

\$\endgroup\$
  • \$\begingroup\$ Using obfuscating is simply security via obscurity which is a know fallacy for security. It provides absolutely no protection and as such is simply bad advice. It will also make your application harder to read understand and maintain and thus more likely to have security issues in it. \$\endgroup\$ – Martin York Mar 26 at 15:16
0
\$\begingroup\$

1.

Do not use

using namespace std;  

Instead you can use

using std::cout;  
using std::getline();
using std::ifstream;
using std::ofstream;
using std::cin;
using std::string;  

Here is a link to why we should avoid using namespace <name>; type of statements

2.

In the function bool keyExists(); your bool variable is not necessary you can directly return the output of expression inside if condition. Same can be done in the other bool function, if you worry about readability then you may add a comment line after the return statement stating the intent behind the return value.

Also you can define both bool functions as inline.

\$\endgroup\$
  • 1
    \$\begingroup\$ You missed std::getline(), std::ifstream, std::ofstream, std::cin and std::string. \$\endgroup\$ – Toby Speight Mar 25 at 9:40
  • \$\begingroup\$ @TobySpeight I am sorry for that, I will edit my answer. I tried to answer using my phone and so there were some restrictions with format and the amount of content I could put in. \$\endgroup\$ – Mukul Kumar Mar 25 at 11:33
  • \$\begingroup\$ Don't use the using clause at all. std:: is not that hard to type. \$\endgroup\$ – Martin York Mar 26 at 15:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.