please have a look at this code

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
toffler
Forum Newbie
Posts: 8
Joined: Sat Sep 13, 2008 4:23 am

please have a look at this code

Post by 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

Code: Select all

 
start_session();
 
Login page contains the following code:

Code: Select all

 
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:

Code: Select all

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

Code: Select all

 
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';
            }
        }
    }
}
 
Last edited by Weirdan on Wed Jun 17, 2009 12:47 pm, edited 1 time in total.
Reason: added [code=php] tags
Eric!
DevNet Resident
Posts: 1146
Joined: Sun Jun 14, 2009 3:13 pm

Re: please have a look at this code

Post by Eric! »

I don't think you need session_start on the pages you allow general access do you?

Everything you've done there looks good and is similar to my site but I'm not an expert. You might consider additional filters on your input variables to protect your database from other non ascii chacters when the account is created (like *).

Also use trim() on the form fields because users sometimes insert a space and can't see it in the form field.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: please have a look at this code

Post by kaisellgren »

Can I see get_data() and decrypt()?
toffler
Forum Newbie
Posts: 8
Joined: Sat Sep 13, 2008 4:23 am

Re: please have a look at this code

Post by toffler »

Eric! wrote:I don't think you need session_start on the pages you allow general access do you?
well, yes, maybe its a bit more than necessary..those pages use session to store some personal settings so I protect that session regenerating its id and such
Eric! wrote: Everything you've done there looks good and is similar to my site but I'm not an expert. You might consider additional filters on your input variables to protect your database from other non ascii chacters when the account is created (like *).

Also use trim() on the form fields because users sometimes insert a space and can't see it in the form field.
Thanks, I'm not an expert as well that's why I want to be sure that at least I'm not missing something important here. The input from $_GET and $_POST is protected with mysql_real_escape_string() and trim(). This is the first thing done on every page receiving input.
toffler
Forum Newbie
Posts: 8
Joined: Sat Sep 13, 2008 4:23 am

Re: please have a look at this code

Post by toffler »

kaisellgren wrote:Can I see get_data() and decrypt()?
Encryption and decryption is done via Rijndael-256, I can post the code here if you like but it's quite big and pretty much standard.
get_data() uses quite a few of other functions but in case of MySQL as the DB it generates a request which looks like

Code: Select all

SELECT login,password,iv FROM `table` WHERE login = $post['login'] LIMIT 1

and here $post is a copy of $_POST which has been escaped and trimmed.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: please have a look at this code

Post by kaisellgren »

toffler wrote:Encryption and decryption is done via Rijndael-256, I can post the code here if you like but it's quite big and pretty much standard.
I suggest using hashes over encryption when it comes to password protection, but if you decide to use encryption, I could take a look at your encryption process, too.
toffler wrote:in case of MySQL as the DB
You are using several RDBMS?
toffler wrote:

Code: Select all

SELECT login,password,iv FROM `table` WHERE login = $post['login'] LIMIT 1
It needs quotes around the variable.
toffler
Forum Newbie
Posts: 8
Joined: Sat Sep 13, 2008 4:23 am

Re: please have a look at this code

Post by toffler »

kaisellgren wrote:....
I don't want to go into details here.
The point of this topic was about the security of this code, what I would like to know if this is secure or not.
So I'd appreciate it if someone with the commercial experience in PHP security gave their opinion.
Thanks!
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: please have a look at this code

Post by kaisellgren »

No one can tell whether code is secure or not without giving it...

The SQL query you showed is vulnerable to SQL injections. Other than that, it looks safe.
danielrs1
Forum Commoner
Posts: 29
Joined: Wed Jun 24, 2009 5:30 pm

Re: please have a look at this code

Post by danielrs1 »

Instead of this code;

Code: Select all

SELECT login,password,iv FROM `table` WHERE login = $post['login'] LIMIT 1
I suggest you to use this one:

Code: Select all

"SELECT `login`,`password`,`iv` FROM `table` WHERE `login` = '".mysql_real_escape_string($post['login'])."' LIMIT 1"
toffler
Forum Newbie
Posts: 8
Joined: Sat Sep 13, 2008 4:23 am

Re: please have a look at this code

Post by toffler »

kaisellgren wrote:No one can tell whether code is secure or not without giving it...

The SQL query you showed is vulnerable to SQL injections. Other than that, it looks safe.
It's a pity you never bothered to read my posts.
As I already mentioned twice "$_GET and $_POST is protected with mysql_real_escape_string() and trim()" so NO, it's NOT vulnerable.

As to this peculiar line of code bothering you so much as I said before "..a request which looks like..". It's not a real request or a part of actual code, it was the answer to you off-topic question about how to query MySQL. I know how to do it, all single and double quotes are in place, code is working, it is protected from injection. I hope now you are satisfied.
toffler
Forum Newbie
Posts: 8
Joined: Sat Sep 13, 2008 4:23 am

Re: please have a look at this code

Post by toffler »

danielrs1 wrote:Instead of this code;

Code: Select all

SELECT login,password,iv FROM `table` WHERE login = $post['login'] LIMIT 1
I suggest you to use this one:

Code: Select all

"SELECT `login`,`password`,`iv` FROM `table` WHERE `login` = '".mysql_real_escape_string($post['login'])."' LIMIT 1"
Same here, thank you very much for pointing that out. Before posting anything here, please read the original post. It's very unfortunate that this line of text is all that got your attention and all you can talk about.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: please have a look at this code

Post by kaisellgren »

toffler wrote:all single and double quotes are in place, code is working, it is protected from injection.
Based on what you have given me:
toffler wrote:

Code: Select all

SELECT login,password,iv FROM `table` WHERE login = $post['login'] LIMIT 1

and here $post is a copy of $_POST which has been escaped and trimmed.
It would be vulnerable to SQL injections.
toffler wrote:It's a pity you never bothered to read my posts.
As I already mentioned twice "$_GET and $_POST is protected with mysql_real_escape_string() and trim()" so NO, it's NOT vulnerable.
I read it twice, and yes, it is vulnerable, because you never enclosed the value within quotes in your SQL query.

Security issues often come from mistakes - not just from bad design, so, if you care about security, the actual code must be evaluated.
toffler
Forum Newbie
Posts: 8
Joined: Sat Sep 13, 2008 4:23 am

Re: please have a look at this code

Post by toffler »

Before posting anything here please read the original message, the code I'm asking about is posted there not anywhere below.
My question is NOT about MySQL injection, it IS about using sessions to create a login/password SECURITY protection for web pages.
Also the code contains some common sense generalization like

Code: Select all

header('Location: 'a password protected page URL');
and

Code: Select all

$loginfo = get_data();
As long as my question is about SESSIONS the code contains enough information to see how security is implemented and that's all I'm interested in.

kaisellgren and the others please do NOT point out that code lines above will not work and I need to put quotes somewhere or I have to put ALL the code and ALL function descriptions here. I have put everything in regards to SESSION based security.
I'll answer your questions about the code in the original post if they are related to SESSIONS and the use of SESSIONS.

speciallly for kaisellgren: you still don't bother to read my posts.. Ok, I'll repeat for you for the third time and this will be the last one:
As to this peculiar line of code bothering you so much as I said before "..a request which looks like..". It's not a real request or a part of actual code, it was the answer to you off-topic question about how to query MySQL. I know how to do it, all single and double quotes are in place, code is working, it is protected from injection.
Please, stop spamming here.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: please have a look at this code

Post by kaisellgren »

I don't understand why would you want to throw irrelevant code at me, but the session system you have built seems all fine. Nothing makes it inherently insecure. If you want to increase its security, tie it to an IP address.
Post Reply