5
\$\begingroup\$

Please check this PHP script. This script is for Login authentication

Some pre-defined constants to indicate the login state

define('LOGIN_NOERROR',0);
define('LOGIN_USER_CREDENTIALS',-1);
define('LOGIN_USER_EXISTS',-2);
define('LOGIN_USER_NONEXISTS',-3);
define('LOGIN_PASSWORD_LINKSENT',-4);
define('LOGIN_PASSWORD_BADMATCH',-5);
define('LOGIN_PASSWORD_TOOSHORT',-6);
define('LOGIN_PASSWORD_LINKEXPIRED',-7);
define('LOGIN_SESSION_EXPIRED',-8);

class auth {
private $db = null; // Database object
private $baseurl = 'http://localhost/admin.php'; // a URL that will have this script  included. Used for password reset emails which require a hyperlink

private $hashfunction = 'sha256'; // Hash function used, this is always computed by PHP due to mysql versions giving binary & non-binary outputs depending on its version
private $hashlength = 32; // Length of $this->hashfunction output in binary format
private $secret = 'LDGH$$$$$'; // A secret salt used in passwords alongside user-specific salts, change this

public $account = array(); // User details on successful login

private $errors = array( // Error array for when there is a UI issue for the user
    LOGIN_NOERROR=>'',
    LOGIN_USER_CREDENTIALS=>'Incorrect username/password',
    LOGIN_USER_EXISTS=>'Username already exists on our system',
    LOGIN_USER_NONEXISTS=>'Username does not exist on our system',
    LOGIN_PASSWORD_LINKSENT=>'A password reset link has been sent to you in email. Follow the link inside your email to create a new password.',
    LOGIN_PASSWORD_BADMATCH=>'Passwords do not match',
    LOGIN_PASSWORD_TOOSHORT=>'Passwords should be at least 8 characters',
    LOGIN_PASSWORD_LINKEXPIRED=>'The password reset link has expired',
    LOGIN_SESSION_EXPIRED=>'Your session has timed out'
);
public $forms = array( // Unique HTML forms used
    'signin'=>array(
        'fields'=>array(
            'username'=>array('type'=>'text','placeholder'=>'Your E-Mail Address','icon'=>'envelope'),
            'password'=>array('type'=>'password','placeholder'=>'Password','icon'=>'lock')
        ),
        'submit'=>'Login',
        'message'=>'Please login with your username and password'
    ),
    'signup'=>array(
        'fields'=>array(
            'newusername'=>array('type'=>'text','placeholder'=>'Your E-Mail Address','icon'=>'envelope'),
            'newpassword'=>array('type'=>'password','placeholder'=>'Password','icon'=>'lock'),
            'confirmnewpassword'=>array('type'=>'password','placeholder'=>'Confirm Password','icon'=>'lock')
        ),
        'submit'=>'Sign Up',
        'message'=>'Provide your email address and create a password to sign up'
    ),
    'lost'=>array(
        'fields'=>array(
            'lostusername'=>array('type'=>'text','placeholder'=>'Your E-Mail Address','icon'=>'envelope')
        ),
        'submit'=>'Email Me',
        'message'=>'Enter your email address to receive password reset instructions'
    ),
    'reset'=>array(
        'fields'=>array(
    'newpassword1'=>array('type'=>'password','placeholder'=>'Password','icon'=>'lock'),
            'newpassword2'=>array('type'=>'password','placeholder'=>'Confirm Password','icon'=>'lock')
        ),
        'submit'=>'Reset Password',
        'message'=>'Enter a new password for your account'
    )
);

Initiate the database if its not connected already

public function __construct($dbobj = null) {
    !$dbobj->connect_errno 
        or die("Failed to connect to MySQL: (" . $dbobj->connect_errno . ") " . $dbobj->connect_error);
    $this->db = &$dbobj;    

    if(isset($_COOKIE['sess']) && $this->session_validate()) {
        // Logged In here

        if(isset($_GET['logout']))
            $this->logout();
    }
    else {
        // Sign in attempt
        if(isset($_POST['username'],$_POST['password']))
            $this->login($_POST['username'],$_POST['password']); 
        // Sign up attempt
        else   if(isset($_POST['newusername'],$_POST['newpassword'],$_POST['confirmnewpassword']))
            $this- >user_add($_POST['newusername'],$_POST['newpassword'],$_POST['confirmnewpassword']); 
        // Lost password, email submitted via form
        else if(isset($_POST['lostusername'])) 
            $this->password_reset_form($_POST['lostusername']); 
        // Lost password area
        else if(isset($_GET['reset']) && $this->session_valid($_GET['reset'])) {
            // Form not submitted
            if(!isset($_POST['newpassword1'],$_POST['newpassword2']))
                $this->session_check($_GET['reset'],'lostpassword'); 
            // Form submitted
            else
                $this->password_reset($_GET['reset'],$_POST['newpassword1'],$_POST['newpassword2']);
        }
        // Login as user, for use within an admin area to impersonate a logged in user
        // You should add in the session value yourself in the admin area using $this->session_add($userid,$type = 'login') and then login via this method/URI
        else if(isset($_GET['sess']) && $this->session_valid($_GET['sess'])) {
            if(($this->session_check($_GET['sess'],'login') == 0) && isset($this->account['id'])) {
                setcookie("sess",$_GET['sess'],time() + 86400);
                $this->redirect();
            }
            else 
                sleep(1);
        }

None shall pass (unless logged in...)

        die($this->form(LOGIN_NOERROR,(isset($_GET['form']) && in_array($_GET['form'],array('signup','lost','reset')) ? $_GET['form'] : 'signin')));
    }
}

Add a session to the hash table

public function session_add($userid,$type = 'login') {
    $hash = hash($this->hashfunction,bin2hex(openssl_random_pseudo_bytes($this->hashlength)));
    $result = $this->query('INSERT IGNORE sessions (hash,sessiontype,userid,created) 
        VALUES (UNHEX(\''.$hash.'\'),\''.$type.'\','.$userid.',UNIX_TIMESTAMP())');
    return $hash;
}

Run this in a cron job once an hour to remove stale sessions and lost password requests

public function housekeeping() {
    // Remove sessions older than 1 day
    // Remove password reset authentication strings after 1 hour
    $this->query('DELETE FROM sessions 
        WHERE (sessiontype = \'login\' AND created < UNIX_TIMESTAMP() - 86400)
        OR (sessiontype = \'lostpassword\' AND created < UNIX_TIMESTAMP() - 3600);');
}

User is not logged in, display one of the forms

private function form($error,$formname) {

    // Throttle failed attempts
    if($formname == 'signin' && $error != 0)
        sleep(1);

    // Show a sign up or sign in link in the navigation         
    if($formname == 'signin')
        $link = '<a href="?form=signup">Sign Up</a>';
    else
        $link = '<a href="'.$this->clean_uri().'">Sign In</a>';

    // Get all form fields and buttons
    $formfields = '';
      foreach($this->forms[$formname]['fields'] as $name => $field)
        $formfields .= sprintf('<div class="input-group input-group-lg">
                <span class="input-group-addon"><i class="glyphicon glyphicon-%s blue"></i></span>
                <input name="%s" type="%s" placeholder="%s" class="form-control">
                </div>
                <div class="clearfix"></div>',$field['icon'],$name,$field['type'],$field['placeholder']);
    $formfields .= sprintf('<p class="center col-md-5"><button class="btn btn-primary" type="submit">%s</button></p>',$this->forms[$formname]['submit']);

    // Navigation links for sign up/sign in/forgot password                             
    $navigation = '<ul class="nav navbar-nav navbar-left">
<li>'.$link.'</li>
<li><a href="?form=lost">Lost Password?</a></li>
</ul>';

Form wrapped in bootstrap 3.0 HTML with variables inserted

    $form = sprintf('<div class="well col-md-5 center login-box">
        <div class="alert alert-info">%s </div>
        <form method="post" action="?form=%s" class="form-horizontal">%s</form>
    </div>',($error != 0 ? $this->errors[$error] : $this->forms[$formname]   ['message']),$formname.(isset($_GET['reset']) ? '&amp;reset='.$_GET['reset'] :  ''),$formfields);

    // The above HTML is taken from a bootstrap template, you can place it into an existing template as such, using %s placeholders for the content area and navigation, for example.
    // echo sprintf(file_get_contents('template.html'),$navigation,$form);
    // otherwise, here is the raw output that is used

    echo $navigation.'<hr>'.$form;

    exit(0);
}

// User is trying to log in
private function login($username,$password) {
    $result = $this->query('SELECT id,salt,password
        FROM users 
        WHERE username = \''.$this->db->real_escape_string($username).'\';');
    // We fetch the row because MySQL's SHA2() functions returns either a binary of hex string format depending on version. 
    // For simplicity the comparison is made in PHP, though it's trivial to change this to save the roundtrip of data 
    if(!($this->account = $result->fetch_array(MYSQLI_ASSOC)))
        die($this->form(LOGIN_USER_CREDENTIALS,'signin'));
    else if($this->account['password'] != pack('H*',hash($this->hashfunction,$this->secret.$this->account['salt'].$password))) 
        die($this->form(LOGIN_USER_CREDENTIALS,'signin'));

    // Successful login, you're about to be logged in and redirected
    $this->query('UPDATE users 
        SET lastlogin = UNIX_TIMESTAMP()
        WHERE id = '.$this->account['id']);
    $hash = $this->session_add($this->account['id'],'login');
    setcookie("sess",$hash,time() + 86400);
    $this->redirect();
}


// Add a new user to the database and sign them in
private function user_add($username,$password,$password2) { 
    if($password != $password2) // Passwords do not match
        die($this->form(LOGIN_PASSWORD_BADMATCH,'signup'));
    elseif(strlen($password) < 8) // Password less than 8 characters
        die($this->form(LOGIN_PASSWORD_TOOSHORT,'signup'));

    $salt = openssl_random_pseudo_bytes($this->hashlength);
    $hash = pack("H*",hash($this->hashfunction,$this->secret.$salt.$password));

    $this->query('INSERT IGNORE users (created,username,salt,password) 
        VALUES (UNIX_TIMESTAMP(),\''.$this->db->real_escape_string($username).'\',\''.$this->db->real_escape_string($salt).'\',\''.$this->db->real_escape_string($hash).'\');');
    if($this->db->affected_rows < 1) 
        die($this->form(LOGIN_USER_EXISTS,'signup'));
    $this->login($username,$password);
}

Reset a password, displays the reset password form if a valid authentication string is provided

private function password_reset_form($username) {
    $result = $this->query('SELECT id
        FROM users 
        WHERE username = \''.$this->db->real_escape_string($username).'\'');
    if(!($row = $result->fetch_array(MYSQLI_ASSOC)))
        die($this->form(LOGIN_USER_NONEXISTS,'lost'));

    $hash = $this->session_add($row['id'],'lostpassword');
    $emailcontents = sprintf("Hello, follow this link to reset your password\n\n%s?form=reset&reset=%s\n\nBest Regards",$this->baseurl,$hash);
    // Here you would send the reset link to an email address (the whole idea of ensuring this is the rightful owner of the account
    // ... but for testing purposes , the output of the email is below
    // mail($username,'Reset your password',$emailcontents);
    echo "<hr>$emailcontents<hr>";

    die($this->form(LOGIN_PASSWORD_LINKSENT,'reset'));
}

On successful reset password link, allow the user to reset their password

 private function password_reset($reset,$password,$password2) {

    if(($error = $this->session_check($reset,'lostpassword')) < 0)
        die($this->form($error,'reset'));
    if($password != $password2)
        die($this->form(LOGIN_PASSWORD_BADMATCH,'reset'));
    elseif(strlen($password) < 8)
        die($this->form(LOGIN_PASSWORD_TOOSHORT,'reset'));

    $salt = openssl_random_pseudo_bytes($this->hashlength);
    $hash = pack('H*',hash($this->hashfunction,$this->secret.$salt.$password));

    $this->query('UPDATE sessions AS s
        INNER JOIN users AS u ON s.userid = u.id
        SET u.salt = \''.$this->db->real_escape_string($salt).'\',u.password = \''.$this->db->real_escape_string($hash).'\'
        WHERE s.hash = UNHEX(\''.$reset.'\') AND s.sessiontype = \'lostpassword\'');
    $this->query('DELETE FROM sessions
        WHERE hash = UNHEX(\''.$reset.'\') AND sessiontype = \'lostpassword\'');

    $this->login($this->account['username'],$password);
}

// Log out
private function logout() {
    $this->query('DELETE FROM sessions
        WHERE sessions.sessiontype = \'login\' AND sessions.hash = UNHEX(\''.$_COOKIE['sess'].'\');');
    setcookie("sess","",time() - 3600);
    $this->redirect();
}

private function redirect() {
    header('Location: //'.$_SERVER['HTTP_HOST'].$this->clean_uri());
    exit(0);
}

// Removes login-specific details from the current URI  
private function clean_uri() {
    return preg_replace("'[\?&](form|reset|logout|sess)=[^&]+'",'',$_SERVER['REQUEST_URI']);
}

// Validate that a user-provided session is syntactically valid
private function session_valid($hash) {
    return preg_match("'^[a-f0-9]{".($this->hashlength*2)."}$'",$hash);
}

// Check a session cookie to see whether it's valid, and logged in or not
private function session_validate() {
    if(!isset($_COOKIE['sess']) || !$this->session_valid($_COOKIE['sess']))
        die($this->form(LOGIN_SESSION_EXPIRED,'signin'));
    if($this->session_check($_COOKIE['sess'],'login') < 0) {
        setcookie("sess","",time() - 3600);
        die($this->form(LOGIN_SESSION_EXPIRED,'signin'));
    }
    return $this->account['id'];
}

Look up the hash table for a given session in a given context

private function session_check($hash,$type = 'login') {
    $result = $this->query('SELECT u.id,u.flags,u.created,u.lastlogin,u.username
        FROM sessions AS s
        INNER JOIN users AS u ON s.userid = u.id
        WHERE s.hash = UNHEX(\''.$hash.'\') AND s.sessiontype = \''.$type.'\';');
    if(!$this->account = $result->fetch_array(MYSQLI_ASSOC)) {
        unset($_GET['reset']);
        die($this->form(LOGIN_PASSWORD_LINKEXPIRED,'reset'));
    }
    return 0;
}

MySQL queries

private function query($sql) {
    $result = $this->db->query($sql) or die(__LINE__.' '.$this->db->error.' '.$sql);
    return $result;
}
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Security

  • sprintf('html', ($error != 0 ? $this->errors[$error] : $this->forms[$formname] ['message']),$formname.(isset($_GET['reset']) ? '&amp;reset='.$_GET['reset'] : ''),$formfields): first of all, this line is a bit too compact for my taste, and thus hard to read. But the relevant part is this: echo '<form method="post" action="?form=$_GET[\'reset\']">', which is open to XSS attacks.
  • real_escape_string is one way to prevent SQL injection, but the preferred way are prepared statements. Not necessarily because they are safer, but because they are easier to use. real_escape_string results in really ugly code with a lot of \', which are quite easy to overlook or forget.
  • I think this ugliness and unusability is also the reason you don't always use it, but just assume that values are safe. Currently, this is actually true for eg $type and your user id (but will it be in the future? who knows). But it is not true for $reset, which just comes out of GET and $hash which comes out of COOKIE (the use of UNHEX doesn't affect the injection, it can just be closed by eg 2D31') [injected code]; injections into insert and update are also possible).
  • You should check out which SERVER fields are user controlled and which aren't. <a href="'.$this->clean_uri().'">Sign In</a> with clean_uri basically being the user controlled $_SERVER['REQUEST_URI'] might be open to XSS (eg "+onload=alert(1), although I'm having problems with " being encoded, but I wouldn't just assume that there is no way around it).
  • you should use password_hash (which uses bcrypt) instead of hash, as it supports multiple rounds, manages salts for you (you can still use a pepper if you want to), and automatically includes a timing safe comparison.

To summarize: You have some security issues in your code (XSS and SQL injection), and your general handling of security could be better. Just use prepared statements for all data, that way you don't have to worry about which data is currently user supplied and which isn't, and you also don't have to worry about data that is currently constant becoming user supplied later on. Same goes for echoing and XSS prevention.

General Readability and Structure

I mostly like your code. It's clean, easy to read, and well structured for the most part.

It's a bit unclear the way you posted it here, but it seems that all this code is in one giant file. I would try to split it up, so that it is easier to reuse and it is easier to find the parts of the code that you are interested in. You could split it up by type (eg all database interacting functions in one class, all functions using HTML in another, and the controlling parts in a third), or by functionality (eg a password reset file, a login file, etc), or both.

Misc

  • use either camelCase or snake_case (or this-case for GET/POST) instead of alllowercase (eg confirmnewpassword, hashlength, formname, emailcontents, etc would be a bit easier to read that way).
  • always use curly brackets
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.