Is this script secure?

[shauny17]

Hi guys would it be possible for you to check this script; i'm nt to sure that what i am doing is secure, it is a process script from a login form on my site. Any tips or watever would be thankfully welcomed:-

<?php
	$serverURL = "http://localhost/beta/modules/myAccount/login.htm";
	$host = "MySQL Host"; //Website host eg. localhost
	$username = "MySQL UID"; //Database username eg. root
	$password = "MySQL PWD"; //Database password eg. password
	$database = "MySQL DB"; //MySQL Schema on server eg. MyDB

session_start(); //Start the session

if($_SERVER['HTTP_REFERER'] == $serverURL){; 
$userName = $_POST['uid'];
$passWord = md5($_POST['pwd']);

$_POST['uid'] = "Spacebar Computers (NW)";
$_POST['pwd'] = "Spacebar Computers (NW)";


//Connect to MySql Database
$loginSQL = mysql_connect($host,$username,$password)
	or die (include 'error.htm');

//Select MySQL Schema on server
mysql_select_db($database)
	or die (include 'error.htm');

$loginQuery = mysql_query("SELECT `id`,`username`,`password`,`real_name` FROM `users` WHERE `username` = '$userName'")
	or die (include 'error.htm');
$loginResults = mysql_fetch_array($loginQuery);
if($loginResults['username'] = $userName && $loginResults['password'] = $passWord){
	$_SESSION['personalID'] = $userName;
	$_SESSION['personalWord'] = $passWord;
	$userName = "Spacebar Computers (NW)";
	$passWord = "Spacebar Computers (NW)";
	$_SESSION['name'] = $loginResults['real_name'];
	header("Location:../../index.php");

} else {
	print "Error!";
}
}

else {

header("Location: http://www.spacebarcomputers.com");
}
?>

NOTE:$serverURL will be changed when the site is entered into production.

[Weedpacket]

Having different variables with names that differ only in case is rather bad form.

If you're not using the values of the id or real_name fields there's not a lot of point in selecting them. That leaves the username and password fields and you already know the value of the username field, so you don't need that either. (Any particular reason why you're using PHP to compare passwords instead of doing it in the database? Or why you're not hashing the password?)

You could avoid starting a lot of irrelevant sessions by waiting to session_start() until you know you're going to be using it, i.e., until after you've validated the login. Speaking of which,

$loginResults['username'] = $userName && $loginResults['password'] = $passWord

Don't you mean

$loginResults['username'] == $userName && $loginResults['password'] == $passWord

    $_POST['uid'] = "Spacebar Computers (NW)";
    $_POST['pwd'] = "Spacebar Computers (NW)"; 

Are these intended to achieve anything?

And the big security problem: you never check the value of $username (sorry, $userName) before embedding it in the query. Bad, bad, bad, very very bad.

[shauny17]

Weedpacket wrote:

    $_POST['uid'] = "Spacebar Computers (NW)";
    $_POST['pwd'] = "Spacebar Computers (NW)"; 

Are these intended to achieve anything?

I set these because i thought that it is possible for somebody to grab these variables, and the password is hashed the second it is received by the form.

How do you send the results to the database for comparison instead of using a script to compare them?

Also, the variables are set out like that (eg. $fooBar) so that the script is easier for me to read because i am dyslexic and it all starts to move on the screen if its the same case 🙂

How can i check the $userName before it is passed to SQL? would it be using stripslashes()?

THanks for your help Weedpacket!

[Piranha]

shauny17 wrote:

How do you send the results to the database for comparison instead of using a script to compare them?

SELECT COUNT(*) FROM table WHERE username = $uname AND password = $pass
Then check that the value returned is exactly 1, if so then you have a match in username and password. Note that this query will always return 1 row, you have to look at the value returned.

shauny17 wrote:

How can i check the $userName before it is passed to SQL? would it be using stripslashes()?

The risk is database injection, and you use [man]mysql_real_escape_string[/man] to check strings for that. Int, double and other number formats can use the same and be written inside ' signs or you can use [man]is_numeric[/man].

I have a few more points to make.
1. First is that you might want to check that username and password variables exist and that they are not empty.
2. Second you might want to have this file in a folder that can't be reached from Internet, otherwise you might get in trouble if something happens to PHP and your information about the database may be visible.
3. Third I wonder what happens after this code if the user are not validated to enter the site. It is only outputting "Error!", but do you catch it someway?
4. Forth I wonder what happens after this code if the user are valudated to enter the site. I can only see that they user gets redirected, but you have to validate the user on every page.

[shauny17]

Yes, in the script, if the user is auth'd successfully, then 3 variables are added to the session, real_name, uid, and an MD5 hash of the password, the script is stored 3 folders in from the main public site, but my site is hosted on a windows server, how can i change my script so if for some reason PHP does not parse the page then it prevents security details from being shown? is it safer to define the security vars in a seperate file and include_once that file?

For the error handling (ie user not authed) then it will redirect to an error page with a link back to the login page., i havent designed that page yet so i just have print "error!" in place of that for now.

for checking empty strings wud i do:

if($userName != "" && $passWord != ""){
//auth user?
}
else {
//Redirect, error
}

Thanks for your advice Piranha

[NogDog]

You might want to check out these two articles on sessions and security:
Session fixation and session hijacking
Shared host issues with session data

[Piranha]

Look at [man]isset[/man] and [man]empty[/man] to see if if it is exists and are empty. Note that you need to first see if it exists, then to see if it is empty.

If you use Apache as the webserver you can restrict access to folders from the Internet. I can't remember the command though.

[shauny17]

unfortunately, i use Apache as a testing server, but my webhost uses IIS, so i dnt no really how i can sort the permissions problem, plus wud i change it to allow read only or what?

[Piranha]

You should change it to no access at all. Then you include that file in the file that can be read.

[Weedpacket]

shauny17 wrote:

I set these because i thought that it is possible for somebody to grab these variables

If those values were going to be "grabbed" it would be before the script starts (in fact, before they even reach the script, even before they reach the server), not while the script is running. If an attacker was going to see the values of those variables while the script was running, they would have to be directly monitoring the memory of your server, identifying activity by process. If they can do that then your box is already pwn3d! LOLOLOLOLOL!

Besides, if they could read the contents of $POST, they could read the contents of $userName. Or $REQUEST, for that matter. And if they could read the contents of $_POST when the script is at that point in the code they could read it three lines (or a couple of milliseconds) earlier.

the password is hashed the second it is received by the form.

I admit I did not see that. I think I may have been looking for $password instead of $passWord.

Also, the variables are set out like that (eg. $fooBar) so that the script is easier for me to read because i am dyslexic and it all starts to move on the screen if its the same case

I've got no problem with using capitalisation in variable names, but your explanation doesn't explain why you use $username AND $userName, which is what I was pointing out. (Doesn't $username move on the screen? It's all the same case....)

[shauny17]

yes $username moves on the screen, but them details are correct, my host is big on security, they claim there servers are this this and that keycard security access blah blah blah blah probably rented servers from the data centr strip on M62, you think it would be safer to hash the username aswell, and have them both stored in the database, is there a way wer i can MD5 both username and password before it reaches the script? also, i think it would be safer if i had a hidden field with something like <input type=hidden name=legit value=true> you thinik?

[Piranha]

shauny17 wrote:

also, i think it would be safer if i had a hidden field with something like <input type=hidden name=legit value=true> you thinik?

The user can still see that in the source code. So no, it would not be any safer at all to use that.

[Weedpacket]

Client-side security is too easy to circumvent because you don't control the client, the user does.
http://forums.thedailywtf.com/forums/thread/99361.aspx

Should you hash the password client-side? You could (there are Javascript implementations of MD5), but why bother? If someone can get hold of the plaintext password and send it then they can hash it themselves and send the hash.

Should you hash the username? Sure, if you're not going to use it for anything other than validating the login. In which case, why have the username, why not just the password, since all you'd be doing is splitting the password into two fields?

Server-side hashing could be improved. MD5 is crumbling at the edges and for Unclassified material the U.S. Government uses the SHA2 [man]hash[/man] functions (SHA-256, SHA-384, etc.)