7

I have a VERY small site and recently I've been trying to make it more secure, I used to store my passwords in plain text.

I think Im doing it right, but as a "hobby" programmer I wanna make sure so I ask you, the professionals

When a user register I do: password_hash($their_password, PASSWORD_DEFAULT) and store that in the 'password' column in the users table. I use PASSWORD_DEFAULT as that seems the best according to php.net.

Note that this constant is designed to change over time as new and stronger algorithms are added to PHP."

Sounds good!

And the Login part (very simple):

if (count($_POST) > 0) {

$username = trim($_POST['username']);
$password = trim($_POST['password']);

$query = $db->prepare("SELECT password FROM users WHERE username = ?");
$query->execute(array($username));
$row = $query->fetch();

if (password_verify($password, $row['password'])) {
    echo "Correct password";
    // create session...
} else {
    // wrong password
}

Maybe I should check if the username exists first but other than that what do you think?

nhgrif
  • 61,578
  • 25
  • 134
  • 173
Smugglaren
  • 99
  • 1
  • 7
  • 1
    Looks fine.. Thats the way to use password_hash\verify. Important to know that the algorithm you are using called bcrypt which is recommended. I recommend you to use mysql column as BINARY(60) for saving memory, – Yair.R Mar 01 '15 at 13:24
  • 1
    Looks good to me. I'd personally place the query part in a different part of your code though (wrap it in a class perhaps). – Bono Mar 01 '15 at 13:24
  • I'm voting to close this question as off-topic because it's a good fit for http://codereview.stackexchange.com, not so much SO. – deceze Mar 01 '15 at 13:33
  • 3
    @deceze This will be closed on Code Review as off topic under this option: **Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example.** – bhathiya-perera Mar 01 '15 at 13:38
  • 2
    @deceze Regardless of whether this question is a better fit for another StackExchange site or not, the only reason to vote to close a question on [so] is if it is off-topic for [so] according to [so]'s own guidelines. If you think the question is a good fit for another site, the appropriate action is to recommend the user cross-post (this sometimes makes sense) or flag it for migration (unless that specific site is already in the vote-to-migrate options). Voting to close because you think it's a better fit somewhere else usually leads to the question being closed at two sites. – nhgrif Mar 01 '15 at 13:47
  • 1
    `PASSWORD_DEFAULT` is blowcrypt, with is a good choice, because of slow hashing and protection from rainbow tables. However in most practices passwords are hashed several times using different algorithms, to extend hashing speed and provide more 'tangled' hashes. – Ben Mar 01 '15 at 14:28

2 Answers2

2

You appear to have perfectly understood the documentation and how to construct the code you need. Shame on you for using plaintext password even temporarily, but your decision to fix with the correct method (ie. not md5 like me a silly person (I really need to update my password saving systems...)) is awesome.

The only issue I can see is that some people might have their passwords start or end with a space. Such passwords would lose their leading/trailing spaces and indeed the user may be alarmed that they can log in with two spaces, or none! So probably best to remove those trim calls ;)

Niet the Dark Absol
  • 320,036
  • 81
  • 464
  • 592
  • thank you. yes shame on me i mean damn its 2015 and you're right about the trim, you should allow your users to enter whatever they want as their password. i will remove those trims right now – Smugglaren Mar 01 '15 at 14:59
-3

Hello :) I'm a hobbyist too, and I think I can point you in the right direction, despite not knowing much about how the magic happens. 1) user enters his password, program encrypts using a certain method, and THAT encrypted password gets saved. 2) bam. NOBODY can see what the original was - not even the user who entered it. At login, that operation is repeated and "whatever the user entered for login" password gets encrypted by the same process and then it is compared with the also encrypted saved password. They should match if they were the same unencrypted original.

Well

For extra security, something called a "salt" is sometimes "added" to the encryption process, to make it yet harder to crack the password. Say somehow someone got hold of your encryption code AND encrypted passwords list, and tries to revert the process by reverse engineering your code? Well, now that person has the extra job of finding what your "salt" was... (it could be a string saved in your server, a clever "playing with dates of the month" trick, etc... many options). This is what I remember from what I read. Plenty of pointers to get you started. And more:

I use this: which I got from somewhere in the internet years ago

 function encryptTheString($password, $salt, $iter_count=4096, $keylen=64,         $hash_alg= 'sha256' ) 
 {
     // Compute the length of hash alg output.
     // Some folks use a static variable and save the value of the hash len.
     // Considering we are doing 1000s hmacs, doing one more won't hurt.
     $hashlen = strlen(hash($hash_alg, null, true));

     // compute number of blocks need to make $keylen number of bytes
     $numblocks = ceil($keylen / $hashlen);

     // blocks are appended to this
     $output = '';
     for ($i = 1; $i <= $numblocks; ++$i) {
         $block = hash_hmac($hash_alg, $salt . pack('N', $i), $password, true);
         $ib = $block;
         for ($j = 1; $j < $iter_count; ++$j) {
             $block = hash_hmac($hash_alg, $block, $password, true);
             $ib ^= $block;
         }
         $output .= $ib;
     }

     // extract the right number of output bytes
     return substr($output, 0, $keylen);
 }

And a call like

$ePassword=ANDYETpbkdf2($password,"111111111122222222223333333333444444444455555555566666666661234");

Would be perfectly ok :) give sha256 a reading for the start of further enlightenment.

nhgrif
  • 61,578
  • 25
  • 134
  • 173
Jorge
  • 21
  • 5
  • 2
    Salting is automatically done by `password_hash`. No need for it here. – Niet the Dark Absol Mar 01 '15 at 14:10
  • well thank you but i dont think salt is necessary after reading php.net as password_hash generates a salt for each hashed password so you dont have to worry about that. "It is strongly recommended that you do not generate your own salt for this function. It will create a secure salt automatically for you if you do not specify one." – Smugglaren Mar 01 '15 at 15:03