Criticise my code!

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
dunc
Forum Newbie
Posts: 13
Joined: Mon Sep 04, 2006 10:17 am

Criticise my code!

Post by dunc »

Hey ladies n gents.

I'm new to PHP.. basically, I've taught myself the language. I'm developing a website for a joint venture with two friends - a fishkeeping information resource.

I have a control panel which allows users to login, then depending on their user level, do various things - admins can do everything, authors can create profiles, guests can only view - and so on.

A friend of a friend was looking at my code, and has informed me that it's "woefully insecure". He also mentioned that my SQL statements would lead to very slow loading searches. Unfortunately, he hasn't told me how to rectify it!

Could you please give me some basic security and SQL tips? I've been looking up security on the net and realise that I should probably add a makesafe() function for variables which removes semi-colons, hashes and adds slashes to search variables. I realise, also, that I should turn off register global variables - but I'm not sure how to do that.

Said friend of a friend also told me that I should avoid using get/post variables from forms. I was using get methods, but because the "back" button stopped working, I changed the methods to post - apparently that's a bad idea.

The site needs to be secure. I have absolutely no doubt about that - I am not naive enough to think nobody will ever attempt to randomly access the site.

Excuse the coding please, it was one of the first PHP documents I wrote - with some bits borrowed from tutorials and the like.

Code: Select all

<?php session_start(); ?>
<html>
<head>
<title>Search Results</title>
</head>

<body>

<?php

include("../config.php");

if(!$_SESSION['username'] && !$_SESSION['password']) { 

echo '<center>Please log in</center>';

echo '
   <form method="post" action="../log.php?act=login"> 
   <table align="center" class="l_table">
   <tr>
   <td>Username:</td><td><input type="text" name="username" size="13" /></td></tr>
   <tr>
   <td>Password:</td> <td><input type="password" name="password" size="13" /></td></tr>
   <tr>
   <td>&nbsp;</td><td><input type="submit" value="Login" /></td></tr>
   </table>
   </form>';
}
else { 

mysql_close($db);

$username = $_SESSION['username'];
$ulevel = $_SESSION['userlevel'];
$mid = $_SESSION['mid'];

include "dataLib.php";

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

// BEGIN MULTIPLE SEARCH PAGES
// >Start variables

if (!($limit))
{
     $limit = 50;
} // Default results per-page.

if (!($page))
{
     $page = 0;
} // Default page value.

$search = $_REQUEST['commonName'];
$searchSpecies = $_REQUEST['species'];
$searchGenus = $_REQUEST['genus'];
$searchCat = 0;
$searchCatInp = $_REQUEST['category'];
$searchAuthInp = $_REQUEST['author'];
$catRead = read_cat($_REQUEST['field']);
$authRead = "";
if ($searchAuthInp == "666")
{
	$authRead = NULL;
	$authAddition = NULL;
}
else
{
	$authRead = read_auth($searchAuthInp);
	$authAddition = "AND author LIKE '$searchAuthInp%'";
}

$sqlphp = "commonName=$search&species=$searchSpecies&genus=$searchGenus&category=$searchCatInp&author=$searchAuthInp";

$sql = "";

// CATEGORY
if ($searchCatInp == "")
{
 	$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition";   
}
else if ($searchCatInp == "666")
{
  $sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition";
}
else
{
  $sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' AND category LIKE '$searchCatInp%' $authAddition";
}

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

if ($numrows == 0)
{
     echo("No results matching your query - ($search / $searchSpecies / $searchGenus / $catRead / $authRead)");
     exit();
}

// >Finish variables

// >Start math

$pages = intval($numrows/$limit); // Number of results pages.

// $pages now contains int of pages, unless there is a remainder from division.

if ($numrows % $limit)
{
  $pages++;
} // has remainder so add one page

$current = ($page/$limit) + 1; // Current page number.

if (($pages < 1) || ($pages == 0))
{
  $total = 1;
} // If $pages is less than one or equal to 0, total pages is 1.
else
{
  $total = $pages;
} // Else total pages is $pages value.

$first = $page + 1; // The first result.

if (!((($page + $limit) / $limit) >= $pages) && $pages != 1)
{
  $last = $page + $limit;
} //If not last results page, last result equals $page plus $limit.
 
else
{
  $last = $numrows;
} // If last results page, last result equals total number of results.

// >Finish math

$query = "$search // $searchSpecies // $searchGenus // $SearchAuthInp";

print <<<HERE

<title>Search Results</title>
</head>
<body><p>
<b><font size="3">Search Results</font></b><br>
<b>Common Name:</b> $search || <b>Genus:</b> $searchGenus || <b>Species:</b> $searchSpecies || 
<b>Category:</b> $catRead || <b>Author:</b> $authRead</p>
<table width="100%" border="0">
 <tr>
  <td align="left">
Page <b>$current</b> of <b>$total</b>
  </td>
 </tr>
 <tr>
  <td align="left">
Results <b>$first</b> - <b>$last</b> of <b>$numrows</b>
  </td>
 </tr>
 <tr>
  <td colspan="2" align="left">&nbsp;
  </td>
 </tr>
 <tr>
  <td colspan="2" align="left">
Results per-page: <a href="$PHP_SELF?$sqlphp&page=$page&limit=5">5</a> | <a href="$PHP_SELF?$sqlphp&page=$page&limit=10">10</a> | <a href="$PHP_SELF?$sqlphp&page=$page&limit=20">20</a> | <a href="$PHP_SELF?$sqlphp&page=$page&limit=50">50</a>
  </td>
 </tr>
</table>

HERE;

$sql = "";

// CATEGORY
if ($searchCatInp == "")
{
 	$sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition ORDER BY genus ASC LIMIT $page, $limit";   
}
else if ($searchCatInp == "666")
{
  $sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' $authAddition ORDER BY genus ASC LIMIT $page, $limit";
}
else
{
  $sql = "SELECT * FROM profile WHERE commonName LIKE '%$search%' AND species LIKE '%$searchSpecies%' AND genus LIKE '%$searchGenus%' AND category LIKE '$searchCatInp%' $authAddition ORDER BY genus ASC LIMIT $page, $limit";
}

$result = mysql_query($sql, $dbConn);

print "<p align=\"center\">";

if ($page != 0)
{ // Don't show back link if current page is first page.
  $back_page = $page - $limit;
  echo("<a href=\"$PHP_SELF?$sqlphp&page=$back_page&limit=$limit\">back</a><br>    \n");
}

for ($i=1; $i <= $pages; $i++) // loop through each page and give link to it.
{
 $ppage = $limit*($i - 1);
 if ($ppage == $page){
 echo("<b>$i</b> \n");} // If current page don't give link, just text.
 else{
 echo("<a href=\"$PHP_SELF?$sqlphp&page=$ppage&limit=$limit\">$i</a> \n");}
}

if (!((($page+$limit) / $limit) >= $pages) && $pages != 1)
{ // If last page don't give next link.
  $next_page = $page + $limit;
  echo("    <br><a href=\"$PHP_SELF?$sqlphp&page=$next_page&limit=$limit\">next</a>\n");
}
 
print "<p>";
 
while ($row = mysql_fetch_assoc($result))
{
  $id = $row["id"];
  $commonName = $row["commonName"];
  $species = strtolower($row["species"]);
  $genus = $row["genus"];
  $authorID = $row["author"];
  $count++;

  print "<p><b>Common name: </b>$commonName\n";
  print "<br><b>Scientific name: </b>$genus $species\n";
  print "<br>[<a href=\"../../profile.php?id=$id\">view</a>]\n";
  if ($ulevel == 1 || $ulevel == 2 && $mid == $authorID)
  {
    print "[<a href=\"edit.php?id=$id\">edit</a>]\n";
  }
  if ($ulevel == 1)
  {
    print "[<a href=\"delete.php?id=$id\">del</a>]</p>\n";
  }
}

print "<p align=\"center\">";

if ($page != 0)
{ // Don't show back link if current page is first page.
  $back_page = $page - $limit;
  echo("<a href=\"$PHP_SELF?$sqlphp&page=$back_page&limit=$limit\">back</a><br>    \n");
}

for ($i=1; $i <= $pages; $i++) // loop through each page and give link to it.
{
 $ppage = $limit*($i - 1);
 if ($ppage == $page){
 echo("<b>$i</b> \n");} // If current page don't give link, just text.
 else{
 echo("<a href=\"$PHP_SELF?$sqlphp&page=$ppage&limit=$limit\">$i</a> \n");}
}

if (!((($page+$limit) / $limit) >= $pages) && $pages != 1)
{ // If last page don't give next link.
  $next_page = $page + $limit;
  echo("    <br><a href=\"$PHP_SELF?$sqlphp&page=$next_page&limit=$limit\">next</a>\n");
}

print "</p>";

print <<<HERE
<p align="left">
<p>> <a href="../index.php">Return to CP</a></p>
</p>
HERE;

}

?>

</body>
</html>
Thanks in advance :)
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Welcome to PHPDN :)

Right, I don't have time to go through everything unfortunately (I'm clocking off in 5mins :D) but have noticed two things:

1. When checking if a variable exists, use:

Code: Select all

if (isset($var))
if you wish to check that it has content:

Code: Select all

if (!empty($var))
(nb: using empty() is useful for checking both if the variable isset and not empty at the same time..)

2. You call mysql_close($db); quite early on in your script. No connection has been made, and $db hasn't been decalred or defined - so there is no need for this function call at all :)
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

$PHP_SELF implies that register_globals in on. You should code as though it weren't (in my opinion). You may also want to look into other ways of getting the script name like $_SERVER['SCRIPT_FILENAME'] or basename(__FILE__).
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

Btw, using $_SERVER['PHP_SELF'] (or the outdated $PHP_SELF) in html is asking for XSS exploits.. Also notice that if you use the constant # instead (eg: <a href='#?var=name'>..</a>) you achieve the same result, namely redirecting to the url through which the page was requested...
dunc
Forum Newbie
Posts: 13
Joined: Mon Sep 04, 2006 10:17 am

Post by dunc »

*takes notes* :D

What's the most secure way of sending information through a HTML form, and receiving it into a variable?

I have heard SQL's FULLTEXT command being mentioned - is that a viable alternative to the SQL query I'm using?

Thanks again,
Duncs
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

Assigning script specific variables from the posted data is a good start...

Code: Select all

<?php
if (isset($_POST['my_var'])) {
    $my_var = $_POST['my_var'];
}
?>
... but as it relates to security, there are a lot of things to consider.
dunc
Forum Newbie
Posts: 13
Joined: Mon Sep 04, 2006 10:17 am

Post by dunc »

Everah wrote:... but as it relates to security, there are a lot of things to consider.
Hehe, such as? :) If you just gimme a poke in the right direction I can look into stuff myself :D Don't wanna miss stuff though!
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Read through all the threads in Security & Theory and Design. If you have questions, don't be afraid to ask.

Direction enough? :twisted:
jmut
Forum Regular
Posts: 945
Joined: Tue Jul 05, 2005 3:54 am
Location: Sofia, Bulgaria
Contact:

Post by jmut »

Jenk wrote:Welcome to PHPDN :)
....

1. When checking if a variable exists, use:.... (nb: using empty() is useful for checking both if the variable isset and not empty at the same time..)
....

uhh...maybe I misunderstood but...I don't think is the same

Code: Select all

<?php

error_reporting(E_ALL);

if (isset($var) && !empty($var)) {
    echo $var;
}

if (empty($var)) {
    echo $var;  //line 10
}
?>

Code: Select all

//outputs
<b>Notice</b>:  Undefined variable: var in <b>/storage/www/test/test.php</b> on line <b>10</b><br />
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

You omitted the '!'
dunc
Forum Newbie
Posts: 13
Joined: Mon Sep 04, 2006 10:17 am

Post by dunc »

Jenk wrote:Welcome to PHPDN :)

Right, I don't have time to go through everything unfortunately (I'm clocking off in 5mins :D) but have noticed two things:

1. When checking if a variable exists, use:

Code: Select all

if (isset($var))
if you wish to check that it has content:

Code: Select all

if (!empty($var))
(nb: using empty() is useful for checking both if the variable isset and not empty at the same time..)
Ok mate, will do :) Presumably that would GET/POST variables?
Jenk wrote:2. You call mysql_close($db); quite early on in your script. No connection has been made, and $db hasn't been decalred or defined - so there is no need for this function call at all :)
That's for the login section - log.php opens a database connection. I have no idea why I haven't closed it in log.php as opposed to the individual files, however.. :)
Post Reply