security question: please have a look at the code

[toffler]

Hi,

 

After reading some posts I came up with this code, I don't know much about security so I'll appreciate it if someone tells me it's a decent protection or not.

 

The web site has a cms part with a login page and password protected pages inside and normal web pages which use session variables to save some personal display settings.

 

Every normal (not password protected) page on the site starts with

 
start_session();

 

Login page contains the following code:

 

 
start_session();
login();
if(isset($_SESSION['logged']) && $_SESSION['logged']==='yes') {
header('Location: 'a password protected page URL');
exit;
}
<form action="login.php" method="post">
<input type="hidden" name="form_name" value="login">
<input type="text" name="login">
<input type="password" name="password">
<input type="submit" value="Submit">
</form>

 

Every password protected page starts with the following:

 
start_session();
if(!isset($_SESSION['logged']) || $_SESSION['logged']!=='yes') {
    header('Location: 'login page URL');
    exit;
}
...

 

Functions:

 
function start_session() {
    ini_set('session.use_only_cookies', 1);
    if(isset($_GET['PHPSESSID'])) { 
      // output error message
      exit ;
    }
    session_start();
    // session expiration time: 30 min
    if(isset($_SESSION['expire']) && (date("U") - $_SESSION['expire'] > 60*30)) logout();
    if(!isset($_SESSION['ini'])) {
        session_regenerate_id(true);
        $_SESSION['ini'] = 1;
    }
    $_SESSION['expire'] = date("U");
}

function logout() {
    $_SESSION = array();
    if (isset($_COOKIE[session_name()])) 
    setcookie(session_name(), '', time() - 60*session_cache_expire() - 60, '/');
    session_destroy();
}

function login() {
    global $post, $get;
// $post and $get arrays are created from $_GET and $_POST accordingly using  mysql_real_escape_string()
    if(isset($post['form_name']) && $post['form_name']==='login' && 
       isset($post['login']) && isset($post['password']) && $post['login']!='' && $post['password']!='') 
    {
        $loginfo = get_data(); // get login and password from db where login = $post['login']
        if(count($loginfo)>0) {
            $loginfo['password'] = decrypt($loginfo['password']); // the password stored in an encrypted form
            if($loginfo['password']===$post['password']) {
                $_SESSION['logged'] = 'yes';
            }
        }
    }
}

[JonnoTheDev]

Fairly straightforward. If the session is not set the user is redirected to login. This is fine.

One point is the use of global variables. This is bad practice. Do not use them in functions. Functions should accept parameters:

 

// bad
function login() {
    global $post, $get;
}

// good
function login($request) {

}

[toffler]

as a general advice, yes. here it makes perfect sense though.