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

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 &gt; and &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 &lt; and &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 < and >

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 &lt; and &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 &lt;hello&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 &gt; and &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