Coding Critique

Xtremer360 · October 28, 2009

I was hoping someone could take a second and look down my code and see if they see any problems with how it was written before I continue on.

<?php 

require "backstageconfig.php";
require "backstagefunctions.php";

ob_start();
//if the login form is submitted
if(isset($_POST['submit']))
{
    // makes sure they filled it in
    if(!$_POST['username'] || !$_POST['password'])
    {
        die('You did not fill in a required field.');
    }
   $username = mysql_real_escape_string($_POST['username']); 
   $pass = mysql_real_escape_string($_POST['password']); 

    $check = mysql_query("SELECT * FROM users WHERE username = '".$username."'")or die(mysql_error());

    //Gives error if user dosen't exist
    $check2 = mysql_num_rows($check);
    if ($check2 == 0)
    {
        die('That user does not exist in our database.');
    }
    while($info = mysql_fetch_array( $check )) 
    {
        $pass = md5(stripslashes($_POST['password']));
        $info['password'] = stripslashes($info['password']);
        //$_POST['pass'] = md5($_POST['pass']); THIS IS DONE IN THE ABOVE STATEMENT
        //gives error if the password is wrong
        if ($pass != $info['password'])
        {
            die('Incorrect password, please try again.');
        }
        else 
      
      // if login is ok then we add a cookie and send them to the correct page
        { 
            $username = stripslashes($username); 
         $_SESSION['username'] = $username; 
         $_SESSION['loggedin'] = time();
            
            // Finds out the user type
            $query = "SELECT `admin` FROM `users` WHERE `username` = '" . $username . "'";
            $result = mysql_query($query) or die(mysql_error()); 
            $row = mysql_fetch_array($result); 
            $admin = $row['admin'];
         $_SESSION['admin'] = $admin;

#########################################
######## ADMIN SCRIPT CAN BE ADDED BELOW
#########################################
if(isset($_SESSION['admin'])) { ?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta http-equiv="Content-Style-Type" content="text/css">
<meta http-equiv="Content-Language" content="en-us">
<meta name="language" content="en-us">
<title>Backstage V1 Administration Console</title>
<link rel="stylesheet" href="backstage.css" type="text/css" media="screen">
</head>
<body>
<div id=container>
<div class=header>
<table cellpadding="0" cellspacing="0" border="0" width="95%">
<tr>
<td width=110 align=center></td>
<td></td>
<td width=40 valign=bottom align=right>
<a href="#" onclick="">Home</a> | <a href="#" onclick="">Logout</a> | <a target="_blank" href="http://kansasoutlawwrestling.com/phpBB3">Forums</a></td>
</tr>
</table>
</div>
<div id=container2>
<div id=nav>
<?php if(isset($_SESSION['loggedin']))   { ?>
<h1>Character</h1>
<ul>
<li><a href="#" onclick="">Biography</a></li>
<li><a href="#" onclick="">Allies</a></li>
<li><a href="#" onclick="">Rivals</a></li>
<li><a href="#" onclick="">Quotes</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']))   { ?>
<h1>Submit</h1>
<ul>
<li><a href="#" onclick="">Roleplay</a></li>
<li><a href="#" onclick="">News</a></li>
<li><a href="#" onclick="">Match</a></li>
<li><a href="#" onclick="">Seg</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>  
<h1>Handler</h1>
<ul>
<li><a href="#" onclick="">Directory</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>  
<h1>Booking</h1>
<ul>
<li><a href="#" onclick="">Champions</a></li>
<li><a href="#" onclick="">Booker</a></li>
<li><a href="#" onclick="">Compiler</a></li>
<li><a href="#" onclick="">Archives</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>  
<h1>Fed Admin</h1>
<ul>
<li><a href="#" onclick="">Handlers</a></li>
<li><a href="#" onclick="">Characters</a></li>
<li><a href="#" onclick="">Applications</a></li>
<li><a href="#" onclick="">Event Names</a></li>
<li><a href="#" onclick="">Title Names</a></li>
<li><a href="#" onclick="">Match Types</a></li>
<li><a href="#" onclick="">Divisions</a></li>
<li><a href="#" onclick="">Arenas</a></li>

</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>  
<h1>Site Admin</h1>
<ul>
<li><a href="#" onclick="">Templates</a></li>
<li><a href="#" onclick="">Content</a></li>
<li><a href="#" onclick="">Bio Configuration</a></li>
<li><a href="#" onclick="">News Categories</a></li>
<li><a href="#" onclick="">Menus</a></li>
</ul>
<?php } ?>
</div>
<div id=content>

</div>
<div id="footer">Backstage 1 © 2009
</div>
</div>
</div>
</body>
</html>
<?php  
#########################################
######## ADMIN SCRIPT HAS TO END ABOVE
#########################################
    }
        } 
    } 
} 
else 
{
// if they have not submitted the form
?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta http-equiv="Content-Style-Type" content="text/css">
<meta http-equiv="Content-Language" content="en-us">
<meta name="language" content="en-us">
<title>Backstage V1 Administration Console</title>
<link rel="stylesheet" href="backstage.css" type="text/css" media="screen">
</head>
<body>
<div id=login>
<form method="POST" action="/mybackstage/backstage.php">
<h1>KOW Backstage</h1>
<p><label>Username:<br><input type="text" name="username" id="log" tabindex="1"></label></p>
<p><label>Password:<br><input type="password" name="password" id="pwd" tabindex="2"></label></p>
<p style="text-align: center;"><input type="submit" class="button" name="submit" id="submit" value="Login »" tabindex="4"></p>
</form>
</div>
</body>
</html>
<?php
}
?>

Daniel0 · October 28, 2009

Well, there are a couple of things:

1) Calling die() is not an appropriate way of handling errors.

2) You should use some sort of salting along with the password hashing.

3) Your usage of stripslashes() seems to suggest that you are running with magic quotes on; turn it off.

4) You should separate your presentational logic from your business logic (separations of concerns).

5) You are assuming that $_POST['username'] and $_POST['password'] are both set. If they are not you will get an E_NOTICE. You should check that an index exists in an array before you use it (unless you already know it does).

6) You don't need to MySQL escape $_POST['password'] seeing as you aren't using it in any query.

7) You are selecting the same user from the database twice, which is obviously redundant.

mrMarcus · October 28, 2009

you don't seem to using $pass to check the user against the db .. you forget to add it to the query? otherwise, just a username is needed to login.

Daniel0 · October 28, 2009

Just thought of another thing:

Assuming your variable upholds the invariant that usernames are unique, you needn't use a while loop when fetching the info because there will only every be at most one row returned. Seeing as you've also verified that a row was actually returned, you don't even need to check that mysql_fetch_array() returns the result. If it doesn't it would be a bug in PHP.

Xtremer360 · October 28, 2009

I appreciate everyone's willingness to look over my coding and find errors and problems with my coding. The only thing with me as a coder and keeping in mind I'm still a beginner learning the ropes as I go I can understand for the most part the issues but don't know what needs to go about and modifying the code to make it better itself.

Can someone be more specific on some of the lines that DanielO was referring to please?

mrMarcus · October 28, 2009

for my advice:

<?php
$username = mysql_real_escape_string ($_POST['username']);

/*
* now, your passwords should be hashed
* in the db using something like md5();
* they will look something like: f418380f96b42ece06ff40d664da19c0
* this would be done when inserting the user
* into the db: $passwordGoingIntoDb = md5 (mysql_real_escape_string ($_POST['password']));
* so, your $pass must be first hashed
* using the exact same method as was
* used when initially putting the
* password into the database, and then checked
* against that same value in the db, otherwise,
* they won't match;
*/
$pass = md5 (mysql_real_escape_string ($_POST['password']));

$check = mysql_query("SELECT * FROM users WHERE username = '".$username."' AND password = '".$pass."' LIMIT 1") or die (mysql_error());
?>

EDIT: for Daniel's #3, magic_quotes can be turned off in your php.ini file

Xtremer360 · October 28, 2009

Thank you. However what part of my coding should I replace that with?

for my advice:

<?php
$username = mysql_real_escape_string ($_POST['username']);

/*
* now, your passwords should be hashed
* in the db using something like md5();
* they will look something like: f418380f96b42ece06ff40d664da19c0
* this would be done when inserting the user
* into the db: $passwordGoingIntoDb = md5 (mysql_real_escape_string ($_POST['password']));
* so, your $pass must be first hashed
* using the exact same method as was
* used when initially putting the
* password into the database, and then checked
* against that same value in the db, otherwise,
* they won't match;
*/
$pass = md5 (mysql_real_escape_string ($_POST['password']));

$check = mysql_query("SELECT * FROM users WHERE username = '".$username."' AND password = '".$pass."' LIMIT 1") or die (mysql_error());
?>

EDIT: for Daniel's #3, magic_quotes can be turned off in your php.ini file

mrMarcus · October 28, 2009

at the top where you have:

$username = mysql_real_escape_string($_POST['username']); 
$pass = mysql_real_escape_string($_POST['password']); 

$check = mysql_query("SELECT * FROM users WHERE username = '".$username."'")or die(mysql_error());

Xtremer360 · October 28, 2009

Okay that takes care of yours now I should work on figuring out DanielO's 8 points to correct.

Alex · October 28, 2009

Addressing Daniel0's suggestions won't be as simple as "change this line, and this line". Essentially you'd need to rewrite the entire code. For example, error handling is a key part of application design, and using die() is a bad habit, so you'd really need to rethink your entire logic. Implementing some form of SoC (like MVC) would be even more drastic, and isn't something you'll master over-night.

Daniel0 · October 29, 2009

1) Calling die() is not an appropriate way of handling errors.

As AlexWD said, this is a non-trivial change. Essentially the script is designed fundamentally bad.

2) You should use some sort of salting along with the password hashing.

This is more easy. A salt is something that is hashed along with the actual password. While a hash cannot be "decrypted" (that word doesn't even make sense in this context, but that's another story), a brute-force or dictionary attack would be possible if you somehow obtain the hash. Salting foils this attack because there are lots of random stuff added to the hash (which makes dictionary attacks useless) and it becomes longer (which makes brute-forcing take a long time). Of course if you get the salt and know where it is positioned, you're back to where you started.

A good idea may be using a salt that changes occasionally and is stored in the user row, and another static salt that doesn't which is stored in a config file. You can change the user specific salt whenever you've got the plaintext password (that would be when the user logs in or changes his password).

Read up on salting on the internet, or search the forum archives here.

3) Your usage of stripslashes() seems to suggest that you are running with magic quotes on; turn it off.

This is trivial, turn of magic_quotes_gpc off in php.ini.

4) You should separate your presentational logic from your business logic (separations of concerns).

See #1.

5) You are assuming that $_POST['username'] and $_POST['password'] are both set. If they are not you will get an E_NOTICE. You should check that an index exists in an array before you use it (unless you already know it does).

Things such as empty, isset and key_exists will help you with that. So instead of if (!$_POST['username']) you could do if (empty($_POST['username]']) because empty() checks that the index is actually exists as well.

6) You don't need to MySQL escape $_POST['password'] seeing as you aren't using it in any query.

In this case, just remove that statement entirely. It's redundant, you aren't using it and you're overwriting it a little while later.

7) You are selecting the same user from the database twice, which is obviously redundant.

Just use the same info throughout the entire script. This should be fairly trivial.

Assuming your variable upholds the invariant that usernames are unique, you needn't use a while loop when fetching the info because there will only every be at most one row returned. Seeing as you've also verified that a row was actually returned, you don't even need to check that mysql_fetch_array() returns the result. If it doesn't it would be a bug in PHP.

Just remove the while structure (though retain the statement within it) including the brackets that belong to it.

for my advice:

<?php
$username = mysql_real_escape_string ($_POST['username']);
$pass = md5 (mysql_real_escape_string ($_POST['password']));

$check = mysql_query("SELECT * FROM users WHERE username = '".$username."' AND password = '".$pass."' LIMIT 1") or die (mysql_error());
?>

First of all, md5() isn't vulnerable to SQL injection attacks, so escaping isn't needed. Actually it's incorrect because it changes the string.

Secondly, when he checks the password in his script, he needn't select where that password matches as well. Actually, when he implements salting he might store the salt in the database and in that case he cannot do what you're suggesting.

keldorn · October 29, 2009

Some of the points mentioned here is why I use Smarty. I've read alot people bashing smarty saying its useless, but it can address and solve namely these 2 problems,

1. Seperate business logic from presentation

2. generate Friendly error message instead of Die();

I dont know where I would be without Smarty, I probably would still have horrible ways to generate error messages. But this is how I generate them. Bascially saying I'm checking some $_POST. (this for an example there is no validation really except for checking their empty.)

//registration.php

//we have a post!
if($_POST){
  $error = false;
  $email = $_POST['email'];
  $name = $_POST['name'];
  

   if(empty($email)){
    $error .= "<li>Your forgot your email</li>\n";
   }

   if(empty($name)){
    $error .= "<li>Your forgot your name</li>\n";
   }

//ERROR?
if($error){

$smarty->assign('error',$error);
$smarty->display('registration.tpl);
// All done
exit;

}

}

Then in the .tpl file I would have something like this,

{if isset($error)}<ul>{$error}</ul>{/if}

:touche:

magic_quotes_gpc off

That might be bad to base your script around assuming its off. What if down the road you end up on different hosting where its on, and you cant' control it? you totally forget about ti and now site is messed up and your pulling your hair going threw the code your forget how it works and it was commented bad... yadda. lol

Checking for it I would say is still the best.

So in the example above we simple modify it a bit to include this.

#MAGIC QUOTES
if(function_exists('get_magic_quotes_gpc')) {
$magic_quotes = true;
} 

//we have a post!
if($_POST){
  $error = false;
  $email = $_POST['email'];
  $name = $_POST['name'];


if($magic_quotes == true){

		   $name   = stripslashes($name);
		   $email  = stripslashes($email);

		   
		}
  

   if(empty($email)){
    $error .= "<li>Your forgot your email</li>\n";
   }

   if(empty($name)){
    $error .= "<li>Your forgot your name</li>\n";
   }

//ERROR?
if($error){

$smarty->assign('error',$error);
$smarty->display('registration.tpl);
// All done
exit;

}

}

Daniel0 · October 29, 2009

magic_quotes_gpc off

That might be bad to base your script around assuming its off. What if down the road you end up on different hosting where its on, and you cant' control it? you totally forget about ti and now site is messed up and your pulling your hair going threw the code your forget how it works and it was commented bad... yadda. lol

Then you should probably find another host. It's been turned off by default for several years, been bad practice for even longer, and it's officially deprecated right now.

Some of the points mentioned here is why I use Smarty. I've read alot people bashing smarty saying its useless, but it can address and solve namely these 2 problems,

1. Seperate business logic from presentation

2. generate Friendly error message instead of Die();

It can solve it, but it doesn't necessarily do it, and it certainly does not enforce it.

Re #2: No, that is still up to yourself to do. Smarty doesn't prevent you from calling die() in your business logic if you don't know any better.

Re #1: Ironically, you have mixed presentational logic with business logic in the code you posted.

Touché yourself

keldorn · October 29, 2009

It works for me. :pirate:

Oh yeah about checking if a $_POST variable is actually set. Say $name = $_POST['name']; and $_POST['email']; which you were expecting ,but some wanker sends only one of the variables threw a Post becuase there casing your script for holes. That would generate a Full Path Disclosure saying, undefined index: var in /path/path/path/, you could as you said use isset() or empty() on each $_POST,

like

if(isset($_POST['var'])){
   $var = $_POST['var'];
}
if(!empty($_POST['var2'])){
   $var2 = $_POST['var2'];
}

I think that would quickly get redundant and annoying having to type that out.

Would you think using the @ would be better?, which will remove the error.

Say this

$name = @$_POST['name'];
$email = trim(@$_POST['email']);

I've tried this and it seems to work quite well.

Daniel0 · October 29, 2009

If e.g. $_POST['name'] doesn't exist then why would you want to reassign a non-existent value to another variable? That doesn't make sense.

That would generate a Full Path Disclosure saying, undefined index: var in /path/path/path/,

You shouldn't run with display_errors=on in a production environment.

Would you think using the @ would be better?, which will remove the error.

Say this
$name = @$_POST['name'];
$email = trim(@$_POST['email']);
I've tried this and it seems to work quite well.

It doesn't remove the error, it suppresses it. As a general rule, you should never suppress errors, you should make sure they do not happen.

Using the error suppression operator @ is also inefficient. It's equivalent to doing this:

$oldErrorReporting = error_reporting(0);
$name = $_POST['name'];
error_reporting($oldErrorReporting);

mrMarcus · October 29, 2009

First of all, md5() isn't vulnerable to SQL injection attacks, so escaping isn't needed. Actually it's incorrect because it changes the string.

my bad. sometimes i get carried away with ideas and fail to think things entirely through.

EDIT: to the OP, having: md5 (mysql_real_escape_string ($_POST['password'])); is incorrect .. if you allow for special characters in your password, primarily ' or ", mysql_real_escape_string will escape those (\'), then md5() will hash the escaped characters, changing the hashed password entirely.

Secondly, when he checks the password in his script, he needn't select where that password matches as well. Actually, when he implements salting he might store the salt in the database and in that case he cannot do what you're suggesting.

why wouldn't you check a user's password via the query? aren't we talking 6 of one half, half dozen of the other? saves several more lines of PHP just to determine the same thing as the query would immediately. a password is not even being checked in the posted script .. how is that secure? at least a password check in the query as i stated would prove to ensure a user retains integrity of their account. of course you can check the password after the query, but how is that any more efficient?

understandably wouldn't work with a db driven salt pattern, unless you wanted to add a multiple queried format, but that'd be quite inefficient.

nothing of what i have advised is unsafe, and judging by the OP's inexperience, i don't believe that over-complicating login queries with salting methods and such is going to anything but confusing for someone just learning.

Daniel0 · October 29, 2009

a password is not even being checked in the posted script

Yes it is:

if ($pass != $info['password'])
        {
            die('Incorrect password, please try again.');
        }

of course you can check the password after the query, but how is that any more efficient?

I don't recall having spoken about efficiency.

saves several more lines of PHP just to determine the same thing as the query would immediately.

Shorter code isn't necessarily neither better nor faster.

understandably wouldn't work with a db driven salt pattern, unless you wanted to add a multiple queried format, but that'd be quite inefficient.

Which is all I ever said. Everything else is stuff you made up.

mrMarcus · October 29, 2009

not trying to turn this into a shouting match .. couldn't care less to.

do you disagree then that this wouldn't work equally as well:

<?php
//stuff...
$check = mysql_query ("SELECT username, password FROM users WHERE username = '".$username."' AND password = '".$pass."' LIMIT 1");
if (mysql_num_rows ($check) > 0)
{
//do login stuff;
}
else
{
// password doesn't match .. display
// error and/or go back to form;
}
?>

Daniel0 · October 29, 2009

I don't believe I ever said so. I said that when he implemented salting it could complicate things.

keldorn · October 29, 2009

Something like this would work for checking a user login.


//database connection here


$username = mysql_real_escape_string($_POST['username']);
$password = md5($_POST['password'];
$sql = mysql_query("SELECT password FROM members WHERE username='{$username}'");
$mysql_result = mysql_fetch_row($result);

if(!is_array($mysql_result)){

$error = "No username found by that username";
}elseif($password ==  $mysql_result['password'])) {
// log in stuff
} else {

$error = "Oops your password is wrong";

}

Sign In

Coding Critique

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information