Why is my code not working when trying to prevent Sql injection?

Question

I am a young developer attempting to prevent Sql injections on a site that I didnt originally code myself. I have read many articles about injections and theory behind them but I cannot seem to get this right. Please help me before I put my head through this monitor. Thanks My Monitor

here is the code:

<div id="main">
<div id="header"><? include('site_headergraphics.php'); ?></div>
<table width="800" height="100%" border="0" cellspacing="0" cellpadding="0" 
class="content">
  <tr>
    <td width="800" valign="top">
      <div style="padding:10px;">


  <? if($_SESSION[$_SESSION['SFIX'].'_owner_id'] == ''){ ?>
  <br><br>
        <form name="balance_login" method="post" action="<? if ($_GET['passthru']){    
?>?passthru=<?= $_GET['passthru'] ?><? } else { ?>?<? } ?>">
      <input name="action" type="hidden" value="login" />
      <table width="780" border="0" cellpadding="5" bgcolor="#FFFFFF">
        <tr>
          <td><table width="780" border="0" cellpadding="5" cellspacing="0" 
bgcolor="#F7C30F" class="body_text">
            <tr>
              <td colspan="3" align="left" class="rightmenu"><?= $passmessage ?><?= 
$specialmessage ?></td>
                </tr>
            <tr>
              <td colspan="2" align="left" valign="top" class="rightmenu">To sign on, 
enter your information below.</td>
                  <td width="412" rowspan="5" valign="top"><p>Welcome to Mediterranean 
Wellness!</p>
                  <p>Ready to join us?  You can <a 
href="index.php?section=payment">GET STARTED here. </a></p></td>
            </tr>
            <tr valign="top">
              <td width="121" align="right" class="rightmenu">Username:</td>
                  <td width="217"><input name="username" type="text" class="body_text" 
id="username" /></td>

                  <!--Added in the prevent SQL code injections-->
                  <?php /*?><?php unset($FindUser);       
                        if(isset($_POST['username']))
                        {
                            $_POST['username'] = 
trim($_POST['username']);
                            if(preg_match('/^[a-
zA-Z0-9^$.*+\[\]{,}]{1,32}$/u', $_POST['username']))
                                $FindUser = 
$_POST['username'];
                        }
                         ?><?php */?>
                </tr>
            <tr valign="top">
              <td align="right" class="rightmenu">Password:</td>
              <td><input name="password" type="password" class="body_text" 
id="password" /></td>
                <?php /*?><?php unset($FindPass);
                        if(isset($_POST['password']))
                        {
                            $_POST['password'] = 
trim($_POST['password']);
                            if(preg_match('/^[a-
zA-Z0-9^$.*+\[\]{,}]{1,32}$/u', $_POST['password']))
                                $$FindPass = 
$_POST['password'];
                        }
                         ?><?php */?>
                         <!--End of code added in the prevent SQL code injections-->
            </tr>

            <tr valign="top">
              <td>&nbsp;</td>
                  <td><input name="Submit" type="submit" class="body_text" 
value="Login" />

                   <?php 
                    function make_safe($variable) {
                        $variable = 
mysql_real_escape_string(trim($variable));
                        return $variable;
                    }

                    $username = make_safe($_POST['username']);
                    $password = make_safe($_POST['password']);
                    $check = mysql_query("SELECT Username, 
Password, UserLevel FROM Users WHERE Username = '".$username."' and Password = 
'".$password."'");

                    ?>

                  <br>
                  <br>
                  <a href="?section=forgot">Forgot your username or password?</a></td>
                </tr>
            <tr>
              <td>&nbsp;</td>
                  <td colspan="2">&nbsp;</td>
                </tr>
            </table></td>
          </tr>
        </table>
      </form>
    <? } else {  ?>
    <p class="body_text">You must <a href="?user_action=logout">logout</a> to continue 
to this page</p>
    <? } ?>
      </div></td>
    </tr>
</table>
</div>

Where is your `make_safe` function defined? Also it would probably help to remove all the HTML from your code sample for clarity, as it has not relevance to the data sanitizing effort. — Mike Brant, Sep 04 '12 at 21:45
Sidenote: `$$FindPass = $_POST['password'];` should probably be `$FindPass = $_POST['password'];`. And regarding `make_safe` it's in there, way down to the end. — Henrik Ammer, Sep 04 '12 at 21:47
The general idea will be to filter the username and password before using in the query. You might be doing this with make_safe(), but we can't see. Even better, do this verification as the user's entering it so you can provide help with the correct format. Also, checkout PDO: http://php.net/manual/en/intro.pdo.php — HappyTimeGopher, Sep 04 '12 at 21:48
`but I cannot seem to get this right` are you serious ?? What am I suppose to do with that statement ? Please tell us more about the problem!! are you getting any error ? — Deepak, Sep 04 '12 at 21:50
Side note: this is the proper way to do it: http://stackoverflow.com/a/60496/1329367 — Mahn, Sep 04 '12 at 21:50

score 1 · Answer 1 · answered Sep 04 '12 at 21:49

1

I think the best way to avoid SQL injections is to use prepared statements when you query the database.

Also you should clean up your code to make it better readable, and using design patterns like MVC would give it all a better structure.

answered Sep 04 '12 at 21:49

tbraun89

2,246
3
25
44

1

See also http://bobby-tables.com/php.html for a crash course in the PHP way to avoid SQL injection. – Andy Lester Sep 04 '12 at 21:59

score -1 · Answer 2 · edited May 21 '14 at 22:32

-1

mysql_* is being deprecated and won't work in newer version- you should checkout PDO or MySQLi instead.

Specifically the bind_param() function- this is how you should be handling user input now (mysql_real_escape_String() isn't perfect)

That said, you are never connecting to the database in your code- without an active DB connection mysql_real_escape_string() will not work

edited May 21 '14 at 22:32

BenMorel

34,448
50
182
322

answered Sep 04 '12 at 21:51

msEmmaMays

1,073
7
7

1

`mysql_* is being depricated and won't work in newer version` which version did it not work ? – Deepak Sep 04 '12 at 21:54
@Deepak in the newer versions it will not but for now it is still working – rsz Sep 04 '12 at 22:06
@rsz Yes i agree with u!! From his answer it looks like it is not working in newer versions... Should have be said as future versions!! – Deepak Sep 04 '12 at 22:08
What exactly does "`mysql_real_escape_String()` isn't perfect" mean? Which cases are not covered? – glglgl Sep 04 '12 at 22:15
UTF8 characters OR if you forget quotes - mysql_query("select * from user where id=".mysql_real_escape_string($_GET[id])); could still be injected (http://stackoverflow.com/questions/2353666/php-is-mysql-real-escape-string-sufficient-for-cleaning-user-input) PDO/MySQLI puts the quotes around strings for you - you can't forget them. – msEmmaMays Sep 04 '12 at 22:32
for UTF8 - mysql_real_escape_String() doesn't know what character set your Mysql Connection is using (if you aren't using latin) - so it won't escape all the characters it should - http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html – msEmmaMays Sep 04 '12 at 22:36

Why is my code not working when trying to prevent Sql injection?

2 Answers2