Problem with encrypting passwords.

[garrette]

Now I found out that my passwords were very insecure, and that rainbow tables would be able to easily find out my information. . . so I decided to try and encrypt them.

function enc($string) {
        $salt = "@x2p";
        $hash = sha1(md5($salt.$string)).md5($string).sha1(md5(md5($string)));
        return $hash;
        }
        //Encrypts the passwords.
        $password = enc($password);
        $repeatpassword = enc($repeatpassword); 

Is what I use, but it doesn't work. I get the error code: Parse error: syntax error, unexpected T_STRING, expecting '(' on line 57. line 57 is function enc(@string) {

anyone know what I am doing wrong? I am very new to this, and most of this was made using various tutorials and examples, so I probably went wrong somewhere. And also is this actually a secure way to encode my passwords?

Okay, I fixed it. It was because this was placed within and "else" statement. Like so:

else

function enc($string) {
        $salt = "@x2p";
        $hash = sha1(md5($salt.$string)).md5($string).sha1(md5(md5($string)));
        return $hash;
        }
        //Encrypts the passwords.
        $password = enc($password);
        $repeatpassword = enc($repeatpassword);

//lots of other coding dealing with username, email, etc. etc. 

and I fixed it by simply putting brackets before and after this sequence

else {
my code here
}

My new question is that is this always the case? My code worked perfectly fine before, and only when I added this encryption it decided not to work. Why? And is this also a secure way to store my passwords?

[essellar]

Well, salting with a system salt is better than a straight md5 would be, but you're not gaining a lot by what you're doing.

In a security scheme, you should always assume right from the start that all of your secrets are out -- the bad guys have your database AND a glitch in the PHP runtime let them get the page as well, so they have your encryption scheme. All they have to do is find out what the plain text of the password is.*

The functions you are using to hash the password all suffer from the same problem: they're really efficient. They need to be -- their designed use is to come up with a digest string that represents the content of something else. If you've ever downloaded the source code for an open-source project, you'll see a hash used as a "checksum" -- even a one-byte difference in a large file will produce a very different digest value. For a function to be used for something like document verification, it needs to be able to process a large amount of data quickly, and all of the normal hash functions can do that. Hashes are also used as lookup values in data storage structures, and speed becomes vital to that application as well.

When used in a password verification system, speed is your enemy, not your friend. Since cracking salted-and-hashed passwords is a brute force job (you guess at passwords until you find one that works), you want to make it hard. Being tricksy with the way you glue together a very few, very efficient hash functions doesn't make it significantly harder -- it's not at all difficult to check millions of guesses per second. And with the same salt value used for all of the passwords, you don't even have to make a separate set of guesses for each user; you just try something, and if it turns up in any row in the users table, you've cracked that password.

When you use a nonce (a separate salt value for each user), then the bad guys have to work on each user row separately. So you've multiplied the amount of time it's going to take to crack all of your users' passwords by the number of users. That nonce can be something you create for only that purpose, or it could be something as simple as the username (which is already going to be in the table,and will be provided at login as well). But if the bad guys are only interested in targeting a single user (or a small number of users) whose passwords might be useful elsewhere (it's a fact that the vast majority of people only use one password, or perhaps a very small number of passwords, for all sites they log into), you're still stuck with the fact that making a guess and testing it doesn't take very much time.

We have to slow the process down by using something that's nowhere near as efficient as a standard hash function. If it takes an extra third of a second or so to check your user's password when they log in, it barely makes a difference to your site or to the user's experience. but if it takes an extra third of a second or so for each of the millions of guesses that the bad guys have to make to crack each password, that takes their time expense up into the days, weeks, months and even years territory (depending on what they're trying to accomplish) -- and that's what real security buys you. There's no such thing as "perfectly secure"; the best we can do is "really frickin' inconvenient for the bad guys".

There are a couple of well-accepted standard ways of slowing the process way down. One is to use a Blowfish hash (which requires files that may or may not be installed on the server, and over which you have no control in a shared hosting environment). It's probably the best current approach, but because it's not completely portable, it may be better to use the next best thing:

<?php
  // PBKDF2 Implementation ( described in RFC 2898 )
  function pbkdf2( $password, $salt, $iter_count=1000, $key_length=256, $algorithm = 'sha256', $start_pos=0 )
  {
    $key_blocks = $start_pos + $key_length;
    $derived_key = '';
    for ($block=1; $block<=$key_blocks; $block++)
    {
      $iterated_block = $current_hash = hash_hmac($algorithm, $salt . pack('N', $block), $password, true);
      for ($i=1; $i<$iter_count; $i++)
      {
        $iterated_block ^= ($current_hash = hash_hmac($algorithm, $current_hash, $password, true));
      }
      $derived_key .= $iterated_block;
    }
    return substr($derived_key, $start_pos, $key_length);
  }
?>

That little function is called PBKDF2 (Password-Based Key Derivation Function 2). It's slow and it's ugly and you don't need to know how it works if you don't want to. All of the arguments except the password and the salt are optional -- you can call it using just $hash = pbkdf2($user_pass, $salt);. If you want to slow the thing down a bit, you can set the $iter_count argument to a higher number (going lower than a thousand isn't recommended). Basically, it's going to call sha256 on the password + hash a thousand times, and do some value munging in between each call as well. Calling it once on the password your user gave you at login takes ages in computer time, but in human time it happens in the blink of a couple of eyes. Calling it a million times on different password guesses takes a very long time, even on the human scale. Calling it a million times on each of several hundreds or thousands of users whose passwords you want to crack is starting to get into serious calendar time -- if you not only let, but encourage, your users to use long pass phrases, it can take centuries with the current technology. It ain't perfect security, but I think it qualifies as "really frickin' inconvenient".

*Technically, they don't actually have to find the password, just something that hashes to the same value as the password does. Since hashes have a finite size that's smaller than the plain text can be (the only limit on the plain text is the string length limit in the language), it is possible to get hash collisions -- two or more different plain text values that hash to the same digest value.