Application hacked and insecure

[andibakker]

Hi, our application has been hacked. Please give me help on how to make this code more secure.

The input comes from a form (similar to the newthread form that I'm currently typing this message in), where the user can use the editor to insert bold / italics / hyperlinks etc.

Below is the code :

if(isset($_REQUEST['Submit']))
{
	$title=addslashes($_REQUEST['name']);
	$desc=addslashes($_REQUEST['desc']);
	$mtitle=addslashes($_REQUEST['metatitle']);	
	$mkeys=addslashes($_REQUEST['mkeys']);	
	$mdesc=addslashes($_REQUEST['mdesc']);	

if ((isset($_GET['lid'])) && (is_numeric($_GET['lid'])))
{  
  $lid = mysql_real_escape_string($_GET['lid']);
  $query="update news 	
							 SET	name='$title',  description='$desc' , meta_title='$mtitle' , meta_tags='$mkeys', meta_desc='$mdesc' 
							 where newsid=".$lid;
mysql_query($query);

and the form looks like this:

<td height="575" align="center" valign="top"><form action="editnews.php?lid=<?php echo $_GET['lid'];?>" method="post" enctype="multipart/form-data" name="form1" id="form1">
              <p>&nbsp;</p>
              <table width="595" border="1" cellpadding="0" cellspacing="0" >
	 <tr>
	 <td align="center"><table width="528" border="0" cellspacing="0" cellpadding="0">
        <tr>
          <th colspan="3" scope="col"   align="left" class="pageHeading" ><img src="images/ic_order.gif" alt="a" width="16" height="16"/> Edit News </th>
          </tr>

	    <tr>
      <td colspan="3" align="center" class="menutext3" scope="row">&nbsp;</th>
        <?php echo $msg;?></tr>
    <tr>
      <td width="153" align="left" class="td_borders maintext" scope="row"> Title </td>
      <td width="375" colspan="2" align="left" class="td_borders maintext"><label>
        <input name="name" type="text" id="name" value="<?php echo $admin->name;?>" size="40"/>
      </label></td>
    </tr>
    <tr>
      <td align="left" class="td_borders maintext" scope="row">Description </td>
      <td colspan="2" align="left" class="td_borders maintext"><label>
        <textarea name="desc" id="desc"><?php echo $admin->description;?></textarea>
      </label></td>
    </tr>

	<tr>
      <td align="left" class="td_borders maintext" scope="row">Meta title </td>
      <td colspan="2" align="left" class="td_borders maintext"><label>
        <input name="metatitle" type="text" id="metatitle"  value="<?php echo $admin->meta_title;?>" size="50"/>
      </label></td>
    </tr>

    <tr>
      <td align="left" class="td_borders maintext" scope="row">Meta keys </td>
      <td colspan="2" align="left" class="td_borders maintext"><label>
        <input name="mkeys" type="text" id="mkeys" size="50" value="<?php echo $admin->meta_tags;?>" />
      </label></td>
    </tr>
    <tr>
      <td align="left" class="td_borders maintext" scope="row">Meta Description </td>
      <td colspan="2" align="left" class="td_borders maintext"><label>
        <textarea name="mdesc" id="mdesc"><?php echo $admin->meta_desc;?></textarea>
      </label></td>
    </tr>
    <tr>
      <th colspan="3" scope="row">&nbsp;</th>
      </tr>
    <tr>
      <th colspan="3" scope="row"><label>
        <input type="submit" name="Submit" value="Update News" onclick="return validate();"/>
      </label>
        <label></label></th>
      </tr>
  </table></td>
 </tr></table>
        </form></td>

[laserlight]

I might write it this way:

if (isset($_REQUEST['Submit'], $_REQUEST['name'], $_REQUEST['desc'],
    $_REQUEST['metatitle'], $_REQUEST['mkeys'], $_REQUEST['mdesc']))
{
    if (isset($_GET['lid']) && is_numeric($_GET['lid']))
    {
        $query = sprintf("UPDATE news
            SET name='%s', description='%s', meta_title='%s', meta_tags='%s', meta_desc='%s'
            WHERE newsid=%d",
            mysql_real_escape_string($_REQUEST['name']),
            mysql_real_escape_string($_REQUEST['desc']),
            mysql_real_escape_string($_REQUEST['metatitle']),
            mysql_real_escape_string($_REQUEST['mkeys']),
            mysql_real_escape_string($_REQUEST['mdesc']),
            $_GET['lid']);
        mysql_query($query);

Notice the use of mysql_real_escape_string instead of addslashes. You might want to switch to the PDO extension or MySQLi extension and use prepared statements.

[andibakker]

Thanks for your advice. I'll implement the changes. So mysql_real_escape_String is sufficient ? I don't need to use anything else like htmlentities on the form data ?

[laserlight]

andibakker wrote:

I don't need to use anything else like htmlentities on the form data ?

When you are displaying it, yes, you should use htmlentities.

[andibakker]

But now if I display it in the form using htmlentities - it displays the data like this :

<p style="\"><span style="\">In our forums section you will find a <strong>

How do I use htmlentities to secure the input / display data from the form ?

[bradgrafelman]

andibakker;10953659 wrote:

So mysql_real_escape_String is sufficient ?

It's sufficient for string data, yes; it shouldn't be used for numerical data (hence the "_string" suffix in the function's name). Instead, you should use [man]intval/man for integers, cast the data to the appropriate type (e.g. (float)$var😉, or use sprintf() which forces the parameters to be of whatever type you specify (as laserlight has shown above).