Critique Please - User Auth

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
infolock
DevNet Resident
Posts: 1708
Joined: Wed Sep 25, 2002 7:47 pm

Critique Please - User Auth

Post by infolock »

Here's a user Auth page..

I'm looking for anything/everything that's wrong with this. Maybe something like "You should be validating numbers and letters only in user/pass". Also, anything security wise, please let me know. Thanks a bunch!

Code: Select all

 
<?php
session_start();
/**
 * @author Jonathon Hibbard
 * @copyright Copyright (c) 2008, Jonathon Hibbard
 * 
 * This authentication script basically sets up Session variables indicating whether or not the user was successfully authenticated against the mysql database.
 * If the user was, the "Logged In" variable is set to 1 and the username/password is stored.  Else, the value is set to 0 and the session is destroyed.
 * 
 */
define('UA_SEED', 'WEBAPP');
$errors     = array();
 
function redirect_to_login_page() {
  $_SESSION['sid'] = 0;
    session_destroy();
    header('Location: http://localhost/login.php');
    exit;
}
 
try {
  $dbh = new PDO("mysql:host=localhost;dbname=db", 'login', 'pass');
} catch(PDOException $e) {
  $errors[] = "PDO FAILED: " . $e->getMessage();
  redirect_to_login_page();
}
 
if(isset($_SESSION['user_id']) && intval($_SESSION['user_id']) > 0) {
  if($_SESSION['user_agent'] != md5($_SERVER['HTTP_USER_AGENT'] . UA_SEED)) {
    //Need to log this......
    redirect_to_login_page();
  }
 
  $sql = "SELECT user_id FROM user WHERE user_id = '{$_SESSION['user_id']}'";
  $rst = $dbh->query($sql);
 
  $row = $rst->fetch(PDO::FETCH_ASSOC);
 
  if(!isset($row['user_id']) || empty($row['user_id'])) {
    $dbh = null;
    redirect_to_login_page();
  }
 
} else {
  /* if a POST-born username and password were passed, check to see if they exist in the database, and redirect them to the investigator's page. */
  if(isset($_POST['txtUsername']) && !empty($_POST['txtUsername']) && isset($_POST['pwdPassword'])) {
 
    if(empty($_POST['pwdPassword'])) redirect_to_login_page();
 
    $uname = addslashes(substr($_POST['txtUsername'], 0, 30));
    $pass  = addslashes(substr($_POST['pwdPassword'], 0, 80));
    $_POST = array();
 
    $sql = "SELECT user.user_id, user.fname, account_type.account_type_description
            FROM user 
              INNER JOIN account_type ON user.account_type_id = account_type.account_type_id
            WHERE user.username = '$uname' AND user.password = md5('$pass')";
    $rst = $dbh->query($sql);
    $row = $rst->fetch(PDO::FETCH_ASSOC);
 
    if(!isset($row['user_id']) || empty($row['user_id'])) {
      $dbh = null;
      redirect_to_login_page();
    }
 
    $_SESSION['user_agent']    = md5($_SERVER['HTTP_USER_AGENT'] . UA_SEED);
    $_SESSION['user_id']       = $row['user_id'];
    $_SESSION['name']          = $row['fname'];
    $_SESSION['account_type']  = $row['account_type_description'];
 
  } else redirect_to_login_page();
}
?>
 
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Re: Critique Please - User Auth

Post by John Cartwright »

I would get rid of the addslashes and use PDO to bind the parameters. Slashes are not the only things needing escape.

Code: Select all

$sql = "SELECT user.user_id, user.fname, account_type.account_type_description
          FROM user
          INNER JOIN account_type ON user.account_type_id = account_type.account_type_id
          WHERE user.username = ? AND user.password = md5(?)";
$stmt->prepare($sql);
 
$stmt->execute(array(
   substr($_POST['txtUsername'], 0, 30), 
   substr($_POST['pwdPassword'], 0, 80)
));
 
$stmt->bindColumn(...);
 
$return = $stmt->fetch(PDO::FETCH_ASSOC);
 
User avatar
infolock
DevNet Resident
Posts: 1708
Joined: Wed Sep 25, 2002 7:47 pm

Re: Critique Please - User Auth

Post by infolock »

Good call thanks. I just keep hearing that PDO's bind for preparing isn't suposed to be relied on by itself, but maybe i'll try it out and see what else i can do.

should probably through a few statements in there to sanitize the input and make sure we're getting the types of characters expected for u/p
Post Reply