Sound safe?

Craig321

Craig321

Craig321

Soldato
Joined
2 May 2004
Posts
19,950
Hi

I have been coding a login/registration system today. I don't want to the post the code here else someone might take my work that I have worked quite a while on, I'll try and explain what I have done :)

On the index.php for now you will be able to see a table with all the usernames and password in it, this is just temporary for when it's not being used properly.

When you haven't got a cookie stored (we'll come to them later) on the page, below the table, you'll see:

Code: 
You are not logged in.
Register
Login

register.php - a basic form, not security holes in that as it's just a form. This form submits to processreg.php.
At the top of processreg.php there is this:

Code: 
if ($username === htmlspecialchars($username)) {

And at the bottom is the else. This checks the username for HTML and goes to the else if there is HTML in the username, this will also be done for the password.

A bit below that I have:

Code: 
if (($username == "") || ($password == "") || ($password != $rpassword)) {

This goes to the else if the username or password is blank or if the password wasn't repeated properly.

Now that the checking is done, if everything is all ok, it posts the info to SQL.
At the end of that it says register successful for now.


login.php - another simple form which goes to authenticate.php.

authenticate.php:
Here I have an SQL query, below I have this:

Code: 
if(mysql_affected_rows()==0){

to check if that username/password actually exist in the database.
Under that if is "username or password incorrect" and then in the else is this:

Code: 
   setcookie("PasswordCookie", $password, time() + 99999999);
   setcookie("UsernameCookie", $username, time() + 99999999);
   header("Location: index.php");

These will be used on the index page.

index.php:
lets ignore the table for now, it's not meant to be there and won't be there if the register code is being used properly.
I set the cookie bits up:
Code: 
$cookie_username = $_COOKIE["UsernameCookie"];
$cookie_password = $_COOKIE["PasswordCookie"];

Then I run an SQL query: select * from users where username='$cookie_username' and password='$cookie_password'

I then, below that used if(mysql_affected_rows()==0){ again to make sure the info in the cookie is valid, if it isn't the default "You are not logged in", "Register", and "Login" will be shown. If the information in the cookie is vaild then the register and login button will go away and you'll see "Logged in as <username>".

That's basically it, any security holes you can see in there please?
Any security things I have forgotten that I should have??

Thanks
Craig.
 
How are we supposed to audit the system when we only have words (and some code) :p

I can write the most secure words in the world, but then produce the most awful code. Infact I did that recently, too. Don't ask :o

Instead of ($password != $rpassword) you might want to use ($password !== $rpassword) --- identical.

mysql_affected_rows() only works for INSERT, UPDATE or DELETE queries. If you want to find the number of rows of a SELECT query, try using mysql_num_rows() :)

Also, SQL injection prevention is so simple: www.php.net/mysql_real_escape_string - even has a sample function to cut'n'paste. You should use this before *every* variable you send to the db, And *never* trust *any* input, including cookies. And use single quotes, not double quotes, in those $_COOKIE[] vars :)

Also, MD5 (at the least) all passwords stored in the database and never, ever put passwords into a cookie. If you want to go pro then you create a unique hash and store that in a sessions table and also in the cookie, so that you're not even sending the encrypted password in the cookie.
 
Last edited:
Beansprout said:
Also, MD5 (at the least) all passwords stored in the database and never, ever put passwords into a cookie. If you want to go pro then you create a unique hash and store that in a sessions table and also in the cookie, so that you're not even sending the encrypted password in the cookie.
Click to expand...

Thing is, I want them to be able to stay logged in (option will be added later) & sessions only last one session :(
How can I keep them logged in w/o putting pass in cookie ?

I've added the real escape string, does this look right? I done it for all my queries:

Code: 
	$sql = sprintf("INSERT INTO users (username, password) VALUES ('$username', '$password')",
           mysql_real_escape_string($username),
           mysql_real_escape_string($password));




Thx
Craig.
 
Last edited:
i can't see anyone stealing your code when (no offence) there are far better and free examples that already exist.
 
Craig321 said:
Thing is, I want them to be able to stay logged in (option will be added later) & sessions only last one session :(
How can I keep them logged in w/o putting pass in cookie ?
Click to expand...
Session table, not PHP sessions :p

Actually looking at vB it stores a permanent username & password cookie and also a session hash which expires at the end of the browser session.

Just store the encrypted password in a cookie, you'll be fine. Ignore my other suggestion, gets too complicated for stuff you don't need :)
 
Right, i have encrypted the passes, and they're encrypted in the database.

processreg.php:
I have simply done this: $pass = crypt($password); and then in the insert query it inserts $pass which is now encrypted.

Then to use it on the index and for login I do this:

authenticate.php:
Code: 
blah blah.... WHERE username='$username' AND password=ENCRYPT('".$password."',password)

index.php:
Code: 
$cookie_username = $_COOKIE['UsernameCookie'];
$cookie_password = $_COOKIE['PasswordCookie'];

// Run the query... as usual for SQL coding...
$sql = sprintf("SELECT username, password FROM users WHERE username='$cookie_username' AND password=ENCRYPT('".$cookie_password."',password)",
		mysql_real_escape_string($username),
		mysql_real_escape_string($password));
$result = mysql_query($sql);

Does that look all right ? Also, please check my last post, I have edited in what I've done to avoid sql injection.

Mainly would like to know if I have done the escapes correctly as the encryption works fine. :)

Thx
Craig.
 
Last edited:
The Link I Gave said:
Example 3. A "Best Practice" query

Using mysql_real_escape_string() around each variable prevents SQL Injection. This example demonstrates the "best practice" method for querying a database, independent of the Magic Quotes setting.
<?php
// Quote variable to make safe
function quote_smart($value)
{
// Stripslashes
if (get_magic_quotes_gpc()) {
$value = stripslashes($value);
}
// Quote if not integer
if (!is_numeric($value)) {
$value = "'" . mysql_real_escape_string($value) . "'";
}
return $value;
}

// Connect
$link = mysql_connect('mysql_host', 'mysql_user', 'mysql_password')
OR die(mysql_error());

// Make a safe query
$query = sprintf("SELECT * FROM users WHERE user=%s AND password=%s",
quote_smart($_POST['username']),
quote_smart($_POST['password']));

mysql_query($query);
?>

The query will now execute correctly, and SQL Injection attacks will not work.
Notes

Note: A MySQL connection is required before using mysql_real_escape_string() otherwise an error of level E_WARNING is generated, and FALSE is returned. If link_identifier isn't defined, the last MySQL connection is used.

Note: If magic_quotes_gpc is enabled, first apply stripslashes() to the data. Using this function on data which has already been escaped will escape the data twice.

Note: If this function is not used to escape data, the query is vulnerable to SQL Injection Attacks.
Click to expand...

;) :)

Also, it looks like you're still using raw passwords in the cookie unless I missed something :)
 
Last edited:
Yea, had a little problem with that. I can post it into the cookie encrypted, but then when it comes to using it on the index page from the cookie I'm not sure how to do that. I know how to use it from a SQL DB, but not from a cookie. Any ideas plz?

[Edit]

I'm just adding in the best practice code, I take it it's alright if i do this?

$username = $_POST["username"];

And then in the query instead of putting $_POST etc. I put $username ?

Craig.
 
Last edited:
Yup, so long as you filter it through the necessary functions like mysql_real_escape_string() :)

I'm also not sure what you mean about using it on the index page - couldn't a simple echo work?
 
Beansprout said:
Yup, so long as you filter it through the necessary functions like mysql_real_escape_string() :)

I'm also not sure what you mean about using it on the index page - couldn't a simple echo work?
Click to expand...

I have done the "best practice code". Is this all correct? :

Code: 
// Quote variable to make safe
function quote_smart($value)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $value = stripslashes($value);
   }
   // Quote if not integer
   if (!is_numeric($value)) {
       $value = "'" . mysql_real_escape_string($value) . "'";
   }
   return $value;
}

// Include the config file
include "config.php";

$username = $_POST["username"];
$password = $_POST["password"];

// Run the query... as usual for SQL coding...
$sql = sprintf("SELECT username, password FROM users WHERE username='$username' AND password=ENCRYPT('".$password."',password)",
		   quote_smart($username),
           quote_smart($password));
$result = mysql_query($sql);
.

Also, on the index page the cookies are called.
When the cookie is called SQL checks if the password/username is correct in the cookie, if not I will make it delete the cookie(s).
Anyway, for some reason when I encrypt the password in the cookie it won't show that the person is logged in, it'll just show the usual register and login buttons meaning SQL thinks the information in the cookie is invalid.
Any ideas why plz?
 
OK >.<

Here's the index.php

Code: 
<?PHP
//Ask nicely for the cookies...
$cookie_username = $_COOKIE['UsernameCookie'];
$cookie_password = $_COOKIE['PasswordCookie'];

// Run the query... as usual for SQL coding...
$sql = sprintf("SELECT username, password FROM users WHERE username='$cookie_username' AND password=ENCRYPT('".$cookie_password."',password)",
		mysql_real_escape_string($cookie_username),
		mysql_real_escape_string($cookie_password));
$result = mysql_query($sql);

// Ask SQL if the info in the cookie is valid. If not print the below message
if(mysql_affected_rows()==0){
   echo "<br>You are not logged in.";
   echo "<br><a href='register.php'>Register</a>";
   echo "<br><a href='login.php'>Login</a>";
}

// Ask SQL if the information in the cookie is valid. If it is show the logout button & info.
else{
	echo "<br><br>Logged in as $cookie_username";
	echo "<br><a href='logout.php'>Logout</a><br>";
}
?>

Basically whenever I'm encrypting the password in the cookie then the index page won't accept that cookie. Any ideas plz?
 
I think this is wrong:

$sql = sprintf("SELECT username, password FROM users WHERE username='$cookie_username' AND password=ENCRYPT('".$cookie_password."',password)",
mysql_real_escape_string($cookie_username),
mysql_real_escape_string($cookie_password))
Click to expand...
Because you're not using a %s etc?
 
Hi,

When I use this:
setcookie("PasswordCookie", crypt($password), time() + 99999999);

At the login bit to set the login cookie it doesn't want to show "You are logged in" on the index page, I have changed it to a normal $sql = "blah"; like you suggested but it's still doing the same thing >.<

Any idea please?

[Edit]
I just looked at the encrypted cookie and it's nothing like the encrypted password in the database, so that's probably why it isn't working :S :(

[Edit]
OK, I found out when the cookie is being set it's being encrypted as $1$/mxQBkFs$gzV.7/zC/HsPZOyhsbhDY/ instead of $1$5anYHnkZ$0F4bXXf3i5IapYbIXKHWn/ for some reason :(

Any idea why please :) ?

[Another Edit]
Right, I was browsing on PHP about encrypting and looked at MD5, so I replace all encrypt( with md5( and all the ENCRYPT in the queries with MD5 and now it works perfect :D

Thanks.

Craig.
 
Last edited:
Use MD5 or SHA1.

Make sure you're storing something more than just the password in the cookie, else you're not going to know what user they are - it'd be a bit silly if I could log in as anyone else who happened to have picked the same password as me!

So to conclude, the steps should be:

  1. I register using the password "scruttocks".
  2. The registration process adds the SHA1 hash of "scruttocks" (ced3a7d693c3747f8dd7bf5b26bb62054ba2804f), along with my other details, to the database.
  3. I log in to the site using my username and password.
  4. The login script hashes the password I give with SHA1, and compares it to the one in the database row corresponding to the username I give.
  5. If the username and password is correct, you create session and cookie values corresponding to my user ID (either username or actual ID, it doesn't matter so long as it's unique) and SHA1'd password.
  6. At the top of every single page, you check these session and cookie values to make sure they're still correct - you musn't allow a user to log in as themselves and then change the user ID in their cookie to an admin's without having the correct password hash.

Also, as a quick aside, there's no need to use htmlentities() when you're just comparing a value, and it could outright break the system if someone used a remotely strange character in their username. Only use htmlentities() if the user's input will ever be outputted to the page.

And yep, Beansprout's right about the sprintf. It is useful for escaping values, however - it can make things a bit more concise. Take this example:

Code: 
$sql = sprintf(
'INSERT INTO foo
(lol, rofl)
VALUES("%s", "%s")
',
mysql_real_escape_string($lol),
mysql_real_escape_string($rofl)
);

Saves a bit of space without sacrificing readability (IMO, of course).
 
Hi Rob,

When someone logs in it checks to see if the username and password are in the database, that's all fine.

I have this bit:

Code: 
//Ask nicely for the cookies...
$cookie_username = $_COOKIE['UsernameCookie'];
$cookie_password = $_COOKIE['PasswordCookie'];

// Run the query... as usual for SQL coding...
$sql = "SELECT username, password FROM users WHERE username='$cookie_username' AND password='$cookie_password'";

$result = mysql_query($sql);

// Ask SQL if the info in the cookie is valid. If not print the below message
if(mysql_affected_rows()==0){
	
   echo "<br>You are not logged in.";
   echo "<br><a href='register.php'>Register</a>";
   echo "<br><a href='login.php'>Login</a>";
   
}

// Ask SQL if the information in the cookie is valid. If it is show the logout button & info.
else{
	echo "<br><br>Logged in as $cookie_username";
	echo "<br><a href='logout.php'>Logout</a><br>";
}

This is the part, as you know, that checks the username and password. If I put this on every page, will that be alright? I have tested it and edited a cookie to a different username, when I do all it does is goes back to being logged out which seems fine :)

robmiller said:
And yep, Beansprout's right about the sprintf. It is useful for escaping values, however - it can make things a bit more concise. Take this example:

Code: 
$sql = sprintf(
'INSERT INTO foo
(lol, rofl)
VALUES("%s", "%s")
',
mysql_real_escape_string($lol),
mysql_real_escape_string($rofl)
);

Saves a bit of space without sacrificing readability (IMO, of course).
Click to expand...

Do I not need that whole big bit of coding above the SQL connection then like php.net says? The coding that beanspout posted (http://forums.overclockers.co.uk/showpost.php?p=6319027&postcount=7). Can I just have the mysql_real_escape_string bits instead of all that?

==[Edit]==
Basically, can I have:
Code: 
	$sql = sprintf("INSERT INTO users (username, password) VALUES ('%s', '%s')",
		   mysql_real_escape_string($username),
           mysql_real_escape_string($pass));

Instead of all that coding at the top of here:

http://forums.overclockers.co.uk/showpost.php?p=6319281&postcount=10

Thanks
:)
Craig.
 
Last edited:
You've got a potential SQL injection there - cookies are stored on users' computers, and are just as open to spoofing as GET or POST data. Escape, escape, escape!

And you should really deal with magic quotes, as the code does in Beansprout's example. Quoting from myself, stick this at the top of your main include or wherever is convenient:

Code: 
function remove_magic_quotes($array) {
    foreach ($array as $k => $v) {
        if (is_array($v)) {
            $array[$k] = remove_magic_quotes($v);
        } else {
            $array[$k] = stripslashes($v);
        }
    }
    return $array;
}
if (get_magic_quotes_gpc()) {
    $_GET    = remove_magic_quotes($_GET);
    $_POST   = remove_magic_quotes($_POST);
    $_COOKIE = remove_magic_quotes($_COOKIE);
}

Then use mysql_real_escape_string on all input you'll use in queries.
 
Craig321 said:
Do I not need that whole big bit of coding above the SQL connection then like php.net says? The coding that beanspout posted (http://forums.overclockers.co.uk/showpost.php?p=6319027&postcount=7). Can I just have the mysql_real_escape_string bits instead of all that?
Click to expand...
I pasted it all for a reason:D - and I also assumed you'd read the php.net mysql_real_escape_string() page which details exactly what you should do to prevent SQL injection - and it also has an example of an injection attack incase it's hard to visualise :)

Seems like you're doing well though, at least learning lots, which is the whole point - I look forward to ze finished system:)

Handy bit of code there Rob:)
 
You must log in or register to reply here.
Back
Top Bottom