Login

[Obrienray1]

Hey I want you guys to tell me if this hackable, if so please fix it for me as this is my first CMS, and I'm doing pretty well with it so far.

Login:

<?php
if(isset($_POST['submit'])) {
  mysql_connect("localhost", "root", "pass");
  mysql_select_db("shadowcms");

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

  $username = mysql_real_escape_string($username);
  $password = mysql_real_escape_string($password);

  $password = md5($password);

  $sql = mysql_query("SELECT * FROM users WHERE username = '$username' AND password = '$password'");
  $nr = mysql_num_rows($sql);

  if($nr == 0) {
    echo "<div class=errormsg>";
    echo "<center>";
    echo "Username, or Password is incorrect";
    echo "</div>";
    echo "</center>";
      }else{
    while($row = mysql_fetch_array($sql)) {
      $username = $row['username'];
    }
    session_start();
    $_SESSION['lia'] = $username;
    echo "Welcome, ".$_SESSION['lia']."";
    echo "<a href='home.php'>Home page</a>";
    header('Location: home.php');
  }
}
?>
   <center><a href='register.htm'><div class=register_box>Register your <?php echo "$name" ?></div></a></center>
 <center><form action='index.php' method='POST'>
<b>Username: <input type='text' name='username'><br />
<b>Password: <input type='password' name='password'><br />
<input type='submit' name='submit' value='Login'>
</form></center>

Thanks 😉

[Bonesnap]

Not sure about hackable, but the things I notice are:

• Don't connect to the database as root. Setup another user (or many users) to do the querying. Should someone manage to do an SQL injection attack, the damage will be mitigated since they don't have permission to do anything (or very little, anyway).

• Don't put the password through mysql_real_escape_string since you're putting it through MD5. If the user has something in their password such as quotes, this could lead to incorrect logins or other unexpected behaviour (duplicate this for registration, too).

• Are the passwords salted?

• I would personally check if the SELECT statement fails. If it does, output an appropriate message.

• Not PHP-related, but please for the love of God don't use the <center> tag :p (it's deprecated). Use CSS styling instead. For example, for the errormsg class, you can set the CSS property text-align to center (<b> tags should also be <strong> tags).

• You also have HTML tags opening and closing before others are finished closing. That could cause the form to break in certain browsers. HTML tags should be nested properly.

• Why bother outputting a welcome message and link if you're automatically redirecting to home.php with the Header function? Furthermore, nothing should be outputted before the headers anyway. I assume this fails to redirect?

[ScottyBoy]

While I see nothing in that script that's actually hackable, there are a few things that could be improved upon.

Firstly, I'd highly recommend that you check that both $POST['username'] and $POST['password'] have values. Otherwise the script will throw annoying undefined errors when someone doesn't fill a field in.

if (isset($_POST['submit']) && !empty($_POST['username']) && !empty($_POST['password']))

Also, I've found from my own experiences, and from reading around on the net, you should call session_start() first if you intend to use it in your script.

With your SQL string, there seems to be no reason for you to be retrieving every database column in that row if you're only using the username column. This isn't a serious problem, it just uses more resources than needed here.

Finally, I'm fairly sure that your redirect will not work as expected. If you output any kind of text before a redirect, it will give you a "headers already sent" error.

Here's your cleaned up script. I didn't touch that bit of HTML at the bottom, cause I didn't have enough time. 🙂

<?php
    session_start();

if (isset($_POST['submit']) && !empty($_POST['username']) && !empty($_POST['password']))
{
   mysql_connect("localhost", "root", "pass");
   mysql_select_db("shadowcms");

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

   $username = mysql_real_escape_string($username);
   $password = md5($password);

   $sql = mysql_query('SELECT username FROM users WHERE username = "'. $username. '" AND password = "'. $password. '"');
   $nr = mysql_num_rows($sql); 

   if ($nr == 0)
   {
       echo'
       <div class="errormsg" style="text-align: center;">
           <p>Username or password is incorrect.</p>
       </div>';
   }
   else
   {
       while ($row = mysql_fetch_array($sql))
       {
           $_SESSION['lia'] = $row['username'];
       }
       echo'
       Welcome, '. $_SESSION['lia']. '!';
   }
}
?>