Pagination Security?

[dropfaith]

how would i secure this from sql injection and xss i heard its not currently last time i had my security checked

 

<?php
// includes
include("template/conf.php");


// open database connection
$connection = mysql_connect($host, $user, $pass) or die ("Unable to connect!");

// select database
mysql_select_db($db) or die ("Unable to select database!");

// find out how many rows are in the table 
$sql = "SELECT COUNT(*) FROM news";
$result = mysql_query($sql, $connection) or trigger_error("SQL", E_USER_ERROR);
$r = mysql_fetch_row($result);
$numrows = $r[0];

// number of rows to show per page
$rowsperpage = 1;
// find out total pages
$totalpages = ceil($numrows / $rowsperpage);
// get the current page or set a default  
if (isset($_GET['currentpage']) && is_numeric($_GET['currentpage'])) {  
   // cast var as int  
    $currentpage = (int) $_GET['currentpage']; 
} else {
   // default page num
   $currentpage = 1;
} // end if

// if current page is greater than total pages...
if ($currentpage > $totalpages) {
   // set current page to last page
   $currentpage = $totalpages;
} // end if
// if current page is less than first page...
if ($currentpage < 1) {
   // set current page to first page
   $currentpage = 1;
} // end if

// the offset of the list, based on current page 
$offset = ($currentpage - 1) * $rowsperpage;

// get the info from the db 
$sql = "SELECT * FROM news order by Id asc LIMIT $offset, $rowsperpage";
$result = mysql_query($sql, $connection) or trigger_error("SQL", E_USER_ERROR);

// while there are rows to be fetched...
while ($list = mysql_fetch_assoc($result)) {
   // echo data
   
echo "<div class=\"news\">";
echo "<div class=\"title\"><a style=\"text-decoration:none;\" href=\"permalink.php?Id=". $list['Id'] . "\">". $list['Title'] . "</a></div>"; 
	echo "<div class=\"date\">". $list['Date'] . "</div>";
	echo "<div class=\"fullnews\">". (nl2br($list['News'])) . "</div>";







} // end while
echo "<div class=\"date\" style=\"text-align:center\">";
/******  build the pagination links ******/
// range of num links to show
$range = 3;

// if not on page 1, don't show back links
if ($currentpage > 1) {
   // show << link to go back to page 1
   echo " <a href='{$_SERVER['PHP_SELF']}?currentpage=1'>Oldest News</a> ";
   // get previous page num
   $prevpage = $currentpage - 1;
   // show < link to go back to 1 page
   echo " <a href='{$_SERVER['PHP_SELF']}?currentpage=$prevpage'>Previous Page</a>   ";
} // end if 



// if not on last page, show forward and last page links	
if ($currentpage != $totalpages) {
   // get next page
   $nextpage = $currentpage + 1;
    // echo forward link for next page 
   echo " <a href='{$_SERVER['PHP_SELF']}?currentpage=$nextpage'>Next Page</a>  ";
   // echo forward link for lastpage
   echo " <a href='{$_SERVER['PHP_SELF']}?currentpage=$totalpages'>Most Recent</a> ";
} // end if
/****** end build pagination links ******/
echo "</div>";
echo "</div>";
?>

[.josh]

$_GET['currentpage'] is the only outside variable the script does anything with, and this code validates it:

// get the current page or set a default  
if (isset($_GET['currentpage']) && is_numeric($_GET['currentpage'])) {  
   // cast var as int  
    $currentpage = (int) $_GET['currentpage']; 
} else {
   // default page num
   $currentpage = 1;
} // end if

 

It checks if it's there.  It checks if it's numeric.  It even casts it as an integer.  This will make any injection or xss attack through it impossible.  If someone told you the script is vulnerable, it's not from the pagination.

[dropfaith]

odd theres no other code on the page except xhtml 

 

oh well ill try to get it checked again and see if they say it again

[dropfaith]

thanks for the help (and the tutorial on pagenation) seeing is it was yours

[.josh]

Script also makes sure you don't try to enter non-existent pages.  I am kind of interested to know what "they" say about it being vulnerable.  I'm not a security expert, but I'm pretty sure it should hold up, as is. 

[dropfaith]

http://www.phpfreaks.com/forums/index.php/topic,220207.0.html

 

i ono but its one the list from darkfreaks of my holes about halfway doen the thread?

[.josh]

Well...mysql_real_escape_string will prevent most sql injection attacks, yes.  But it is just as effective if not better to specifically filter your data by using your own conditions and regexes.  For instance, if you have this:

 

   $allowed = array('red','yellow','blue','green');
   $color = (in_array($_GET['color'], $allowed))? $_GET['color'] : 'red';
   $sql = "select * from table where color = '$color'";

 

using mysql_real_escape_string or trim or any other thing would be superfluous.  If the user tries to enter in something besides the values in the $allowed array, it will simply assign the default.  Same principle as with the pagination.  You are expecting a valid integer in a certain range.  If you make sure it's a valid integer within a certain range, doing anything else is not needed.

 

But on the other hand, if you rely solely on built-in functions to do things, you are putting your trust in things that could have bugs found in them that may not yet be known.  As a matter of fact, I a while back found this article that shows how sql injection is, in fact, possible, even if you use mysql_real_escape_string.