Page 1 of 1

Please criticise my code again :)

Posted: Wed Sep 06, 2006 7:07 pm
by dunc
Ok, I've had a go at rewriting one of my search pages.

Code: Select all

include "dataLib.php";

// *check username is safe*
$username = 0;
$username = check_alphanum($_SESSION['username']);

// *check userlevel is safe*
$userlevel = 0;
$userlevel = check_int($_SESSION['userlevel']);

// *check memberID is safe*
$memberID = 0;
$memberID = check_int($_SESSION['memberID']);

// CONNECT TO DB (Global Function)
$dbConn = 0;
$dbConn = connectToDB();

// *check username is safe*
$commonName = 0;
$commonName = check_string($_POST['commonName']);
$species = 0;
$species = check_string($_POST['species']);
$genus = 0;
$genus = check_string($_POST['genus']);
$categoryID = 0;
$categoryID = check_int($_POST['category']);
$authorID = 0;
$authorID = check_int($_POST['author']);
$category = 0;
$category = check_string(read_cat($categoryID));
$author = 0;
$author = check_string(read_cat($authorID));

$sql = "SELECT * FROM profile WHERE MATCH (commonName, species, genus) AGAINST ('$commonName $species $genus')";

$result = $result = mysql_query($sql, $dbConn) or die (mysql_error());

$numrows = mysql_num_rows($result); // Number of rows returned from above query.

if ($numrows < 1)
{
 echo 'No results were found matching your query: <br />';
 echo 'Common Name: '.$commonName;
}

while($row = mysql_fetch_object($result))
{
 echo '<p>';
 echo 'Common Name: '.$row->commonName.'<br />';
 echo 'Scientific name: '.$row->genus.' '.$row->species.'<br />';
 echo '</p>';
}
then in dataLib.php

Code: Select all

function make_safe($var)
{
   $var = mysql_real_escape_string($var);
   $var = addslashes(trim($var));
   return $var;
}

// *integer checking*
function check_int($var)
{
 if (ctype_digit($var))
 {
  return make_safe($var);
 }
 else
 {
  return NULL;
  session_write_close();
  header("Location: " . $login);
 }
}

// *alpha numeric checking*
function check_alphanum($var)
{
 if (ctype_alnum($var))
 {
  return make_safe($var);
 }
 else
 {
  return NULL;
  session_write_close();
  header("Location: " . $login);
 }
}

// *string checking*
function check_string($var)
{
  if (ctype_alpha($var))
  {
   return make_safe($var);
  }
  else
  {
   return NULL;
   session_write_close();
   header("Location: " . $login);
  }
}
A couple of questions, if you wouldn't mind.

Is the code secure?
Is the code as quick as it can be - including the SQL?

Also.. that search works fine if you put a full name in (i.e. a common name like 'Molly'), but doesn't work if you put 'Mol'. Is there any way I can do a wildcard search using full text indexing?

And, what is the quickest way to bring up a full set of results from a table? Simply SELECT * FROM table?

Regards,
Duncan

Posted: Wed Sep 06, 2006 8:08 pm
by feyd
Could you try coming up with a better, more descriptive (to your code) thread title, please?

Posted: Wed Sep 06, 2006 9:11 pm
by Christopher
After a quick look the first thing that stands out is the duplicate error checking code in each check function. Perhaps a centralized validator would be better.

Posted: Thu Sep 07, 2006 3:00 am
by jmut
- check_int() and all its associates should not really do any mysql_real_escape_string(). After all you might use it elsewhere - you limit yourself to db ...and validating and escaping are distinct activities. This decreased readability as at first I thought you have sql injection assuming that check_sting only checks if string and does no escaping.

- Another thing I would consider is $_SESSION variables double check. They should be more or less considered secure....after all you validate data before using it (putting it in $_SESSION).

- check_* functions kind of cannot reach session_write_close(); part. They return before it.

Posted: Mon Sep 11, 2006 8:44 am
by Mordred

Code: Select all

include "dataLib.php";
Libraries should be included like this:

Code: Select all

require_once 'dataLib.php';

Code: Select all

$username = 0; 
$username = check_alphanum($_SESSION['username']);
Why assign twice, afraid a bit will remain from the previous value? :P

All check_*() functions will raise warnings when passed an unset $_POST['something']. Either check before passing or (prefferably) pass by reference.

Code: Select all

... or die (mysql_error());
This should be left for the development environment, not a wise thing to do on the 'real' server.

Code: Select all

function make_safe($var) 
{ 
   $var = mysql_real_escape_string($var); 
   $var = addslashes(trim($var)); 
   return $var; 
}
Paranoid, are you? Always doing it twice just in case ;)
Remove the addslashes/trim call. Have you actually tested this code? You would have seen right away that it's no good.

Also - make_safe() is bad, listen to jmut. It DOES NOT make a string safe, it merely prepares it (badly at that) for putting in a db query. Safe is a relative concept on what you try to do with a string. IF you were to echo it on the returning page, the string would be quite unsafe.

Scrap all this and start anew. Write "Cross Site Scripting" ten times on the blackboard, and no copy-paste cheating! ;)

EDIT: Not only your SQL is wrong, but it is vulnerable for wildcard inclusion. You don't actually test your code, do you?
EDIT2: On rereading a bit, your SQL is right, and no wildcard inclusion is possible, I' was wrong, sorry.