Security Concerns

[Night4554]

I'm concerned about the security of my code. Here are some exceprts of an Open Source Gallery System. (If you're really ambitious or want more background information the site is at http://galant.night4554.com )


$query = "SELECT * FROM " . $tblpre . "galleries WHERE id = '$photo[gal_id]' LIMIT 1";
$temp = mysql_query($query) or err_report($query);

===============================

$query="UPDATE users SET
username = '$params[username]',
pub_mail = '$params[pub_mail]',
priv_mail = '$params[priv_mail]',
aim = '$params[aim]',
yim = '$params[yim]',
msn = '$params[msn]',
icq = '$params[icq]',
website = '$params[website]',
profile = '$params[profile]',
birthday = '$new_birthday',
real_name = '$params[real_name]',
occupation = '$params[occupation]',
interests = '$params[interests]',
address1 = '$params[address1]',
address2 = '$params[address2]',
city = '$params[city]',
state = '$params[state]',
zip = '$params[zip]',
country = '$params[country]'
WHERE id = '$_SESSION[id]'";
$temp=mysql_query($query) or err_report($query);

===============================

if(($_POST[submit])&&(!isset($_POST[type])))
{
$query = "Select id
FROM users
WHERE username = '$_POST[username]'
AND password = md5('$_POST[password]')
LIMIT 1";
$id = sing_result($query);

$query = "Select username
FROM users
WHERE username = '$_POST[username]'
AND id = '$id'
AND password = md5('$_POST[password]')
LIMIT 1";
$user = sing_result($query);

$query = "Select user_level
FROM users
WHERE username = '$user'
AND id = '$id'
AND password = md5('$_POST[password]')
LIMIT 1";
$user_level = sing_result($query);


if(!empty($id)&&($user == $username)&&(isset($user_level)))
{
$mode="logged";
}
}
elseif(($_POST[submit])&&($_POST[type]))
{
$query="SELECT password FROM gallery_galleries WHERE id = '$_POST[gal_id]' LIMIT 1";
$thisgalpass = sing_result($query);
if($thisgalpass == $_POST[password])
{
$mode="galpassed";
}
}



switch($mode)
{
case "logged":
{
session_register("id");
session_register("user_level");

if($_POST[cookie])
{
$loginarray[username]=md5($_POST[username]);
$loginarray[pass]=md5($_POST[password]);
@setcookie("$config[cookie_name]", serialize($loginarray), "$config[cookie_length]",
"$config[cookie_path]", "$config[cookie_domain]");
}

if(isset($_POST[gal]))
{
session_unregister("redirect");
header("Location: view_gallery.php?gal=".$_POST[gal]);
exit;
}
if(!empty($_SESSION['redirect']))
{
$go=$_SESSION['redirect'];
session_unregister("redirect");
header("Location: ".$go);
exit;
}
else
{
header("Location: view_album.php");
exit;
}
break;
}





I know, I know. I have a lot of problems in regard to security. Can you point me at them?

[jweissig]

hi,

You need to escape all your databases reads/writes where you input a field from a users.

Basically here is a small example. Say that you use something like this..

$sql = "select * from users where username = '$user' and password = '$password' limit 1";

where $user and $password are given from $_POST what happens when an attacher puts '' or 1 = 1 as the password? We he would be through all your secureity because he was able to escape your security.. You need to strip out all ' and \ characters.

Also i don't know about this code here..


if(($_POST[submit])&&(!isset($_POST[type])))
{
$query = "Select id
FROM users
WHERE username = '$_POST[username]'
AND password = md5('$_POST[password]')
LIMIT 1";
$id = sing_result($query);

$query = "Select username
FROM users
WHERE username = '$_POST[username]'
AND id = '$id'
AND password = md5('$_POST[password]')
LIMIT 1";
$user = sing_result($query);

$query = "Select user_level
FROM users
WHERE username = '$user'
AND id = '$id'
AND password = md5('$_POST[password]')
LIMIT 1";
$user_level = sing_result($query);


why don't you just use


$sql = "SELECT id, username, user_level
FROM users
WHERE username = '$user'
AND password = md5('$_POST[password]')
LIMIT 1


you should **NEVER** pass $_POST data directly to a SQL command because bad things can happen.. you should ALWAYS clean the code.. check addslashes() and stripslashes() for php they can really help you.

thanks,
- justin

[elskan]

you should **NEVER** pass $_POST data directly to a SQL command because bad things can happen.. you should ALWAYS clean the code.. check addslashes() and stripslashes() for php they can really help you.

What bad things?

I take it passing form data directly into a customer database is very bad then?

[Night4554]

I will need to gthrough all the code and make appropriate changes. Thank You.

As for the extra bits of unnessary code, I put it in there for, I don't know peice of mind.


"I take it passing form data directly into a customer database is very bad then?"

Yes, that's the gist of what he said.

[jweissig]

Hi,

You should try and make as little SQL querys as possibly because these all have to be run on the SQL server. If you have a large site with many users the extra SQL querys are just excess bagage if you could do the together.

Anyways passing $_POST or $_GET data directly into your SQL statments is not a good idea.. for example say that you have an on-line store script that updates a record.

$sql = "update products set price = $_POST[price] where product_id = $_POST[product_id]";

Then in your script you pass the value something like "script.php?price=4.99&prodiuct_id=3941" this all works fine till someone plays with it and does "script.php?price=4.99&prodiuct_id=3941 or product_id < 100000".

This would create a SQL statment something like this

$sql = "update products set price = 4.99 where product_id = 3941 or product_id < 100000";

Now you would have all the price data in the database set to 4.99. It would not be that hard for the attacker to guess the column names as it is pretty common for people to use these column names in the $_POST data. This would get really messy if you where using a delete command. This is just an example but you really want to make sure you are executing the correct commands on the database.

[Merve]

Originally posted by elskan
you should **NEVER** pass $_POST data directly to a SQL command because bad things can happen.. you should ALWAYS clean the code.. check addslashes() and stripslashes() for php they can really help you.

What bad things?

I take it passing form data directly into a customer database is very bad then?

ALWAYS do data scrubbing. Functions like strip_tags() and htmlentities() will save you a helluva lotta headaches.

Also, PCRE is good. Something like:

if(!preg_match("/^[a-zA-z0-9-_.;:,]*$", $string_to_match) {
//do blah
} else {
//do other blah
}


Data scrubbing is very important. There are other ways to data scrub, refer to the manual.

[Night4554]

There are instances where I want to allow HTML tags, I'll get to malicious tags later.

I looked up addslashes and I have magic_quotes_gpc enabled. So I'm going to put in an if statement like thing:


if(!get_magic_quotes_gpc())
{
$query=addslashes($query);
}


Will that cover me from malicious database manipulations?

[drawmack]

with strip tags you can give a list of allowed tags and then it will remove all other tags so you can allow formatting tags like <b><u><br> and such and remove all the malicious tags

http://www.php.net/striptags

[Merve]

Originally posted by drawmack
with strip tags you can give a list of allowed tags and then it will remove all other tags so you can allow formatting tags like <b><u><br> and such and remove all the malicious tags

http://www.php.net/striptags

Yes, but strip_tags() would not remove something like <SCriPT>. Use htmlentities(), then use a regex for tags like <b><u><s><i>, as in converting &amp;gt; and &amp;lt; to > and <.

[drawmack]

or, or, or you can do a regex that converts every tag like item to lowercase and then use strip tags which is a lot easier and a lot quicker too.

[Weedpacket]

Originally posted by drawmack
or, or, or you can do a regex that converts every tag like item to lowercase and then use strip tags which is a lot easier and a lot quicker too. And if your page is XHTML, more correct, too :)

[Merve]

Originally posted by drawmack
or, or, or you can do a regex that converts every tag like item to lowercase and then use strip tags which is a lot easier and a lot quicker too.

Yes, but with my method, you can leave those things in if you'd like; yours is also good for certain things. Your method would remove stuff like <hello>. It all depends on what you want. If you combined to the two methods to make the page XHTML compliant, it would be even better.

[drawmack]

to be xhtml compliant you can have things like <hello> in your text though. I personally find it's better to be strict with user's tell them to use &amp;lt; and &amp;gt; and not <, >. It'll make your life better in the long run.

If they complain about learning curve blame security the average person will believe that anything is because of security.

[Night4554]

In places I don't want html, I'm simply taking all < and > and making them &lt; and &gt;

I'm still trying to think of a way to stop things like <script> while leaving the other HTML alone.

[drawmack]

what you need is a regex that will convert all tags to lower case and then you can use script_tags which can take a list of 'safe' tags as a parameter and it will not touch those tags.

[Merve]

Originally posted by drawmack
to be xhtml compliant you can have things like <hello> in your text though. I personally find it's better to be strict with user's tell them to use &amp;lt; and &amp;gt; and not <, >. It'll make your life better in the long run.

If they complain about learning curve blame security the average person will believe that anything is because of security.

What I mean was using &amp;lt;hello&amp;gt;

[laserlight]

Originally posted by drawmack
or, or, or you can do a regex that converts every tag like item to lowercase and then use strip tags which is a lot easier and a lot quicker too.

hmm... can avoid the regex with strtolower(), methinks.

[drawmack]

Originally posted by laserlight
hmm... can avoid the regex with strtolower(), methinks.

but wouldn't that convert the entire string? You don't want to change the case of the person's input text just their tags so that strip_tags can catch them.

[HalfaBee]

Originally posted by Merve
Yes, but strip_tags() would not remove something like <SCriPT>. Use htmlentities(), then use a regex for tags like <b><u><s><i>, as in converting &amp;gt; and &amp;lt; to > and <.

Strip_tags() will remove <SCript> tags, it is not as dumb as you think it is.

HalfaBee

[Moonglobe]

as far as i know, only as of PHP5beta1

[HalfaBee]

This script

echo "PHPversion = ".phpversion()."<BR>";
$test = "Remove these tags <ScRipt>Javascript in here</ScripT>";
echo "Test string = ".htmlentities( $test )."<br>";
echo "After striptags = ".strip_tags( $test );


displayed this

PHPversion = 4.3.2
Test string = Remove these tags <ScRipt>Javascript in here</ScripT>
After striptags = Remove these tags Javascript in here

HalfaBee

[Merve]

Not everybody's on 4.3.2; my test server's on 4.2.3 and my website uses 4.1.2

Regex is your best bet.