Page 1 of 1

Critique Please - User Auth

Posted: Sun Feb 01, 2009 9:53 am
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();
}
?>
 

Re: Critique Please - User Auth

Posted: Sun Feb 01, 2009 2:28 pm
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);
 

Re: Critique Please - User Auth

Posted: Sun Feb 01, 2009 3:44 pm
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