0

I have a class that takes user input (username/password), bcrypt hashes the input password to check if it matches the hash stored in the database, and then logs the user in if successful. The problem I'm experiencing is that if I call    cout << "\n"    or    sleep(1)    before hashing, the password check works as expected, but if I comment out sleep and cout, the hasher always fails, which results in the user getting an incorrect invalid credentials message.

I'm using pqxx to read the database, and rg3's bcrypt to hash / check the passwords.

Code snippet where I first found the problem:

// pqxx::result
string storedPass = result.begin()["passwordBCrypt_12"].as<string>();

// Uncommenting either cout or sleep causes checkPassword to work as expected
//cout << "\n"; // Confusingly, cout must contain "\n" to have the effect
//sleep(1);
if (!checkPassword(inputPass, storedPass))
    credError = true;


Code for checkPassword():

bool DB::checkPassword(string& password, string& passwordHash){
    char cpassword[password.length()];
    char hashInDatabase[BCRYPT_HASHSIZE];
    char outTestHash[BCRYPT_HASHSIZE];

    for (size_t i = 0; i < password.length(); i++){
        cpassword[i] = password[i];
    }
    for (size_t i = 0; i < BCRYPT_HASHSIZE; i++){
        hashInDatabase[i] = passwordHash[i];
    }

    if (bcrypt_hashpw(cpassword, hashInDatabase, outTestHash) == 0){
        if (strcmp(hashInDatabase, outTestHash) == 0) {
            // password matches
            return true;
        }
        // password does not match
    }
    return false;
}

String inputPass from the first code snippet is not handed down as a reference from other threads; it is copied.

aggregate1166877
  • 2,196
  • 23
  • 38
  • Sounds like a timing issues probably caused by a data race. If you are using threads, are you making correct use of mutex's? – const_ref Mar 21 '14 at 16:18
  • I am using threads (it's a web server), and to my knowledge, I'm using my mutexes correctly. In this specific example, the only mutex needed is for the pqxx connection, and it is locked at the beginning of the first snippet's method. – aggregate1166877 Mar 21 '14 at 16:21
  • have you checked the strings are not empty/are valid? – const_ref Mar 21 '14 at 16:25
  • I introduced a check now, and the string is not zero (it is 60 chars, which is the correct size of the hash). Also, my string check did not make `checkPassword()` evaluate correctly. – aggregate1166877 Mar 21 '14 at 16:33
  • The previous check was done before calling `checkPassword()`. Placing the check inside `checkPassword()` causes `checkPassword()` to run correctly. – aggregate1166877 Mar 21 '14 at 16:43

1 Answers1

1

As mentioned in the comments, you have a sync issue.

As a rule of thumb, whenever sleep or cout (or printf) solves your issue, then you didn't implement the mutex logic correctly , in some statement in your program.

You consider correct the checkPassword function. Are you 100% sure that the way stored pass doesn't need to have some sort of synchronization ?

PS : The confusingly "\n" effect has the habbit of flushing your buffer, leading to a synchronization .

MichaelCMS
  • 4,703
  • 2
  • 23
  • 29