Posted by Chris Shiflett on 12/23/05 06:56

Greg Donald wrote:
> I've been using this database driven PHP sessions setup for years:
>
> http://dbsessions.destiney.com/

I offer a similar implementation:

http://phpsecurity.org/code/ch08-2

A few notes about yours (if you don't mind the critique):

1. It doesn't use a separate database connection, but it creates one
anyway. I create a separate one, but only so that it can be dropped into
any web application without adversely affecting it. In practice, I
prefer to use a single connection.

2. Your sess_close() function does nothing, and this can make the
session mechanism behave unexpectedly for a developer. For example,
someone who actually depends upon session_write_close() for concurrent
connections is severely impacted.

3. $GLOBALS[tb_sessions] is invalid syntax. It works, but only because
PHP will check to see if you meant $GLOBALS['tb_sessions'] after it
fails to find a constant named tb_sessions.

4. Your sess_read() function should return an empty string when no data
is found, not FALSE. I originally made this same mistake.

5. Your sess_read() function has an SQL injection vulnerability. The
argument passed comes directly from the user.

6. Your sess_write() function has the same SQL injection vulnerability.

7. In sess_write(), you also don't escape the session data, and this
will give you problems, even with legitimate data that doesn't represent
an SQL injection attack. For example, try setting a session variable
that contains a quote somewhere:

$_SESSION['publisher'] = "O'Reilly";

8. Your sess_write() function has the same SQL injection vulnerability.

9. Your sess_destroy() function has the same SQL injection vulnerability.

10. Your sess_gc() function returns the number of affected rows.
Shouldn't it return TRUE on success? You may just be depending upon the
evaluation of any non-zero integer to TRUE.

11. Your sess_gc() function ignores the session configuration and
chooses its own expiry.

I hope this was helpful. I don't mean any disrespect - quite the
opposite, since you're freely sharing your code. I applaud that. :-)

I think the SQL injection vulnerability is the biggest flaw. Most
everything else doesn't matter too much, but that's a big one, and it
exists in most of your session functions. A simple call to
mysql_real_escape_string() can protect against this vulnerability (in
these cases), and you'll see that I use this function on everything I
use in my SQL queries, even when it seems ridiculous to do so:

$access = time();
$access = mysql_real_escape_string($access);

Hope that helps.

Chris

--
Chris Shiflett
Brain Bulb, The PHP Consultancy
http://brainbulb.com/

[Back to original message]