Page 1 of 1

Criticise my code!

Posted: Mon Sep 04, 2006 10:30 am
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 :)

Posted: Mon Sep 04, 2006 10:53 am
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 :)

Posted: Mon Sep 04, 2006 11:42 am
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__).

Posted: Mon Sep 04, 2006 12:02 pm
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...

Posted: Mon Sep 04, 2006 5:48 pm
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

Posted: Mon Sep 04, 2006 7:00 pm
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.

Posted: Mon Sep 04, 2006 7:21 pm
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!

Posted: Mon Sep 04, 2006 7:24 pm
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:

Posted: Tue Sep 05, 2006 3:03 am
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 />

Posted: Tue Sep 05, 2006 4:15 am
by Jenk
You omitted the '!'

Posted: Tue Sep 05, 2006 8:14 am
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.. :)