4
\$\begingroup\$

I tried to implement something similar to the Linux shadow.txt file and I am not aware of concepts such as memory fingerprinting. Could someone please review this code?

#include <map>
#include <fstream>
#include <sstream>
#include <vector>
#include <string>
#include <iostream>
#include <openssl/md5.h>
using namespace std;

vector<string> split(string &s, char delim) {
    vector<string> elems;
    stringstream ss(s);
    string item;
    while(getline(ss,item,delim))
        elems.push_back(item);
    return elems;
}

void load_hashes(map<string, pair<string,string> > &active, string &fileName) {
    ifstream file(fileName);
    string str;
    while(getline(file,str)) {
        vector<string> elems = split(str,' ');
        active[elems[0]] = {elems[1],elems[2]};
    }
}

bool check_username_exists(map<string, pair<string,string> > &active, string username) {
    if(active.find(username)!=active.end())
        return true;
    return false;
}

char *md5_conversion(const char *str) {
    MD5_CTX c;
    MD5_Init(&c);
    MD5_Update(&c, str, 30);
    unsigned char digest[16];
    MD5_Final(digest, &c);
    char* md5string = (char*) malloc(sizeof(char)*33);
    for(int i = 0; i < 16; ++i)
        sprintf(&md5string[i*2], "%02x", (unsigned int)digest[i]);
    return md5string;
}

string password_and_salt_hash(const char* password, string& salt) {
    char password_plus_salt[50];
    int i = 0;
    while(password[i]!=0) {
        password_plus_salt[i] = password[i];
        i++;
    }
    for(int j=0;j<salt.length();j++,i++)
        password_plus_salt[i] = salt[j];
    password_plus_salt[i] = 0;
    const char* str = password_plus_salt;
    char* md5_value = md5_conversion(password_plus_salt);
    string return_value(md5_value);
    delete[] md5_value;
    return return_value;
}

/* Complete this function (Will use a third party library) */
string generate_random_salt() {
    return "asdfadfadcasdcaf";
}

bool new_entry(map<string, pair<string,string> > &active, string& username, const char* password) {
    if(check_username_exists(active,username)) { 
        cout << "The username already exists." << endl;
        return 0;  
    }
    string salt = generate_random_salt();
    string hash = password_and_salt_hash(password,salt);
    active[username] = {salt,hash};
    cout << "Username successfully added." << endl;
    return 1;
}

bool check_entry(map<string, pair<string,string> > &active, string& username, const char* password) {
    if(!check_username_exists(active,username)) { 
        cout << "Username does not exist." << endl;
        return 0;  
    }
    string salt = active[username].first;
    string hash = password_and_salt_hash(password,salt);
    string stored_hash = active[username].second;
    if(hash==stored_hash) {
        cout << "Correct Username and Password." << endl;
        return 1;
    }
    else cout << "Wrong Password." << endl;
    return 0;
}

bool change_entry(map<string, pair<string,string> > &active, string& username, const char* password) {
    if(!check_username_exists(active,username)) { 
        cout << "Username does not exist." << endl;
        return 0;  
    }
    string salt = generate_random_salt();
    string hash = password_and_salt_hash(password,salt);
    active[username] = {salt,hash};
    cout << "Password successfully changed." << endl;
    return 1;
}

/* This function is just for debugging purpose during development. */
void print_map(map<string, pair<string,string> > &active) {
    for(auto elem : active) 
        cout << elem.first << " " << elem.second.first << " " << elem.second.second << endl;
}

void store_hashes(map<string, pair<string,string> > &active,string &fileName) {
    ofstream file(fileName);
    for(auto elems: active) 
        file << elems.first << " " << elems.second.first << " " << elems.second.second << "\n";
}

void test() {
    map<string, pair<string,string> > active;
    string fileName = "secret.txt";
    load_hashes(active,fileName);
    print_map(active);
    string username = "shubham";
    // Limit password length to 20 characters
    const char* password = "darkknight";
    new_entry(active, username, password);
    print_map(active);
    check_entry(active,username, password);
    const char* wrong_password = "fastestmanalive";
    check_entry(active,username, wrong_password);
    const char* change_password = "darkknightrises";
    change_entry(active,username, change_password);
    print_map(active);
    store_hashes(active,fileName);  
} 

int main() {
    test();
    return 0;
}

Compiled using:

g++ password.cpp -std=c++11 -lcrypto
\$\endgroup\$
  • 1
    \$\begingroup\$ Why don't you compile with g++ -Wall -Wextra -Os? \$\endgroup\$ – Roland Illig Dec 2 '16 at 8:17
2
\$\begingroup\$
  • Half of your code is written in C, the other half in C++. There is no reason to use plain char[] when you can have the powerful std::string.
  • When you work with plain arrays (like for passwords), make sure you don't overflow them. Every array access must provably have a valid index.
  • Instead of computing md5(password + salt), feed the two parts independently into the MD5 computation. That way you don't have to concatenate them, so the buffer overflow vulnerability goes away.
  • Don't use using namespace std.
  • You must check array indexes when loading data from untrusted files (e.g. after the split).
  • Passing a map<string, pair<string, string>> around is bad for a human reader, since it is totally unclear what each of the string means.
  • The variable name active is usually of type boolean, not map. Yiu should find a better name for that.
  • Error messages typically go to std::cerr, not std::cout.
  • The function test should fail automatically instead of letting a human check its complete output.
  • When storing the hashes, first write to a temporary file. When all has gone well (check for errors!), rename the temporary file to its final destination. This prevents massive data loss in case the disk gets full.
  • Enable compiler warnings.
  • 0 is not a boolean value. At least not for humans. So when you return from a bool function, don't return 0, return false instead.
  • There is a requirement (limit password to 20 chars) that you haven't yet implemented. Nor should you, since passwords should not have an upper limit for the length. So delete that comment.
  • Your code leaks the plaintext password, since it is copied to password_plus_salt, but that array is not cleared after use. The point of using plain char arrays for passwords is exactly to be able to clear them after use. Whenever you clean an array after use, make sure that the generated machine code actually does clean. Many compilers optimize it away if it is improperly written. OpenSSL has a utility function to do this.
\$\endgroup\$
  • \$\begingroup\$ Thanks for the suggestions. About using string, I have found various answers on stackoverflow which suggest to use char array for storing passwords, instead of string. \$\endgroup\$ – Shubham Jain Dec 3 '16 at 4:02
  • \$\begingroup\$ Good point, I edited my answer. \$\endgroup\$ – Roland Illig Dec 3 '16 at 7:13
  • 1
    \$\begingroup\$ The use of char arrays for storing passwords is valid in case of languages like C# or Java which have immutable strings. C++ strings are mutable so you gain no advantage by using raw char*. The whole point is to deterministically remove the password from memory by overriding the characters as soon as the password is no longer of use. \$\endgroup\$ – D. Jurcau Dec 3 '16 at 7:20

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.