Page 1 of 1

please have a look at this code

Posted: Wed Jun 17, 2009 10:15 am
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';
            }
        }
    }
}
 

Re: please have a look at this code

Posted: Wed Jun 17, 2009 11:58 am
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.

Re: please have a look at this code

Posted: Wed Jun 17, 2009 1:14 pm
by kaisellgren
Can I see get_data() and decrypt()?

Re: please have a look at this code

Posted: Wed Jun 17, 2009 1:27 pm
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.

Re: please have a look at this code

Posted: Wed Jun 17, 2009 1:39 pm
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.

Re: please have a look at this code

Posted: Wed Jun 17, 2009 2:22 pm
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.

Re: please have a look at this code

Posted: Fri Jun 26, 2009 8:43 pm
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!

Re: please have a look at this code

Posted: Sat Jun 27, 2009 6:17 am
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.

Re: please have a look at this code

Posted: Sat Jun 27, 2009 6:36 am
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"

Re: please have a look at this code

Posted: Sat Jun 27, 2009 12:16 pm
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.

Re: please have a look at this code

Posted: Sat Jun 27, 2009 12:21 pm
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.

Re: please have a look at this code

Posted: Sat Jun 27, 2009 12:29 pm
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.

Re: please have a look at this code

Posted: Sat Jun 27, 2009 12:36 pm
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.

Re: please have a look at this code

Posted: Sat Jun 27, 2009 1:04 pm
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.