Checking code for SQL injection and XSS.

[codeboy89]

I am really new to PHP, and I want to make sure my coding is secure!!! Any help would be greatly appreciated. This is my first post and visit to PHP Freaks. thanks guys

 

PHP SIDE:

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

<?php

$has_error = 0;

 

if(isset($_POST['go']))

{

$error = array();

if(isset($_POST['usernameid']) && !empty($_POST['usernameid']))

{

$username = mysql_real_escape_string(strip_tags($_POST['usernameid']));

} else {

$error['usernameid'] = "Username not typed";

}

if(isset($_POST['password']) && !empty($_POST['password']))

{

$password = md5($_POST['password']);

} else {

$error['password'] = "Password not typed";

}

$check = mysql_query("SELECT * FROM users WHERE username='$username' AND password='$password'");

if(mysql_num_rows($check) == 0)

{

$error['usernameid'] = "Login invalid";

$error['password'] = "Login invalid";

}

 

if(empty($error))

{

$row = mysql_fetch_array($check);

$user_id = $row['user_id'];

 

session_start();

 

$_SESSION['logged_id'] = $user_id;

$_SESSION['logged_user'] = $username;

$_SESSION['pass'] = $password;

 

header("Location:success.php");

} else {

$has_error = 1;

}

}

?>

 

HTML SIDE:

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

<div>

<form action="<?php echo $_SERVER['PHP_SELF'] ?>" method="post">

<?php

$msg = array("success" => "Account created", "logout" => "Logged out", "notlogged" => "Logged in");

 

$show_message = "";

 

if(isset($_REQUEST['msg']) && !empty($_REQUEST['msg']))

{

$got = $_REQUEST['msg'];

if($got == 'success')

$show_message = $msg['success'];

else if($got == 'logout')

$show_message = $msg['logout'];

else if($got == 'notlogged')

$show_message = $msg['notlogged'];

}

if(!empty($show_message))

{

?>

<?php echo $show_message ?><br />

<?php

}

?>

Username<br />

<input type="text" name="usernameid" /><br />

<?php

if(isset($error['usernameid']) && !empty($error['usernameid']))

{

?>

<?php echo $error['usernameid'] ?><br />

<?php

}

?>

Password</span><br />

<input type="password" name="password" /><br />

<?php

if(isset($error['password']) && !empty($error['password']))

{

?>

<?php echo $error['password'] ?><br />

<?php

}

?>

[Bricktop]

Hi codeboy89,

 

Have a look at Daniel's excellent PHP Secutiy Tutorial which should give you al lthe information you need but looking through your code it all looks pretty secure except you're not using mysql_real_escape_string() on the POSTed password variable.  Change your code to read:

 

if(isset($_POST['password']) && !empty($_POST['password']))
   {
      $password = mysql_real_escape_string($_POST['password']);
      $password = md5($password);

 

As a general rule of thumb as long as you use mysql_real_escape_string() on POSTed date or data retrieved via GET before entering it into the database you should be fine.

 

As covered in the above tutorial, use htmlentities() to protect against XSS attacks.

 

Hope this helps.

[codeboy89]

thanks!

 

not sure what to do with htmlentities()

[Bricktop]

Hi codeboy89,

 

XSS issues are really only relevant if code is being inputted by a user and then outputted back to the screen.

 

Have a look at the tutorial I posted (specifically section 4), in the code you posted I don't think it's particularly relevant.  Therefore, you won't need to use htmlentities().

 

But essentially, what htmlentities() does is turn HTML code into its relevant entity, i.e. < becomes < and > becomes >.  This helps stop users posting dangerous Javascript code directly into your application, which when retrieved back to the screen will be run.

[MadTechie]

Humm.. Why has no one has made a comment about this line

<form action="<?php echo $_SERVER['PHP_SELF'] ?>" method="post">

yet!

 

open your page ie form.php

but open it like this

form.php/"><script>alert('xss attack')</script>

or for fun try

form.php/"><script>window.location='http://www.phpfreaks.com/forums/index.php/topic%2C274636.0.html';</script>

 

Change it to

<form action="#" method="post">

 

[codeboy89]

thanks MadTechie. I have 2 more snippets I would like checked, if you are willing please!

 

REG.PHP

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

<?php

session_start();

$has_error = 0;

 

if(isset($_POST['register']))

{

$error = array();

 

if(isset($_POST['uname']) && !empty($_POST['uname']))

{

$username = $_POST['uname'];

 

if(ctype_alnum($_POST['uname']))

{

if(strlen($_POST['uname']) < 4)

{

$error['uname'] = "Username must have at least 4 chars.";

}

else if(strlen($_POST['uname']) > 15)

{

$error['uname'] = "Username must have under 15 chars.";

} else {

$check = mysql_query("SELECT * FROM users WHERE username='$username'");

 

if(mysql_num_rows($check) > 0)

{

$error['uname'] = "Username taken";

}

}

} else {

$error['uname'] = "Username invalid";

}

} else {

$error['uname'] = "Username not given.";

}

 

if(isset($_POST['password']) && !empty($_POST['password']))

{

$password = $_POST['password'];

 

if(strlen($_POST['password']) < 6)

{

$error['password'] = "Password must have at least 6 chars.";

}

else if(strlen($_POST['password']) > 15)

{

$error['password'] = "Password must have under 15 chars.";

}

} else {

$error['password'] = "Password was not given.";

}

 

if(isset($_POST['re_password']) && !empty($_POST['re_password']))

{

$re_password = $_POST['re_password'];

 

if(strlen($_POST['re_password']) < 6)

{

$error['re_password'] = "Password must have at least 6 chars.";

}

else if(strlen($_POST['re_password']) > 15)

{

$error['re_password'] = "Password must have under 15 chars.";

}

} else {

$error['re_password'] = "Password was not given twice.";

}

 

if($password != $re_password)

{

$error['password'] = "Passwords did not match.";

$error['re_password'] = "Passwords did not match.";

}

 

if($_POST['secure'] != $_SESSION['security_number'])

{

$error['secure'] = "CAPTCHA failed";

}

 

if(empty($error))

{

$insert = mysql_query("INSERT INTO users(`user_id`,`username`,`password`,`joined`) VALUES(0,'".mysql_real_escape_string(strip_tags($username))."','".md5($password)."',CURDATE())");

 

if($insert)

header("Location:index.php?msg=success");

} else {

$has_error = 1;

}

}

?>

 

 

[codeboy89]

MAIN

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

<?php

$character_limit = 100;

session_start();

 

if(isset($_SESSION['logged_id']) && isset($_SESSION['logged_user']) && isset($_SESSION['pass']))

{

$check = mysql_query("SELECT * FROM users WHERE user_id=".$_SESSION['logged_id']." AND username='".$_SESSION['logged_user']."' AND password='".$_SESSION['pass']."'");

 

if(mysql_num_rows($check)==0)

{

header("Location:index.php?msg=notlogged");

}

} else {

header("Location:index.php?msg=notlogged");

}

 

$has_error = 0;

$error = array();

 

if(isset($_POST['save']))

{

 

if(isset($_POST['comment']) && !empty($_POST['comment']))

{

$comment = $_POST['comment'];

 

if(strlen($comment) > $character_limit)

{

$error['comment'] = "Only $character_limit characters.";

}

} else {

$error['comment'] = "At least 1 character needed.";

}

 

if(empty($error))

{

$insert = mysql_query("INSERT INTO comments(`id`,`user_id`,`comment`,`created_at`) VALUES(0,".$_SESSION['logged_id'].",'".mysql_real_escape_string($comment)."','".time()."')");

 

$check_comment = mysql_query("SELECT * FROM comments WHERE user_id=".$_SESSION['logged_id']);

 

if(mysql_num_rows($check_comment) > 0)

{

$update = mysql_query("UPDATE comments SET comment='".mysql_real_escape_string(strip_tags($comment))."',created_at='".time()."' WHERE user_id=".$_SESSION['logged_id']);

 

} else {

 

$insert = mysql_query("INSERT INTO comments(`id`,`user_id`,`comment`,`created_at`) VALUES(0,".$_SESSION['logged_id'].",'".mysql_real_escape_string(strip_tags($comment))."','".time()."')");

}

} else {

$has_error = 1;

}

}

else if(isset($_POST['logout']))

{

header("Location:logout.php");

}

 

$get_comment_result = mysql_query("SELECT * FROM comments WHERE user_id=".$_SESSION['logged_id']." ORDER BY id DESC LIMIT 0,1");

 

if(mysql_num_rows($get_comment_result) > 0)

{

$get_comment_data = mysql_fetch_array($get_comment_result);

$saved_comment = stripslashes($get_comment_data['comment']);

} else {

$saved_comment = "";

}

?>

 

THANKS AGAIN FOR ALL THE HELP! 

[MadTechie]

I would be willing, if you used code tags (#) (pleased edit your posts)

 

a quick look of your 2nd to last post i had a Coughing fit @

$check = mysql_query("SELECT * FROM users WHERE username='$username'");

 

please read

As a general rule of thumb as long as you use mysql_real_escape_string() on POSTed date or data retrieved via GET before entering it into the database you should be fine.

[codeboy89]

so instead of

 

$check = mysql_query("SELECT * FROM users WHERE username='$username'");

 

i need to use

 

$check = mysql_real_escape_string("SELECT * FROM users WHERE username='$username'");

 

correct? i am sorry i am watching my 8month old while i am doing this. i missed it

[MadTechie]

You just escape the variable

ie

$check = "SELECT * FROM users WHERE username='".mysql_real_escape_string($username)."' ";

[codeboy89]

if i amended:

 

$username = mysql_real_escape_string(strip_tags($_POST['uname']));

 

do i still need to do that?

[MadTechie]

That line doesn't exists in REG.PHP (which is what I have been referring to)

 

EDIT: however if you have added that line to REG.PHP then it should be fine

[Bricktop]

if i amended:

 

$username = mysql_real_escape_string(strip_tags($_POST['uname']));

 

do i still need to do that?

 

Hi codeboy89,

 

No, if you have done that (as per my PM) then that's fine.

 

@MadTechie, codeboy89 has also been PM'ing me so I think there's been a bit of overlapping of code etc.

[codeboy89]

sorry about that. thanks for all the help, i learned some new stuff today. thanks again!

[MadTechie]

@MadTechie, codeboy89 has also been PM'ing me so I think there's been a bit of overlapping of code etc.

Ahhh.. okay.. personally (when possible) I try to keep it to the boards as anyone search will also get the solution,

[Bricktop]

Yes, I totally agree, I did suggest that codeboy89 did this for that exact reason, also you get everyone's opinion on the problem not just one person's. 

 

However, he said he didn't want to post his code on the forums, but it turns out he did in the end (as well as PM'ing it to me!)

 

Oh well, at least he got a solution.

 

Thanks

 

[codeboy89]

I used the XSS Me addon, found this was vulnerable on my HTML side. How would I fix this problem?

 

<input type="text" name="uname" class="text" maxlength="15" <?php if($has_error) { ?> value="<?php echo $username ?>" <?php } ?> /><br />

[MadTechie]

I used the XSS Me addon, found this was vulnerable on my HTML side. How would I fix this problem?

 

<input type="text" name="uname" class="text" maxlength="15" <?php if($has_error) { ?> value="<?php echo $username ?>" <?php } ?> /><br />

 

Your need to html encode it,

what htmlentities() does is turn HTML code into its relevant entity, i.e. < becomes < and > becomes >.  This helps stop users posting dangerous Javascript code directly into your application, which when retrieved back to the screen will be run.

 

IE

<?php echo htmlentities($username) ?>

[codeboy89]

it still shows as vulnerable in XSS Me. 

[MadTechie]

you may want to encode the quotes as well

<?php echo htmlentities($username, ENT_QUOTES) ?>

 

Whats the message your getting ?

[codeboy89]

 

if i remove the php code from the username text input, it fixes the issue but i lose my error handling if they do not enter a username or one with less than 4 characters

[codeboy89]

is that what you wanted to know?

[MadTechie]

I don't see any harm with not encoding ; \ ? or =

you could of course wipe them out

ie

<?php echo htmlentities(preg_replace('/[;\\\\?=]/sm', '', $username), ENT_QUOTES); ?> 

but that's a little OTT

[codeboy89]

i like to go over the top, thanks for solving my issue!