Spot the Security Hole

By Harry Fuecks | July 3, 2004 | Programming

If you’ve arrived at this page from the Tech Times newsletter, we apologise. A few of our links went awry. You’re probably after one of the following:

We now return you to our regular program…

Here’s a PHP script:


<?php
# Common include file for MySQL
require("auth_conn_inc_reg.php");
$valid = false;
if (isset($_SERVER['PHP_AUTH_USER']) &&
	isset($_SERVER['PHP_AUTH_PW']) ) {
	$sql = "SELECT * FROM users WHERE
		logins='{$_SERVER['PHP_AUTH_USER']}' AND
		password='{$_SERVER['PHP_AUTH_PW']}'";
	$mysql_result = mysql_query($sql,$connection);
	$num_rows = @mysql_num_rows($mysql_result);
	if ( $num_rows != 0 ) {
		$valid = true;
	}
}
if ( !$valid ) {
	header ("WWW-Authenticate: Basic realm=\"Restricted\"");
	header ("HTTP/1.0 401 Unauthorized");
	echo "Authorization required";
	exit();
} else {
	# Valid user - do stuff here
}
?>

Spot the problem?

What gets me depressed about reading this is it’s part of an article in a UK Linux Magazine this month (I’ll leave the name out; it’s otherwise a good magazine). Sure everyone makes mistakes, myself more than a few but this particular example is a classic and part of why PHP gets flak on security.

What’s it going to take to stop this happening over and over in future? Perhaps on http://www.php.net/mysql_query there needs to a big message like “Before you use this function, make sure you read about mysql_escape_string(). And perhaps the page on mysql_escape_string() could do more to explain why it’s important?

Harry Fuecks

{ 26 comments }

Anonymous November 12, 2008 at 10:11 pm: Another option is to use prepared statements. This way, the parameters supplied to the query are automatically escaped.
I use MSSQL and use stored procedures for all my code. So essentially, from PHP’s perspective, all my database work is just a bunch of functions.
$b_LoggedIn = $o_DB->spLogin($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']);
You cannot supply SQL code to a param and expect it to behave like SQL code.
If I had a user enter :
‘ OR TRUE;–
as a username, this would be a string of 12 characters.
Simple.
Compositing SQL statements in PHP is a fools game. You are ALWAYS going to have to find new ways to protect yourself.
Whereas your SQL server already knows how to protect itself. So using it seems like a MUCH safer option.
DougBTX February 3, 2005 at 9:56 pm: Charlie: it is in the $_SERVER var, but that is just a name. It comes from the window that people have to type in their username/password when they get to a password protected page. See: http://www.zend.com/manual/features.http-auth.php
Douglas
Charlie January 20, 2005 at 11:14 am: Wait, wouldn’t you have to set an .htpassword password or username with a crazy name like ” or 1 = 1″ to make this hack work? Everyone here is assuming that you have access to the server already. So if a hacker has access to the server already what’s the point of being all secure??
malikyte December 7, 2004 at 2:50 pm: Rick, I like your point, however not everyone has the ability to use encrypted based logins; at least if by that you meant something such as SSL. Any other standard web-based submittal from a form or otherwise (as far as I’m aware) can be sniffed.
Your code can be as perfect as necessary, but if the network enabling the code is not secure, any code can be easily circumvented.
…for the record, I hate magic_quotes_gpc. When you do your own testing, it adds a lot of code to test if the server is running magic_quotes_gpc or not (I write open source multi-server scripts).
rick_g22 December 6, 2004 at 5:35 pm: That’s a DOUBLE security hole. First, it’s subject to SQL injection attacks. Second, MAN, don’t you _EVER_ use server authentication! That’s the worst thing you can EVER do! The password can be sniffed right away, and if you’re using digest authentication, it’s subject to reply attacks. I’d toss that thing into the garbage. Use instead session + encrypted based logins, like Yahoo which uses MD5 to encrypt the passwords. You can strenghten it as much as you want, like MD5(MD5(seed+password)) or MD5(MD5(seed)+MD5(password)) or SHA1(etc…)
My adaptation also has IP-based session validation. There should be a Sitepoint “build your own secure login using PHP+Javascript” article, you know.
Victor Rafael Rivarola December 6, 2004 at 4:55 pm: You should probably stick with PEAR, as that is the direction PHP is going.
Myself, I chosed ADODB (http://adodb.sourceforge.net). It is a pretty good abstraction layer, but if I had to make the choice now, I would choose PEAR.
craig34 July 8, 2004 at 9:06 am: What is a good database abstraction layer to use? I’ve heard mention of the PEAR DB layer, any others I should look at?
Also, any good tutorials on how to use one of these?
siteguru July 8, 2004 at 3:21 am: This will be a never-ending saga. As PHP becomes ever more popular then there will always be beginners, thus ever more people to be educated.
All we can do is keep on repeating the mantra – check your data before you use it in a SQL query; and this applies REGARDLESS of which server language/database combination is being used.
Anonymous July 6, 2004 at 3:19 pm: if ( $num_rows != 0 ) … if mysql_result isn’t valid for any reason (problem with db connection, users table somehow locked or corrupted …) i expected this statement would eval to TRUE, but it doesn’t. Still seems like a shaky way to do the check, tho.
Anyway, off to check on my own server if this quote thing is really a problem …
CT July 5, 2004 at 5:11 pm: I agree with the need for mysql_escape_string(), but even more important is that ALL data from the request be filtered.
Before using mysql_escape_string() I’d recommend something like this so you control what characters you actually accept:
$logins=preg_replace(‘/[^a-zA-Z0-9]/’, ”, $_SERVER['PHP_AUTH_USER']);
$password=preg_replace(‘/[^a-zA-Z0-9\-\_\@\.]/’, ”, $_SERVER['PHP_AUTH_PW']};
Depending on the filter you may not need to escape, but probably do it anyway to catch programmer errors.
biffl July 5, 2004 at 4:27 pm: Also, this check is a bad idea:
if ( $num_rows != 0 ) {$valid = true;}
The check should be: if ($num_rows == 1)
Presumably, the username/password combination will be unique in the database, so a valid query should only ever return a single row.
HarryF July 5, 2004 at 10:56 am: Why always make it mysql specific?
Don’t use mysql_escape_string – use addslashes – then it will apply to any database.
Sadly it’s not so simple. mysql_escape_string can be aware of i18n character sets so think it’s a good choice to use, when you’re writing a MySql only application. But MySQL breaks the SQL specs by escaping with a backslash. Most databases use a single quote to escape other characters. It’s worth exploring what PEAR::DB or ADOdb does to see how to implement cross database “escaping”.
Andreas July 5, 2004 at 6:02 am: people should read:
http://www.php.net/manual/en/security.index.php
-> http://www.php.net/manual/en/security.database.php
WALoeIII July 5, 2004 at 2:14 am: SQL Injection :\
MJC July 5, 2004 at 1:20 am: Polvero, FFS read the mysql_escape_string blurb like he says & secure your website. If I was a blackhat polvero.com would be going down as we speak.
Chris July 4, 2004 at 8:33 pm: Why always make it mysql specific?
Don’t use mysql_escape_string – use addslashes – then it will apply to any database.
$sql = “SELECT * FROM users WHERE logins=’” . addslashes($_SERVER['PHP_AUTH_USER']) . “‘ AND
password=’” . addslashes($_SERVER['PHP_AUTH_PW']) . “‘”;
HarryF July 4, 2004 at 6:07 pm: so which one is it?
You can’t execute multiple SQL statements in a single query (thankfully) with MySQL until 4.1: see here.
Also don’t think (haven’t checked) that magic quotes would have helped at all here – it screens GET, POST and COOKIE – probably not $_SERVER['PHP_AUTH_PW'].
Widow Maker July 4, 2004 at 5:41 pm: I remember a magazine demonstating JavaScript as a valid means of authentication…
Yer, but [I]that[/I] was some time ago, :lol:
My guess is that you should by default, be verifying magic quotes anyway, regardless of using mySqls escape facility :)
I don’t use it, but I know my data going to the database has already been escaped anyway.
Jellybob July 4, 2004 at 11:56 am: so which one is it?
Very possibly both – people may not be able to run more than one query, but they can still set their username to TRUE or something along those lines, and be authenticated automatically.
Andrew-J2000 July 4, 2004 at 5:46 am: Going back some time ago, I remember a magazine demonstating JavaScript as a valid means of authentication…
polvero July 4, 2004 at 2:55 am: means the script is subject to SQL-injection attacks
….
there isn’t any danger of deleting anything (I don’t think).
so which one is it?
Webservant Alec July 3, 2004 at 9:36 pm: Actually, since mysql_query won’t execute more than one query in one call, there isn’t any danger of deleting anything (I don’t think). The vulnerability is that the attacker can appear to be an authenticated user by making the query return one or more rows. You’d do that by e.g. setting PHP_AUTH_USER to “blah’ OR TRUE OR logins=’blahblah”, so that the whole query becomes:
SELECT * FROM users WHERE
logins = ‘blah’ OR TRUE OR logins=’blahblah’ AND
password = ‘whatever’
The “OR TRUE” matches all users, so $num_rows will be non-zero. (Something like that, anyway.)
In a way, the fact that mysql_query() will only execute one query at a time could be seen as an example of how php is inherently more secure than other languages, at least when you’re using mysql.
Kings July 3, 2004 at 7:26 pm: Polvero, the AUTH variables aren’t escaped in the SQL query, which means the script is subject to SQL-injection attacks, e.g. I could delete the whole database, remove tables, change users… I’m sure you get the point :)
polvero July 3, 2004 at 6:29 pm: Spot the problem?
No.
And I’m assuming it’s not a puncuation or parse error… or anything that would cause the script to malfunction, but I don’t see the loop-hole. Does this have to do with leaving register globals on?
I see that it used both isset twice here:
if (isset($_SERVER['PHP_AUTH_USER']) && isset($_SERVER['PHP_AUTH_PW']) ) {
so is the problem here…
if ( $num_rows != 0 ) { $valid = true;
could you perhaps send a request to the browser telling that $num_rows is 1 or 2…or perhaps just send a request saying $valid is true?
How would someone guess that? Or is that just commmon guesswork?
Skunk July 3, 2004 at 6:27 pm: Nasty! This kind of thing is yet another demonstration of why magic quotes are a bad idea: people learn to rely on them, then forget that they don’t apply to other information sources.
Using a DB abstraction layer that handles variable interpolation (and auto-escapes things for you) is far, far more reliable. It’s the approach taken by many other languages, including (you guessed it) Python. I just checked out the PEAR DB abstraction layer and it seems to support this kind of querying as well, which is definitely a good thing.
cagrET July 3, 2004 at 5:54 pm: That is why I always use a db abstraction layer and I can’t make such a mistake even if I want ;) The data is never put into the query string directly by me.
Some examples:
$Db->execute("SELECT * FROM table WHERE id = ?", $id); $sql = "SELECT * FROM users WHERE logins=? AND password=?"; if ($Db->getRow($sql, $_SERVER['PHP_AUTH_PW'], $_SERVER['PHP_AUTH_USER'])) { $valid = true; } // building advanced queries that consists of a few strings that are later joined is also easy, i have a func buildQuery() $sql = "SELECT * FROM users WHERE logins=? AND password=?"; $sql = $Db->buildQuery($sql, $_SERVER['PHP_AUTH_PW'], $_SERVER['PHP_AUTH_USER']); $sql = $sql . $sql2 . $sql3; $Rs =& $Db->query($sql);
All methods like getOne(), getRow(), getCol(), getAssoc(), getAll() support this way of inserting data. You can also quote the data directle with funcs: quote(), quoteLike()