0

So i've been trying to make secure PHP login / registration scripts and so far without any kind of password encryption I have this:

if($_POST)
{

function GenericError()
{
echo '<script type="text/javascript">window.location.href="error.php"</script>';
}

function CheckEmpty($param)
{
if($param == "" || $param == null)
    echo '<script type="text/javascript">window.location.href="empty.php"    </script>';
}

function AllYourBase()
{
mysql_connect("MyHost", "MyUsername", "MyPassword") or die(mysql_error());
mysql_select_db("MyDatabase") or die(mysql_error());
}

$username = CheckEmpty($_POST['username']);
$first = CheckEmpty($_POST['fname']);
$last = CheckEmpty($_POST['lname']);

if($_POST['password'] == $_POST['vpass'])
$password = $_POST['password'];
else
echo '<script type="text/javascript">window.location.href="pass.php"</script>';

if($_POST['email'] == $_POST['vemail'])
$email = $_POST['email'];
else
echo '<script type="text/javascript">window.location.href="email.php"</script>';

AllYourBase();
mysql_query("INSERT INTO Users (username, password, firstname, lastname, email) VALUES ('%s', '%s', '%s', '%s, '%s')",
        mysql_real_escape_string($username),
        mysql_real_escape_string($password),
        mysql_real_escape_string($first),
        mysql_real_escape_string($last),
        mysql_real_escape_string($email)) or die(GenericError());

echo '<script type="text/javascript">window.location.href="win.php"</script>';

}
</pre>

Does this seem right to you guys? Is there anything else I can do besides password encryption to make this more secure? Also, is there a better way to handle errors than making all these individual pages?

Shandhanu Mohan
  • 129
  • 7
Howdy_McGee
  • 10,422
  • 29
  • 111
  • 186
  • You don't need to encrypt passwords unless you need them back. Simple hashing (together with a random salt) will do. – Álvaro González Aug 01 '11 at 18:00
  • If I do hash / salt - is it possible to decrypt them so for say I can add a "forgot password" button? – Howdy_McGee Aug 01 '11 at 18:01
  • 2
    No, you cannot decrypt (at least easily) a hash. That is the point. There are other password reset workflows. – rcravens Aug 01 '11 at 18:03
  • 4
    You cannot. That's the whole point. The *forgot password* feature should not retrieve your old password: it should allow you to set a new one. – Álvaro González Aug 01 '11 at 18:04
  • Oh ok, I like that idea better - having the user reset it to what they want. thanks! – Howdy_McGee Aug 01 '11 at 18:06
  • Generally, don't let the user reset it directly. A forgotten password request should generate an email with a reset link (otherwise somebody could reset a users password just to annoy them). When the user clicks on the reset link, another email is generated with a random new random password you generate for them. If they want to user some custom password, they can then login with the random generated password and change their password on some account management page. – Endophage Aug 01 '11 at 18:14
  • @Endophage: sending them a random generated password only increases vectors of attack. A lot of people will keep it at that password, and keep the email, and if their email is hacked, the password is right there. A reset link is indeed OK (if they're hacked at that moment they are of course still in a world of hurt, but those links should expire, so only valid for a short period). But please let them enter a password themselves. – Wrikken Aug 01 '11 at 19:49
  • @Wrikken A truly random (or at least as random as a computer can get it) password is considerably more secure than most of the passwords people will choose for themselves. For a start, there are considerably more character (letter/number/symbol) combinations that don't represent valid text that than do so a random password is relatively impervious to a dictionary attack. Also if somebody has hacked your email account, it doesn't matter if the reset links expire, they can just request another one. A hacked email is a deeper problem that a site developer can't be expected to protect against. – Endophage Aug 01 '11 at 22:37
  • You have a point about people using weak passwords. Though, I would maintain that if they choose a weak password, it's _their_ fault, but if I sent a password through the mail, it's _my_ fault, especially since a lot of mail uses _insecure_ transport (although less and less, still a problem). We can of course not be expected to protect against hacked mail accounts, we can however make it somewhat less obviously abusable then the first simple search for 'password' in a mailbox. If you need strong passwords, just require them, and don't accept weak ones. – Wrikken Aug 01 '11 at 22:45

2 Answers2

1

I don't think this looks right, because I don't think you should ever store passwords in your database. Especially when you ask such questions on Stackoverflow(I don't even recommend myself to store passwords inside my database, although I did a lot of research on this topic, but I still don't consider myself a security expert). I always recommend people to use OpenID(or Facebook Connect) instead. It is very simple to implement, secure. Most users already have an OpenID like for example Google openID or Yahoo! openID. I have a demo available at my hosting provider(simple) at location http://westerveld.name/php-openid/. When you implement OpenID you don't need to worry about authentication at all. I have this code available at github. You could just simply clone code and get started => https://github.com/alfredwesterveld/php-openid

But If you really want to store passwords yourself I would advice you to look into phpass. It supports the most secure hashing method OpenBSD-style Blowfish-based bcrypt which is Moore's law proof. I made a simple library wrapping phpass also available at github, although I don't advice you to use this => https://github.com/alfredwesterveld/php-auth

Also I would advice you to look into PDO to do safe/fast cross-database SQL.

Community
  • 1
  • 1
Alfred
  • 60,935
  • 33
  • 147
  • 186
  • -1 for the "never store pwds in a db" (OpenID is a confusing failure), +1 for phpass (secure password generation), so it's a wash. – eykanal Aug 01 '11 at 20:16
  • lol eykanal you succeeded at stackoverflow, because stackoverflow requires openID. I don't understand that people find it difficult to login with openID. It is simple as hell. Please check out my demo at http://westerveld.name/php-openid/ and login with for example google or myopenID. It is so easy... – Alfred Aug 01 '11 at 20:24
  • OpenID is a failure from the point of view of the consumer, not the developer. It's no surprise that a developer-oriented site like SO uses it with little problem, but for consumers it [solves a problem they never had](http://www.quora.com/OpenID/What-s-wrong-with-OpenID), and confuses the hell out of them while doing so. That post describes the problems quite nicely. – eykanal Aug 01 '11 at 20:41
  • This is kind of off subject - I haven't provided any password hashing (which is what i'm going to do) and I said that a few times in my post. The point was to see if I was vulnerable else where such as MYSQL injection and such. Thanks for the input though. – Howdy_McGee Aug 01 '11 at 23:33
  • @Howdy tipped on how to hash correctly and how to prevent SQL-injections. I think I was pretty on topic. Also I gave you advice how to do it better, safer.. :$ – Alfred Aug 02 '11 at 06:24
0
  1. Don't use escaping in your queries. Use variable binding with PDO or similar (You'll also get good transaction support, db abstraction, etc. It's really a better way to go)
  2. It's really easy to store passwords unsafely, there are a lot of things to know. See this related post
Community
  • 1
  • 1
Josh
  • 6,155
  • 2
  • 22
  • 26
  • I wanted to comment about `mysql_query` as it's a common misconception - what you are doing above is akin to sprintf and then passing the string to mysql, it is not the same as data binding. – Josh Aug 01 '11 at 18:20