PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Mon Sep 28, 2020 2:08 pm

All times are UTC - 5 hours




Post new topic Reply to topic  [ 11 posts ] 
Author Message
 Post subject: Criticise my code!
PostPosted: Mon Sep 04, 2006 10:30 am 
Offline
Forum Newbie

Joined: Mon Sep 04, 2006 10:17 am
Posts: 13
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.

Syntax: [ Download ] [ Hide ]
<?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>&gt; <a href="../index.php">Return to CP</a></p>

</p>

HERE
;



}



?>



</body>

</html>


Thanks in advance :)


Top
 Profile  
 
 Post subject:
PostPosted: Mon Sep 04, 2006 10:53 am 
Offline
DevNet Master
User avatar

Joined: Mon Sep 19, 2005 6:24 am
Posts: 3587
Location: London
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:
Syntax: [ Download ] [ Hide ]
if (isset($var))
if you wish to check that it has content:
Syntax: [ Download ] [ Hide ]
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 :)


Top
 Profile  
 
 Post subject:
PostPosted: Mon Sep 04, 2006 11:42 am 
Offline
Site Administrator
User avatar

Joined: Tue Sep 09, 2003 6:04 pm
Posts: 14293
Location: Fremont, CA, USA
$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__).


Top
 Profile  
 
 Post subject:
PostPosted: Mon Sep 04, 2006 12:02 pm 
Offline
DevNet Master

Joined: Tue Jan 20, 2004 12:11 am
Posts: 4897
Location: Leuven, Belgium
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...


Top
 Profile  
 
 Post subject:
PostPosted: Mon Sep 04, 2006 5:48 pm 
Offline
Forum Newbie

Joined: Mon Sep 04, 2006 10:17 am
Posts: 13
*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


Top
 Profile  
 
 Post subject:
PostPosted: Mon Sep 04, 2006 7:00 pm 
Offline
Site Administrator
User avatar

Joined: Tue Sep 09, 2003 6:04 pm
Posts: 14293
Location: Fremont, CA, USA
Assigning script specific variables from the posted data is a good start...
Syntax: [ Download ] [ Hide ]
<?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.


Top
 Profile  
 
 Post subject:
PostPosted: Mon Sep 04, 2006 7:21 pm 
Offline
Forum Newbie

Joined: Mon Sep 04, 2006 10:17 am
Posts: 13


Top
 Profile  
 
 Post subject:
PostPosted: Mon Sep 04, 2006 7:24 pm 
Offline
Neighborhood Spidermoddy
User avatar

Joined: Mon Mar 29, 2004 4:24 pm
Posts: 31559
Location: Bothell, Washington, USA
Read through all the threads in Security & Theory and Design. If you have questions, don't be afraid to ask.

Direction enough? :twisted:


Top
 Profile  
 
 Post subject:
PostPosted: Tue Sep 05, 2006 3:03 am 
Offline
Forum Regular

Joined: Tue Jul 05, 2005 3:54 am
Posts: 945
Location: Sofia, Bulgaria


Top
 Profile  
 
 Post subject:
PostPosted: Tue Sep 05, 2006 4:15 am 
Offline
DevNet Master
User avatar

Joined: Mon Sep 19, 2005 6:24 am
Posts: 3587
Location: London


Top
 Profile  
 
 Post subject:
PostPosted: Tue Sep 05, 2006 8:14 am 
Offline
Forum Newbie

Joined: Mon Sep 04, 2006 10:17 am
Posts: 13


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 11 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group