Php Security

[fesan]

Hi...

 

Have speant some time away from programming, so i need some updates!

I have written a small application for my company and an other company. Suddenly the security part smacked me in the head. This is the first time i actually must have a login to protect info. Nothing very important, but we dont want averyone lurking around being able to read everything.

 

Anyhow. When i first started to do some security on my own sites I did it like this:

<?php
session_start()
if (isset($_GET['logout'])) {
$_SESSION['login'] = false;
unset($_SESSION["user"]);
unset($_SESSION["status"]);
unset($_SESSION["name"]);
}
if($_SESSION['login'] == true && isset($_SESSION['user']) && isset($_SESSION['status'])){

//My Page with secret stuff

} else {
echo "Log in bastard";
}
?>

 

is this a good enugh way for keeping people out or is this old shit that is renewed?

There is offcource MD5 encryption on the passwords in the database, and the login script lookes like this:

 

<?php
session_start();
if($_GET['task'] == "login") {

$usr = !empty($_POST ['username']) ? $_POST['username'] : '';
$pass = !empty($_POST ['password']) ? $_POST['password'] : '';

$md5_pass = md5($pass);
include("admin/conn_us.php");
$query = "SELECT * FROM ${dbtable} WHERE brukernavn = '".mysql_real_escape_string($usr)."'";

if($result = mysql_query($query)) {
if(mysql_num_rows($result) <> 1) {
die("<p><span class='red'>Feil Brukernavn,".
"</p><p>Kontakt systemadministrator.</span></p>");
}


if($row = mysql_fetch_array($result)) {
if($row['passord'] == $md5_pass){
session_start();
$usr = $row['brukernavn'];
$name = $row['navn'];

$_SESSION["login"] = true;
$_SESSION["user"] = $usr;
$_SESSION["name"] = $name;

header("Location: http://bahrawy.net/dlight/ballroom/index.php");
exit;
} else {
echo "<span class='red'>Du har tastet feil passord</span>";

}}}} else {
echo '<p>
<form name="form1" method="post" action="login.php?task=login">
Brukernavn:<br>
<input type="text" name="username" id="username">
<br>
Passord:
<br>
<input type="password" name="password" id="password">
<br>
<input type="submit" name="submit" id="submit" value="Logg Inn">
</form>';

}
?>

 

Thanks for any inputt!

[KevinM1]

I'm going to go through this in ordered list fashion, so please bear with me.

 

1. MD5 is not encryption, but rather it's a hash algorithm. That may seem like a small nit to pick, but there's an important distinction between the two - something that's encrypted can be decrypted. A hash is a one-way transformation. Once something is hashed, you can't dehash it to get the original value.

 

MD5 itself is not a good hash to use for password security. It's insecure, and should really only be used to create a checksum. Passwords need to be protected with a nice, slow algorithm that can generate a lot of entropy. They also, at the very least, need to be created with a salt value.

 

The easiest way to do this is to use phpass. It's a library specifically made to protect passwords.

See: http://www.openwall.com/phpass/

Tutorial: http://www.openwall.com/articles/PHP-Users-Passwords

 

2. Don't use the old mysql_* functions. They're soft deprecated. Instead, use either MySQLi or PDO, and be sure to use their prepared statement functionality. Prepared statements escape your queries (among other things).

 

3. Be sure to validate incoming data. Example, if you expect an integer, check if it's actually an integer. Conversely, if you're not expecting to receive one, don't accept one.

 

That should cover the basics.

[gizmola]

Good advice from Kevin.

 

I'll one up him, and just advise you to use PDO with the mysql driver.

 

With session_start() you do it once at the logical top of your script.

 

You should not have code that does:

 

session_start().... some code if (true) session_start() as your current code does.

 

You really want to generalize your login check into a class or function so you can simply do:

 

if (!loggedIn()) {
  header('Location: login.php');
  exit;
} 

[fesan]

Thanks for the answers guys!

Just a couple of follow ups.

 

OK, so by upgrading my outdated code with PDO(), validating my income data and add some salt to the MD5 hash my site is secure with the code below:

 

if(is_int($_SESSION['login']) == 1 && isset($_SESSION['user']) && isset($_SESSION['name'])){

//secret stuff... HTML output of a table from mysql.

}

 

or should there be some other check and validation that the user is the user.

I would like to keep away from cookies and alike.

 

jup, the double session() was just a mistake of updating my code while developing... But thanks for the tip!

[berridgeab]

Theres further stuff you can do (regnerating session ids after a certain time interval, autologout after x time of inactivity, force logoutt on ip change etc) but it depends specifically on your security needs.

[KevinM1]

Thanks for the answers guys!

Just a couple of follow ups.

 

OK, so by upgrading my outdated code with PDO(), validating my income data and add some salt to the MD5 hash my site is secure with the code below:

 

if(is_int($_SESSION['login']) == 1 && isset($_SESSION['user']) && isset($_SESSION['name'])){

//secret stuff... HTML output of a table from mysql.

}

 

or should there be some other check and validation that the user is the user.

I would like to keep away from cookies and alike.

 

jup, the double session() was just a mistake of updating my code while developing... But thanks for the tip!

 

Again, don't use MD5. If you use phpass (which you really, really should do), you'll be using a different hashing algorithm altogether.

 

Staying away from cookies is pretty easy. Don't read from $_REQUEST or $_COOKIE.

 

For your sessions, you may want to look into session_regenerate_id if you have your users accessing different kinds of hidden info in different areas.

[fesan]

Forgot to ask, the note on generalizing my login check in to a function.

is it a good practice to make one php file with all my functions and include it is the index, so i can call the needed functions from wherever?

[berridgeab]

It makes it easier yes, once you get more comfortable with programming I suggest you look into classes and object orientated programming (OOP).

[TOA]

Generally, yes it's a perfectly acceptable practice.

 

IMO it depends how much overhead that would cause though. If most of those functions are only needed on say one page, then I would have a separate file for just those, and include any functions needed globally from your main include.

 

Hope that makes sense.

[fesan]

Well, this site is mostly the one page!

 

So what you intend to say is that we don't want to include larger files than we need to.?

Both for security reasons, server load and page load time?

[TOA]

Right, but this is a consideration for a larger system. As you say it's only a few pages, loading them all in one file would be acceptable.

[fesan]

Nice!

 

Thanks all for the help!!