Request for review - cookies and sessions

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
bertfour
Forum Commoner
Posts: 45
Joined: Fri Mar 07, 2008 7:33 am

Request for review - cookies and sessions

Post by bertfour »

Dear Phpeople,

I am working on a php app, and it will be an app which I will publish at some time... (free to use)

I have some experience writing php stuff, but I am no crack at all. Things work.

But this will be my first app published online, and I am a bit worried about security, so I would like more experienced coders to have a look at some of my ideas...

First, sessions and cookies.

I have written my own "session manager", and it is a cookie only session manager.

It works like this:

All installations of the app will have a unique "key", generated on installation.

When a user logs in, and supplies the right credentials, a session var is generated (the usual 32 char thing) and in the session table a "hashed" (with unique key) var is stored which contains the first 3 numbers of the IP address, the browser agent, language encoding and user_id (from users_table).

The session cookie is stored on the clients PC.

When a page is requested and the session cookie is present, the app checks the hashed user data, if its still correct, the session continues.

So, the session is tied to the users IP (first 3 numbers only) and browser. (I think)

The use of the unique installation key for hashing makes sure that other people who have downloaded the app cannot generate valid cookies for other installations of the app.

Here is the code which checks for a valid user:

session_clean() deletes expired sessions
session_user_hash() checks the hash, see below.
set_cookie sets a cookie, similar to phpbb3
clean_string = general function to clean user input from nasty things.

Code: Select all

function get_session_user() {
 global $dbname, $link, $user_auth, $lang, $table_prefix, $sess_lenght ;
 $user_auth[user_id]= -1;
 $user_auth[user_name] = $lang[GUEST];
 $user_auth[user_level] = 0;
 session_clean();
 $theCook = clean_string($_COOKIE['sess_usr']);
 if ($theCook <> "") {
  $query = "SELECT * FROM `" . $table_prefix . "sessions` WHERE `s_cookie` = '$theCook' LIMIT 1";
  $result = mysql_db_query ($dbname, $query, $link);
  $num_rows = mysql_num_rows($result);
  if ($num_rows) {
    $Row = mysql_fetch_array ($result);
    if ($Row[s_hash] == session_user_hash($Row[user_id])) {
     $thetime = time();
     $query = "UPDATE `" . $table_prefix . "sessions` SET `time`='$thetime' WHERE `user_id` = $Row[user_id]";
     $result=mysql_db_query ($dbname, $query, $link);
     set_cookie($theCook,$sess_lenght);
     $query = "SELECT * FROM `" . $table_prefix . "users` WHERE `user_id` = '$Row[user_id]' LIMIT 1";
     $result = mysql_db_query ($dbname, $query, $link);
     $Row = mysql_fetch_array ($result);
     $user_auth = $Row;
    } 
  }
 }
return; 
}
 

Code: Select all

function session_user_hash($usr) {
global $site_data;
$cki = session_ip() . session_browser() . $usr . $site_data[site_hash];
$cki = md5($cki);
return $cki;
}

Code: Select all

 
function session_ip() {
 if ($_SERVER['HTTP_X_FORWARDED_FOR']) {
  $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
 } else {
  $ip = $_SERVER["REMOTE_ADDR"];
 }
 $ipArr = explode(".",$ip);
 $ip = $ipArr[0] . "."  . $ipArr[1] . "." . $ipArr[2] ;
 return $ip;
}

Code: Select all

function session_browser() {
 $browser = $_SERVER['HTTP_USER_AGENT'];
 $browser .= $_SERVER['HTTP_ACCEPT_LANGUAGE'];
return $browser;
}
(Why only first 3 numbers of IP? because of rotating proxies, like AOL)

My question is of course, is this "safe" enough? What do you say?

thanks,

Bert
Last edited by bertfour on Fri Mar 07, 2008 6:04 pm, edited 1 time in total.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Request for review - cookies and sessions

Post by Mordred »

The devil, as well as the security of your app, lies in the details. So don't hold them: post all your functions if you want a better shot at your system. What you did is not very well argumented - why don't you go with th regular sessions, which are well known, well tested and with well known limitations. Instead you replace them with a "virgin" system of dubious value (not time tested = dubious, doesn't matter who wrote it and how experienced he is). It might work, if executed correctly, and very very carefully.

Possible holes: How is the "unique key" generated? How is a var "cleaned"? How is user_id assigned - autoincrement integer? User IP can be spoofed. Globals can potentially be overwritten. Unchecked inputs may lead to path disclosure if error reporting is on. Post all relevant code.

Right now you have missed some quotes in the SQL, which most probably isn't a problem here, but might be a problem elsewhere. You are using globals, which is a no-no. The ip can be spoofed, so it's dangerous if you use it anywhere else, like logging to a database. $Row[user_id] should be $Row['user_id'], same for the other places you use arrays.

All in all, I suggest you drop this approach, and do it like the whole industry does it. Research how to use sessions with database storage if you need.
User avatar
Zoxive
Forum Regular
Posts: 974
Joined: Fri Apr 01, 2005 4:37 pm
Location: Bay City, Michigan

Re: Request for review - cookies and sessions

Post by Zoxive »

My concern is you stated that the Unique key you generate is based on the users IP, what if they have a dynamic IP which changes frequently?
Well I can tell you, they would no longer be logged in.

Another thought is why create your own session manager in the first place? What is wrong but php's built in sessions?
bertfour
Forum Commoner
Posts: 45
Joined: Fri Mar 07, 2008 7:33 am

Re: Request for review - cookies and sessions

Post by bertfour »

Thanks Mordred...

(went through Bulgaria with train from Istanbul to Budapest few months ago, nice landscapes)

Looked through what I could find on the net, regarding sessions, but didn't find a copy and paste secure piece of code :) (that i understood)

Regarding the "cleaning" of user input, I would like to skip that, and start a new discussion about that later on... I understand its important...
How is user_id assigned - autoincrement integer?
Yes. Is that unsafe? Most apps I have seen have that 8O

Globals ? I don't use those... I have register_globals off....

The globals in the functions are declared elsewhere....(or should be)
$Row[user_id] should be $Row['user_id'], same for the other places you use arrays.
Thanks, always wondered about that...

IP's can be spoofed, I guess so. But if someone is able to do that, and steal cookies as well, no application is secure anymore for that person right?

The IP (+browser) is only used to have a means to make it the cookie stealer more difficult.
All in all, I suggest you drop this approach, and do it like the whole industry does it.
I see some who use their "own" session system in one or another way. phpbb3 seems pretty secure...
bertfour
Forum Commoner
Posts: 45
Joined: Fri Mar 07, 2008 7:33 am

Re: Request for review - cookies and sessions

Post by bertfour »

Zoxive wrote:My concern is you stated that the Unique key you generate is based on the users IP, what if they have a dynamic IP which changes frequently?
Well I can tell you, they would no longer be logged in.

Another thought is why create your own session manager in the first place? What is wrong but php's built in sessions?
I find the build in a bit hard to work with.... But thats me...

It uses only the first 3 numbers of the IP address. I have the same setting on my phpbb3 forum, and nobody is complaining.

As far as I know there is only AOL who uses a rotating (proxy) ip address, and only the last number varies...
Post Reply