Please review my code, as my site has been sabatoged

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Please review my code, as my site has been sabatoged

Post by draco2317 »

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]


This is my form processer for one of my registration forms for my model site.  I have been hacked/sabataged or whatever people want to call it.  And I believe this page is one of two pages whic might be causing the problem.  This code is supposed to be called when the form is processed instead another page is instead.  I was wondering how weak and ineffient this code is, and what I could do to improve it.

Code: Select all

<?
include("include/dbconnect.php");

$exista=@mysql_query("select * from members where user='$user' or email='$email'");


if (mysql_num_rows($exista) > 0)
{
header("location:subscribe_member.php?name=$name&email=$email&".
"address=$address&city=$city&country=$country&ptype=$ptype&".
"website=$website&experience=$experience&equipment=$equipment&".
"user=$user&pass=$pass&mesaj=".urlencode("Please select another ".
"username. The username or email you selected is allready taken"));

} else
{


mysql_query ("insert into members (name, email, address, city, country, user, pass, date_of_subscription, photo_type,
experience, equipment, status, subscription_expires_on, last_ip, website) values ('$name','$email', '$address', '$city', '$country', '$user', '$pass', CURRENT_DATE, '$ptype', '$experience', '$equipment', 'new', CURRENT_DATE, '$REMOTE_ADDR', '$website')");

}



include("header.php");



?>
<form name="member_join" action="subscribe_member2.php" method="post">
<table width="244" border="0" cellspacing="0" cellpadding="0">
              <tr>
                <td colspan="3"><img src="gfx/head_register.gif" width="492" height="48"></td>
              </tr>
              <tr>
                <td width="1" background="gfx/thead_unten.gif"><img src="gfx/thead_unten.gif" width="1" height="1"></td>
                <td width="490"><table width="490" border="0" cellspacing="5" cellpadding="0">
                    <tr>
                      <td class="BOD"><strong>
					  Thank you !.<br>
					  We have recived your data and saved it into our database. <br>
					  You will be informed if your subscription will be accepted when our operator will review your information.
					  Have patience, do not subscribe several times, you may end up beeing banned.
					  </strong><br>
 <br>





                          <br>
                        <br>
                        <br>
                      </td>
                    </tr>
                  </table></td>
                <td width="1" background="gfx/thead_unten.gif"><img src="gfx/thead_unten.gif" width="1" height="1"></td>
              </tr>
              <tr background="gfx/thead_unten.gif"> 
                <td colspan="3"><img src="gfx/thead_unten.gif" width="492" height="1"></td>
              </tr>
            </table>
			</form>

<?include("footer.php");?>

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

Well, to start with your code is vulnerable to sql injection. There is no data validation and escaping at all.

So i would start by making sure ALL data is being validated/filtered and then use a db specific escaping function (like mysql_real_escape_string() for mysql) to escape the data you use in the queries.
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

I forgot to meantion that the code was being validated with a javascript, but i would like to either replace or integrate with server side validation. Depending on which plan is best. Also what did you mean by escaping?
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]


Here is the validating code

[syntax="html"]<script language="JavaScript">
<!--

function formCheck(formobj){
	//1) Enter name of mandatory fields
	var fieldRequired = Array("name" ,"sex","orientation","phone","marital", "email", "day","month","year", "address", "city", "country", "height","weight","measurements", "hair","eyes","travel","photo_type", "experience", "user", "pass", "agree");


	//2) Enter field description to appear in the dialog box
	var fieldDescription = Array("Name" ,"Sex","Sexual Orientation","Your phone numbers","Marital Status", "E-Mail","Birth day", "Birth month", "Birth year", "Address", "City", "Country","Height", "Weight","Measurements","Hair","Eyes","Travel or not", "Photo type", "Experience",  "Username", "Password","You must agree to our terms and conditions if you want to join.");
	//3) Enter dialog message
	var alertMsg = "Please complete the following fields:\n";
	
	var l_Msg = alertMsg.length;
	
	for (var i = 0; i < fieldRequired.length; i++){
		var obj = formobj.elements[fieldRequired[i]];
		if (obj){
			switch(obj.type){
			case "select":
				if (obj.selectedIndex == -1 || obj.options[obj.selectedIndex].text == ""){
					alertMsg += " - " + fieldDescription[i] + "\n";
				}
				break;
	//		case "select-multiple":
	//			if (obj.selectedIndex == -1){
	//				alertMsg += " - " + fieldDescription[i] + "\n";
	//			}
	//			break;
			case "text":
			case "textarea":
				if (obj.value == "" || obj.value == null){
					alertMsg += " - " + fieldDescription[i] + "\n";
				}
				break;
			default:
				if (obj.value == "" || obj.value == null){
					alertMsg += " - " + fieldDescription[i] + "\n";
				}
			}
		}
	}

	if (alertMsg.length == l_Msg){
		return true;
	}else{
		alert(alertMsg);
		return false;
	}
}
//-->
</script>

feyd | Please use[/syntax]

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

draco2317 wrote:I forgot to meantion that the code was being validated with a javascript
javascript should only be used for pre-validation. Validation will always need to be done on the server side as well because there is no rule saying that somebody has to use your form to post to your script... and javascript can be turned off.
draco2317 wrote:Depending on which plan is best. Also what did you mean by escaping?
you need to escape data before you can insert it into a database please read about mysql_real_escape_string().
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

if i read that page that ninja suggested right, I should make the following changes

From

Code: Select all

$exista=@mysql_query("select * from models where user='$user' or email='$email'");
To

Code: Select all

$exista=@mysql_query("select * from models where user='$user' or email='$email'",
           mysql_real_escape_string($user),
           mysql_real_escape_string($password));
is that correct or no?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

This isn't a critiquing thread, all the code in this thread should have been in your security thread, ~draco2317. Do not take that as an okay to post it there.

Moved to Security.
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

Where are all the vars that are going into your query being set?
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

Wel, i have the code, however im a bit cautious about posting it after how i did it completely wrong before. Just dont want to upset anyone as this site is too good of a resourse for me to risk getting banned.
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

don't worry... you're not going to get banned for posting code wrong... unless you do it like 100 times. Just wrap your php code in the correct tags (you can use the buttons in the post screen) and you'll be fine. Read the forum rules and follow them and you'll be fine :)
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

feyd Posted: Tue Sep 19, 2006 7:54 am Post subject:

--------------------------------------------------------------------------------

This isn't a critiquing thread, all the code in this thread should have been in your security thread, ~draco2317. Do not take that as an okay to post it there.

Moved to Security.

This is what is making me cautious
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

That was a notice telling you to be watchful over your location of posts. If you want us to review your code for security, post it here. If it is just for the purpose of critiqueing your code, that goes somewhere else (code critique I think).
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

I dont mean to be trying to come across as dificult, my culture is known for not breaking rules. :)
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

Okay, let me reaffirm this to you...

This is what is said about the PHP - Security forum:
Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.
And this is what is said about the Coding Critique forum:
Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.
Popular code excerpts may be moved to "Code Snippets" by the moderators.
That means if you want us to look over your code for Security, you need to post it. We will tear it apart to the best of our ability and give you feedback on the Security of it. Or, if you can explain your position clearly, try without posting code.

If all you want is someone to look over your code, for whatever reason, then that would get posted in Coding Critique. The reason you got 'notified' by feyd was that you posted a thread in this forum, then you posted this thread in another forum before it was moved, by feyd, back into Security. There was really no reason for you to start this thread when we could have carried on in the other thread you posted. This is not meant to call you out or point the finger at you in any way. I am trying to explain what our expectations of you are in this community.
draco2317
Forum Newbie
Posts: 19
Joined: Mon Sep 18, 2006 9:17 pm

Post by draco2317 »

I get it now, thanks for clearing it up
Post Reply