[Doc] Securing MODs

This forum is now closed as part of retiring phpBB2.
Forum rules
READ: phpBB.com Board-Wide Rules and Regulations

This forum is now closed due to phpBB2.0 being retired.
Nuttzy99
Former Team Member
Posts: 4917
Joined: Fri Aug 03, 2001 7:09 am
Location: the 11th dimension
Contact:

[Doc] Securing MODs

Post by Nuttzy99 » Tue Mar 23, 2004 9:06 pm

Securing MODs
v0.2.0
03/23/04

Security should be everyone's top concern when writing or installing a MOD. The following is a list of things that MOD authors should keep in mind while coding. MOD users can also take a bit of relief in knowing that the MOD Team specifically checks for these issues in any MOD submitted to the phpbb.com MOD database.

The least you need to know:
  • Use in IN_PHPBB to prevent cross-site script hacks
  • Define in $phpbb_root_path to prevent cross-site script hacks
  • htmlspecialchars and intval can prevent SQL injection and malicious HTML
  • str_replace single quotes with 2 single quotes in SQL
IN_PHPBB - This constant is set to prevent cross-site script hacks that could potentially exploit the database. User facing files (files intended to be called by the user like index.php and viewtopic.php) define this constant. Non-user facing files (not intended to be called by users, such as db.php and common.php) check to make sure this constant is set thereby disallowing direct execution of these files. IN_PHPBB should be the first thing handled in most files. MODs must...
  1. Define IN_PHPBB in user facing files using this syntax:

    Code: Select all

    define('IN_PHPBB', true);
  2. Check for IN_PHPBB in non-user facing files with this code:

    Code: Select all

    if ( !defined('IN_PHPBB') )
    {
    	die("Hacking attempt");
    }
...technically, the check is not required in non-user facing files that contain ONLY functions (like functions.php). If any global variables, requires(), constant definitions, or code outside of a function is present then checking for IN_PHPBB is absolutely required.



$phpbb_root_path - this variable sets the root path of phpBB (the directory containing config.php) relative to where this file is located. For example, a file in the same directory as config.php would use ./ while ./../ would be used for a file in the admin directory. The effect is to prevent cross-site scripts from using files of similar name on a different server to gain access to your DB. MODs must...
  1. the root path should always begin with ./ to ensure execution on the intended server
  2. define $phpbb_root_path in any user facing file that includes or requires files. Immediately following IN_PHPBB handling, usually the next line of the file will be similar to this (with the appropriate path)...

    Code: Select all

    $phpbb_root_path = './';
  3. do not define $phpbb_root_path in non-user facing files as the variable has already been set appropriately in the user facing caller to this file
  4. when including a file, always use $phpbb_root_path as in this example...

    Code: Select all

    include($phpbb_root_path . 'file_name');
  5. generally you don't need or want to use $phpbb_root_path when calling append_sid
htmlspecialchars and intval - one has to be particularly careful when dealing with user supplied data like that obtained from HTTP_GET_VARS and HTTP_POST_VARS. A user could manipulate data that displays on the screen or gets loaded into the DB. Both things could lead to a board being compromised. htmlspecialchars will eliminate any attempts at injecting undesireable HTML code by stripping out the < and > characters (among others). intval will likewise prevent this by forcing whatever input received to be an integer. It is desireable that these actions be performed as soon as possible to prevent unanticpated problems from arising with your MOD or with MODs that could be added in the future. Unless there is some reason not to, you should use these functions directly at the point of assigning the data to a varibale. Therefore we recommend when dealing with user data...
  1. for strings, use htmlspecialchars...

    Code: Select all

    	$username = ( !empty($HTTP_POST_VARS['username']) ) ? htmlspecialchars($HTTP_POST_VARS['username']) : '';
  2. for integers, use intval (or floatval for floats)...

    Code: Select all

    $topic_type = ( !empty($HTTP_POST_VARS['topictype']) ) ? intval($HTTP_POST_VARS['topictype']) : POST_NORMAL;
SQL injection check - For advanced folks, we'll just say: str_replace("\'", "''", $variable_name). For everyone else, you get the full story ;)

SQL injections can pretty much open the entire DB. A user could gain admin privs if he wants or can delete the entire DB if he's feeling particularly vengeful. All string variables used in any sort of SQL query must undergo a quote check. If single quotes (') are left unchecked, then a user could inject text into an SQL statement that can compromise a database. For example, take the following statement where the field some_value is to be updated by the string variable $text....

Code: Select all

INSERT INTO " . SOME_TABLE . " (some_value) VALUES ('" . $text . "')
...assume $text comes from user supplied input. If the user had entered...
user supplied data wrote: '); DROP phpbb_users; some_other_query('
...(NOTE: the offending quotes are in red to make them more clear) then the resulting SQL to be processed will appear as...
INSERT INTO phpbb_sometable (some_value) VALUES (''); DROP phpbb_users; some_other_query('')
...and now you can see the first single quote serves to terminate the intended SQL, keeping it well formatted and valid. Then following the semicolon an SQL command has been inserted that will blow away the users table and this *will* be executed. And the last single quote also serves to just keep the SQL syntax valid.

Fortunately this is easily prevented. For any string variable that will be used in an SQL statement, simply replace occurences of single quotes with two single quotes. However, one must remember that when collecting data from a HTTP_POST_VARS or HTTP_GET_VARS that a \ has automatically been added in front of any single quotes. Therefore, for POST/GET you can defeat SQL injection by issuing this command for each string variable...
the fix for POST/GET wrote: str_replace("\'", "''", $variable_name)
...which changes each instance of \' to '' (two single quotes). For all other methods of obtaining user data, such as reading input from a file, you'll just replace each single quote with two single quotes...
the fix for any other data wrote: str_replace("'", "''", $variable_name)
The two single quotes should be read normally when accessed from the DB and you should not need to do any further manipulation. The two single quotes are the escape value for single quote so they will be displayed as if they were only one single quote.

-Nuttzy :cool:
SpellingCow.com - Free spell check service for your forums or any web form!
No Support via PM please!

wGEric
Former Team Member
Posts: 8805
Joined: Sun Oct 13, 2002 3:01 am
Location: Friday
Name: Eric Faerber
Contact:

Post by wGEric » Mon Aug 16, 2004 5:59 pm

Releasing to public.

This tells you what some security issues are and how to avoid them. Follow these and you won't have any security problems with your MODs. We make sure you use these before we approve your MOD for our Database.
Eric

AnthraX101
Security Consultant
Posts: 497
Joined: Sun Nov 14, 2004 8:05 pm
Contact:

Post by AnthraX101 » Mon Oct 10, 2005 3:53 pm

Nuttzy99 wrote: Securing MODs
SQL injection check - For advanced folks, we'll just say: str_replace("\'", "''", $variable_name). For everyone else, you get the full story ;)

SQL injections can pretty much open the entire DB. A user could gain admin privs if he wants or can delete the entire DB if he's feeling particularly venegeful. All string variables used in any sort of SQL query must undergo a quote check. If single quotes (') are left unchecked, then a user could inject text into an SQL statement that can compromise a database. For example, take the following statement where the field some_value is to be updated by the string variable $text....

Code: Select all

INSERT INTO " . SOME_TABLE . " (some_value) VALUES ('" . $text . "')
...assume $text comes from user supplied input. If the user had entered...
user supplied data wrote:'); DROP phpbb_users; some_other_query('
...(NOTE: the offending quotes are in red to make them more clear) then the resulting SQL to be processed will appear as...
INSERT INTO phpbb_sometable (some_value) VALUES (''); DROP phpbb_users; some_other_query('')
...and now you can see the first single quote serves to terminate the intended SQL, keeping it well formatted and valid. Then following the semicolon an SQL command has been inserted that will blow away the users table and this *will* be executed. And the last single quote also serves to just keep the SQL syntax valid.

Fortunately this is easily prevented. For any string variable that will be used in an SQL statement, simply replace occurences of single quotes with two single quotes. However, one must remember that when collecting data from a HTTP_POST_VARS or HTTP_GET_VARS that a \ has automatically been added in front of any single quotes. Therefore, for POST/GET you can defeat SQL injection by issuing this command for each string variable...
the fix for POST/GET wrote: str_replace("\'", "''", $variable_name)
...which changes each instance of \' to '' (two single quotes). For all other methods of obtaining user data, such as reading input from a file, you'll just replace each single quote with two single quotes...
the fix for any other data wrote: str_replace("'", "''", $variable_name)
The two single quotes should be read normally when accessed from the DB and you should not need to do any further manipulation. The two single quotes are the escape value for single quote so they will be displayed as if they were only one single quote.

-Nuttzy :cool:


Please note that these directions are not complete and may cause a security issue on any mod using them.

The only arrays that you can trust to use in this manner are $HTTP_GET_VARS, $HTTP_POST_VARS, and $HTTP_COOKIE_VARS. If you use any other built-in array, such as $HTTP_SERVER_VARS or $HTTP_ENV_VARS you must run the variable through addslashes() before using str_replace(). It is also recommended that you check the value of magic_quotes and its effect on your variable before using addslashes(), as you may double escape. This is not a security issue in general, but may be a style problem.

AnthraX101

wGEric
Former Team Member
Posts: 8805
Joined: Sun Oct 13, 2002 3:01 am
Location: Friday
Name: Eric Faerber
Contact:

Post by wGEric » Mon Oct 10, 2005 9:10 pm

With the magic_quotes (this is in common.php):

Code: Select all

//
// addslashes to vars if magic_quotes_gpc is off
// this is a security precaution to prevent someone
// trying to break out of a SQL statement.
//
if( !get_magic_quotes_gpc() )
{
	if( is_array($HTTP_GET_VARS) )
	{
		while( list($k, $v) = each($HTTP_GET_VARS) )
		{
			if( is_array($HTTP_GET_VARS[$k]) )
			{
				while( list($k2, $v2) = each($HTTP_GET_VARS[$k]) )
				{
					$HTTP_GET_VARS[$k][$k2] = addslashes($v2);
				}
				@reset($HTTP_GET_VARS[$k]);
			}
			else
			{
				$HTTP_GET_VARS[$k] = addslashes($v);
			}
		}
		@reset($HTTP_GET_VARS);
	}

	if( is_array($HTTP_POST_VARS) )
	{
		...
	}

	if( is_array($HTTP_COOKIE_VARS) )
	{
		...
	}
}
So if you use $HTTP_GET_VARS, $HTTP_POST_VARS and $HTTP_COOKIE_VARS you don't need to worry as much about running variables through addslashes.
Eric

AnthraX101
Security Consultant
Posts: 497
Joined: Sun Nov 14, 2004 8:05 pm
Contact:

Post by AnthraX101 » Mon Oct 10, 2005 10:21 pm

wGEric wrote: So if you use $HTTP_GET_VARS, $HTTP_POST_VARS and $HTTP_COOKIE_VARS you don't need to worry as much about running variables through addslashes.


Correct, that is why I said if you use any other built in array addslashes must be done. At least one mod has gotten bitten by this by using an array other then one of these.

AnthraX101

asinshesq
Registered User
Posts: 6266
Joined: Sun Feb 22, 2004 9:34 pm
Location: NYC
Name: Alan

Post by asinshesq » Sun Nov 06, 2005 5:02 am

Should we turn magic_quotes_gpc on or off if we have the capability to do either?

DKing
Registered User
Posts: 751
Joined: Sat Jul 03, 2004 8:38 pm

Post by DKing » Sun Nov 06, 2005 11:54 am

Shouldn't you use mysql_real_escape_string() for any user-manipulated variables used in any query?
-DKing
Latest phpBB Version: 2.0.21
Search For a MOD: MOD Search

markus_petrux
Former Team Member
Posts: 1887
Joined: Wed Apr 23, 2003 7:11 am
Location: Girona, Catalunya (Spain)
Contact:

Post by markus_petrux » Sun Nov 06, 2005 11:58 am

asinshesq wrote: Should we turn magic_quotes_gpc on or off if we have the capability to do either?

No matter how you set this to run a phpBB board, the code in common.php will make sure PHP globals are slashed.
EasyMOD Standards | MOD Template Actions | MODs in Development Rules
Useful information for MOD Authors | MOD Queue Stats | Search MODs
Write SQL/DDL portable to all SQL servers supported by phpBB!
Get EasyMOD 0.3.0! | Suport al phpBB en Català!
8)

markus_petrux
Former Team Member
Posts: 1887
Joined: Wed Apr 23, 2003 7:11 am
Location: Girona, Catalunya (Spain)
Contact:

Post by markus_petrux » Sun Nov 06, 2005 12:03 pm

DKing wrote: Shouldn't you use mysql_real_escape_string() for any user-manipulated variables used in any query?

Olympus integrates that kind of functions in the DBAL, so it will ensure compatibility with all SQL servers supported. But now, if you do this, then your MOD would only work on MySQL.

I believe str_replace to deal with single quotes is enough for the SQL servers supoprted by phpBB 2.0.x, as far as SQL strings are single quoted as well.
EasyMOD Standards | MOD Template Actions | MODs in Development Rules
Useful information for MOD Authors | MOD Queue Stats | Search MODs
Write SQL/DDL portable to all SQL servers supported by phpBB!
Get EasyMOD 0.3.0! | Suport al phpBB en Català!
8)

asinshesq
Registered User
Posts: 6266
Joined: Sun Feb 22, 2004 9:34 pm
Location: NYC
Name: Alan

Post by asinshesq » Sun Nov 06, 2005 12:07 pm

markus_petrux wrote:
asinshesq wrote:Should we turn magic_quotes_gpc on or off if we have the capability to do either?

No matter how you set this to run a phpBB board, the code in common.php will make sure PHP globals are slashed.

So it's simply a performance issue...I have now tried it both ways and it seems like turning them on does not noticeably impact page generation time.

sssphpbb
Registered User
Posts: 127
Joined: Wed Jun 16, 2004 7:37 pm

Re: [Doc] Securing MODs

Post by sssphpbb » Tue Oct 17, 2006 4:56 pm

Nuttzy99 wrote: All string variables used in any sort of SQL query must undergo a quote check. If single quotes (') are left unchecked, then a user could inject text into an SQL statement that can compromise a database.

What about double quotes -- are they a security risk? If so, what's the code to secure them?

asinshesq
Registered User
Posts: 6266
Joined: Sun Feb 22, 2004 9:34 pm
Location: NYC
Name: Alan

Re: [Doc] Securing MODs

Post by asinshesq » Tue Oct 17, 2006 5:18 pm

sssphpbb wrote:
Nuttzy99 wrote:All string variables used in any sort of SQL query must undergo a quote check. If single quotes (') are left unchecked, then a user could inject text into an SQL statement that can compromise a database.

What about double quotes -- are they a security risk? If so, what's the code to secure them?


Yes, depending on how a sql is written, double quotes can present an injection risk. Using htmlspecialchars as nutzzy describes above deals with that risk, since htmlspecialchars changes a double quote to a &quot (which does not present an injection risk). Htmlspecialchars does not do anything to single quotes (unless a non-default setting is used), and for that reason you need to do an extra str_replace for single quotes as described in nutzzy's post to deal with the single quote injection risk.

Gurus, feel free to correct me or elaborate if I'm wrong or incomplete (which is entirely possible).

sssphpbb
Registered User
Posts: 127
Joined: Wed Jun 16, 2004 7:37 pm

Post by sssphpbb » Tue Oct 17, 2006 6:30 pm

Thanks.

I think this topic should be elevated to Sticky status -- important info for Modder's.

asinshesq
Registered User
Posts: 6266
Joined: Sun Feb 22, 2004 9:34 pm
Location: NYC
Name: Alan

Post by asinshesq » Tue Oct 17, 2006 6:37 pm

sssphpbb wrote: ...I think this topic should be elevated to Sticky status -- important info for Modder's.


I agree that it should be more visible, though I don't know whether that means a sticky in mod authors discussion area (which is an area many mod authors never look at) or some knowledge base article in an obvious place.

Note that these security points are things that the mod validators check for, so if an author forgets about one or more of these points, the validators are supposed to pick the error up (mod validation is a kind of seal of approval on the security front). From my experience, the validatorys seem very careful on these points.

asinshesq
Registered User
Posts: 6266
Joined: Sun Feb 22, 2004 9:34 pm
Location: NYC
Name: Alan

Post by asinshesq » Tue Oct 17, 2006 7:48 pm

asinshesq wrote: I agree that it should be more visible, though I don't know whether that means a sticky in mod authors discussion area (which is an area many mod authors never look at) or some knowledge base article in an obvious place....

I suggest adding a security article to this page: http://www.phpbb.com/kb/list_articles.php?category_id=7
That's a page you get to when you click mods at the top of the screen and then click 'documentation'.

Post Reply

Return to “[2.0.x] MOD Writers Discussion”