Page 1 of 1

My authentication - is there possible dangerous code?

Posted: Fri Oct 31, 2008 5:41 am
by spamyboy
Here is my authentication class.
I was just wondering if there aren't any highly seen and unknown to me possible security holes.

Code: Select all

 
class authentication {
    var $mysql;
    
    function __construct($mysql) {
       $this->mysql = $mysql;
    }
    
    function authentication() {
    global $smarty, $l;
        if (empty($_SESSION['authentication'])) {
            $smarty->display(TEMPLATE_DIR.'/backend/admin.login.tpl');
            
            if (!empty($_POST) and empty($_POST['user_username'])) {
                $smarty->assign('error', $l['username_field_left_empty']);
            } else if (!empty($_POST) and empty($_POST['user_password'])) {
                $smarty->assign('error', $l['password_field_left_empty']);
            } else if (!empty($_POST)) {
                $result = $this->mysql->query_one_result("SELECT COUNT(*) FROM `gcms_user`
                                                          WHERE `user_username`='".$this->mysql->escape($_POST['user_username'])."'
                                                          AND `user_password`='".$this->mysql->escape($this->secure($_POST['user_password']))."';");
                if (empty($result)) {
                    $smarty->assign('error', $l['username_or_password_is_incorrect']);
                } else {
                    $result = $this->mysql->query_one_result("SELECT `user_id` FROM `gcms_user`
                                                              WHERE `user_username`='".$this->mysql->escape($_POST['user_username'])."'
                                                              AND `user_password`='".$this->mysql->escape($this->secure($_POST['user_password']))."';");
                    // Setting session
                    $_SESSION['authentication'] = $result;
                    
                    // [int setting cookie]
                    if (isset($_POST['remember'])) {
                        setcookie("cookie_username", $_POST['user_username'], time()+3600);
                        setcookie("cookie_password", $this->secure($_POST['user_password']), time()+3600);
                    }
                    
                    header('Location: admin.php');
                }
            }
            exit;
        } else if (!empty($_SESSION['authentication']) and isset($_GET['logout'])) {
            $this->logout();            
        }
    }
    
    function check_authentication() {
        if (empty($_SESSION['authentication']) and (!empty($_COOKIE['cookie_username']) and !empty($_COOKIE['cookie_password']))) {
            $result = $this->mysql->query_one_result("SELECT COUNT(*) FROM `gcms_user`
                                                      WHERE `user_username`='".$this->mysql->escape($_COOKIE['cookie_username'])."'
                                                      AND `user_password`='".$this->mysql->escape($this->secure($_COOKIE['cookie_password']))."';");
            if (empty($result)) {
                $smarty->assign('error', $l['username_or_password_is_incorrect']);
            } else {
                $result = $this->mysql->query_one_result("SELECT `user_id` FROM `gcms_user`
                                                          WHERE `user_username`='".$this->mysql->escape($_COOKIE['cookie_username'])."'
                                                          AND `user_password`='".$this->mysql->escape($this->secure($_COOKIE['cookie_password']))."';");
                // Setting session
                $_SESSION['authentication'] = $result;
            }
        } else if (!empty($_SESSION['authentication'])) {
            $result = $_SESSION['authentication'];
            return $result;
        } else {
            return FALSE;
        }
    }
    
    function logout() {
        unset($_SESSION['authentication']);
        setcookie("cookie_username", NULL);
        setcookie("cookie_password", NULL);
        header('Location: '.BASE_URL);
    }
    
    // Used so that it would be easier later to change encode method.
    function secure($data) {
        return md5($data);
    }
}
 

Re: My authentication - is there possible dangerous code?

Posted: Fri Oct 31, 2008 6:05 am
by Mordred
1. Don't keep the username/password in cookies. One XSS and they are gone. Use sessions only, but read point 2.
2. Use session_regenerate_id() on login to prevent session fixation (although originally your script is not vulnerable as it re-checks the credentials)
3. Salt and pepper the password in the database: viewtopic.php?t=62782

Re: My authentication - is there possible dangerous code?

Posted: Fri Oct 31, 2008 12:29 pm
by spamyboy
How about now?

Code: Select all

 
class authentication {
    var $mysql;
    var $session;
    
    function __construct($mysql, $session) {
       $this->mysql     = $mysql;
       $this->session   = $session;
    }
    
    function authentication() {
    global $smarty, $l;
        $session_var['authentication']  = $this->session->_read('authentication');
        if (empty($session_var)) {
            $smarty->display(TEMPLATE_DIR.'/backend/admin.login.tpl');
            
            if (!empty($_POST) and empty($_POST['user_username'])) {
                $smarty->assign('error', $l['username_field_left_empty']);
            } else if (!empty($_POST) and empty($_POST['user_password'])) {
                $smarty->assign('error', $l['password_field_left_empty']);
            } else if (!empty($_POST)) {
                $result = $this->mysql->query_one_result("SELECT COUNT(*) FROM `gcms_user`
                                                          WHERE `user_username`='".$this->mysql->escape($_POST['user_username'])."'
                                                          AND `user_password`='".$this->mysql->escape($this->secure($_POST['user_password']))."';");
                if (empty($result)) {
                    $smarty->assign('error', $l['username_or_password_is_incorrect']);
                } else {
                    $result = $this->mysql->query_one_result("SELECT `user_id` FROM `gcms_user`
                                                              WHERE `user_username`='".$this->mysql->escape($_POST['user_username'])."'
                                                              AND `user_password`='".$this->mysql->escape($this->secure($_POST['user_password']))."';");
                    
                    $this->session->_write('authentication', $result);
                    
                    header('Location: admin.php');
                }
            }
            exit;
        } else if (!empty($session_var['authentication']) and isset($_GET['logout'])) {
            $this->logout();            
        }
    }
    
    function check_authentication() {
        $session_var['authentication']  = $this->session->_read('authentication');
        return (!empty($session_var['authentication'])) ? $session_var['authentication'] : FALSE;
    }
    
    function logout() {
        $this->session->_destroy('authentication');
        header('Location: '.BASE_URL);
    }
    
    // Used so that it would be easier later to change encode method.
    function secure($data) {
        return md5($data);
    }
}