My Code isn't secure... help?

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
Termina
Forum Newbie
Posts: 18
Joined: Sat Apr 10, 2004 11:17 pm

My Code isn't secure... help?

Post by Termina »

It's been awhile since I've used this script, but IIRC, this script allows a person to inject queries into mysql.

I'm wondering how I would go about fixing it. I imagine I'd check to see the users input, and check to make sure they arn't using non-alphanumeric charachters (& ; $, etc.).

I'm not sure how to do this though. Could you please review my code, tell me what parts need to be revamped, and how?

Thanks!

Code: Select all

<?

   // this adds the html structure of the site
   include("layout.php");

   /* desc: checks the database for duplicate account names
      ret: duplicate if found
      ret: false if no duplicates */
   function check_duplicates($link, $name, $email) {
     /* select all names, and emails from the account database
        that equal the values the user wants to use */
     $query = "SELECT accname, email FROM accounts
               WHERE accname = '$name' OR email = '$email'";

     $result = mysql_query($query, $link) or die("No accounts to query");

     // if no accounts are returned there are no duplicates
     if (mysql_num_rows($result) == 0) return FALSE;

     // find the duplicate by searching fields
     $duplicates = mysql_fetch_array($result, MYSQL_ASSOC);

     // were done with recordset so close it
     mysql_free_result($result);

     /* search the duplicates. uses foreach on the event that duplicates
        turns up more than one record (i.e. duplicate email in one record
        and a duplicate name in another) */
     foreach ($duplicates as $value) {

       // returns the duplicate if a match is found
       if ($value == $name)
          return $name;
       elseif ($value == $email)
          return $email;
     }
   }

   // desc: outputs an error the the browser
   function generate_error($error) {

     echo('<center>Sorry! There was an error...<br/><hr/>');
     echo($error);
     echo('<form action="'.$_SERVER["SCRIPT_NAME"].'" method="post">');
     echo('<input type="submit" value="<< Back" />');
     echo('</form></center>');

     // kill the script so it will not continue and generate the form
     die();
   }

   // desc: outputs a custom message after a successful execution
   function report_success($arg) {

     echo(str_replace("#name#", $arg, file_get_contents("success_join.txt")));

     // kill the script so it will not continue and generate the form
     die();
   }

   function add_page_content() {
    /* include your script here and it will be added
       to the content cell in the html layout */

    $cmd = ( empty($_POST["method"]) ? "" : trim($_POST["method"]) );

    // if we get this value then execute the script!
    if ($cmd == "execute") {

       // grab our form data
       if (!empty($_POST["account"]))  $name     = trim($_POST["account"]);
       if (!empty($_POST["email"]))    $email    = trim($_POST["email"]);
       if (!empty($_POST["password"])) $password = trim($_POST["password"]);

       /* check that the data send is all valid otherwise generate
          an error to the user */
       if (empty($name)) {
         generate_error("You forgot to specify an account name.");
       }
       elseif (empty($password)) {
         generate_error("You forgot to specify a password.");
       }
       elseif (empty($email)) {
         generate_error("You forgot to specify an email.");
       }
       elseif (strpos($email, "@") === false) {
         generate_error($email." is not a valid email. Please use a valid one, we may need
                        to contact you with it.");
       }

       // encrypt the player's password
       $str = "/home/will/openserver2/encrypt ".$password;
       $encrpass = exec($str, $ret_val);

       // connect to the account database
       $link = mysql_connect("localhost", "root", "password")
               or die("Could not connect to database");

       mysql_select_db("openserver", $link)
               or die("Could not select database");

       if (($ret = check_duplicates($link, $name, $email)) == false) {

         // if no duplicates were found we can add the new account information
         $query = "INSERT INTO accounts (accname, encrpass, email)
                   VALUES ('$name', '$encrpass', '$email')";

         mysql_query($query, $link) or die("Unable to insert account data.");

         // clean up our resources
         
	//Causes problems, commenting out for now
	//mysql_free_result($result);
         mysql_close($link);

         report_success($name);
       }
       else 
         generate_error($ret." already is in use, please use a different one.");
    }

    // default account sign-up form generated below ?>
<center>
<body bgcolor="#000000">
<body text="#33CC00">
<form action="<?= $_SERVER["SCRIPT_NAME"] ?>" method="post">
<input type="hidden" name="method" value="execute" />
<table cellpadding="3" cellspacing="0">
<tr><td colspan="2"><B>By joining, you agree to the Terms and Conditions of this site</B></td></tr>
<tr><td>Name :</td><td><input type="text" name="account" /></td></tr>
<tr><td>Password :</td><td><input type="password" name="password" /></td></tr>
<tr><td>Email :</td><td><input type="text" name="email" /></td></tr>
<tr><td colspan="2"><input type="submit" value="Add my Account!" /></td></tr>
</table>
</form>
</center>
<? } // end of join.php content script ?>
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

the $_POST data isn't really sanitized before being passed to SQL, like you said. mysql_real_escape_string() can help, but also passing the individual pieces of data through a sanitizer of sorts helps. This can be as simple as making sure something that's supposed to be a number in the range of x to y, is infact in that range.. to more complex things such as using regular expressions on certain strings when they are supposed to be a certain format..

Things of this nature.. we've discussed these sorts of things many times in this board.
Termina
Forum Newbie
Posts: 18
Joined: Sat Apr 10, 2004 11:17 pm

Post by Termina »

The problem is that I'm not very good with PHP. ^_^ I am trying to learn though.

So what I understand about mysql_real_escape_string is that it strips certain characters.

So changing my code to look like (snippet):

Code: Select all

elseif (strpos($email, "@") === false) {
         generate_error($email." is not a valid email. Please use a valid one, we may need
                        to contact you with it.");
       }
function quote_smart($password)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $password = stripslashes($password);
   }
   if (!is_numeric($password)) {
       $password = "'" . mysql_real_escape_string($password) . "'";
   }
   return $password;
}
   
function quote_smart($email)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $email = stripslashes($email);
   }
   if (!is_numeric($email)) {
       $email = "'" . mysql_real_escape_string($email) . "'";
   }
   return $email;
}
   
function quote_smart($account)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $account = stripslashes($account);
   }
   if (!is_numeric($account)) {
       $account = "'" . mysql_real_escape_string($value) . "'";
   }
   return $account;
}

      // encrypt the player's password
       $str = "/home/will/openserver2/encrypt ".$password;
       $encrpass = exec($str, $ret_val);
Basically just pass each of my variables through this:

Code: Select all

function quote_smart($value)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $value = stripslashes($value);
   }
   // Quote if not integer
   if (!is_numeric($value)) {
       $value = "'" . mysql_real_escape_string($value) . "'";
   }
   return $value;
}
I have no idea what you mean by
he $_POST data isn't really sanitized before being passed to SQL
, unless that's what I did? *shrugs*

Could you give me any other nifty things to use, similar to mysql_real_escape_string that I should use? Or links to topics that covered this?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

for the most part, I'd read all topics in this "security" board. It's only 2 pages of threads right now.. and the post rate in here isn't very high, often... so you shouldn't have too much trouble catching up on the issues discussed herein.

There are small threads in "PHP - Code" every now and then. Read up on "SQL Injection".

The basics: don't trust jack crud from a user. Validate and verify everything. Including the stuff you don't think is important. Get in the mindset that every piece of data from a user is important, and inherently tainted until you clean it.

Searching all the boards for mysql_real_escape_string(), and it's sibling, mysql_escape_string() may also yield several threads of interest about sanitization.
Post Reply