Spot the Security Hole

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?

Win an Annual Membership to Learnable,

SitePoint's Learning Platform

  • http://gosu.pl cagrET

    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()

  • http://simon.incutio.com/ Skunk

    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.

  • http://www.dustindiaz.com polvero

    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?

  • http://www.nocertainty.com Kings

    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 :)

  • http://alec.bohemiandrive.com Webservant Alec

    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.

  • http://www.dustindiaz.com polvero

    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?

  • http://www.ajohnstone.com Andrew-J2000

    Going back some time ago, I remember a magazine demonstating JavaScript as a valid means of authentication…

  • Jellybob

    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.

  • http://www.sample.com Widow Maker

    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.

  • http://www.phppatterns.com HarryF

    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'].

  • Chris

    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']) . “‘”;

  • MJC

    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.

  • http://www.andrewloe.com/ WALoeIII

    SQL Injection :

  • Andreas
  • http://www.phppatterns.com HarryF

    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”.

  • biffl

    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.

  • CT

    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.

  • Anonymous

    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 …

  • http://www.siteguru.co.uk siteguru

    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.

  • craig34

    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?

  • Victor Rafael Rivarola

    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.

  • rick_g22

    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.

  • malikyte

    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).

  • Charlie

    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??

  • http://redmelon.net/ DougBTX

    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

  • Anonymous

    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.