How to select safe

[Tasos]

can anyone help me with this I do not know if this is safe, if not can someone tell me how I can do this?

$id //* is pagination
$per_page //* how many i show per page

$getquery = mysql_query("SELECT * FROM `videos` WHERE $construct ORDER BY date DESC LIMIT $id, $per_page");

Any help is appreciated  

[davidannis]

Assuming that $id and $per_page are not passed in the URL/form and $construct is properly escaped it is fine. We'd need to see how those variables get values to be sure.

Edited May 16, 2013 by davidannis

[Tasos]

Thanks for the reply, in the browser when you click the next button ( page 2 ) you see this

www.example.com/example.php?search=&submit=search&id=

 

and i found something, when you type this in the browser  -108%20order%20by%2010--

like

 

www.example.com/example.php?search=&submit=search&id=-108%20order%20by%2010--

 

you see this 

 

Warning: mysql_fetch_assoc(): supplied argument is not a valid MySQL result resource in /...../......./......./......./ on line 94      

 < I dont like to show the name of the website....for security reasons.

I have PIWIK statistics and today i see that somebody has made :

 

97 Action - 26 min 48s on my website.

here below is the ip from where they searched the website

--------------------------------------------------------------------------------------

IP: 67.171.143.215
Provider: Comcast

--------------------------------------------------------------------------------------

And what they type in the browser is this here below

--------------------------------------------------------------------------------------

 

-108/!*UnIoN*/+/!*sElEcT*/1,--

 

From 1 / 20

 

-108/!*UnIoN*/+/!*sElEcT*/1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20--

 

%20union%20select%201--

 

From 1 / 20

 

%20union%20select%201,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20--

 

%20and%201=1

 

<script>alert( 'fart' )</script>

Edited May 16, 2013 by Tasos

[Tasos]

 We'd need to see how those variables get values to be sure.

Do you mean to show more of the code ?

[mac_gyver]

you must condition/validate/limit all external data before using it.

 

in the case of a negative page number, that passes through the math in your code and produces a negative starting row number in the LIMIT clause, which is invalid and produces a query error. since the query doesn't run at all, you are getting an error when you try to use the fetch statement. this also indicates you don't have any error checking logic for your query. if your query fails with an error, don't continue running code that tries to use the result from the query.

 

you need to insure that the page number is greater then or equal to 1 and you should also insure that it is less than or equal to the maximum page number (this condition won't produce an error, but the query won't ever return any rows.)

[Tasos]

I am not so good with php i have a long way to go and to learn more. 

 

Could somebody give me an example like how this code should look ?

$getquery = mysql_query("SELECT * FROM `videos` WHERE $construct ORDER BY date DESC LIMIT $id, $per_page");

Thanks for the replys

[davidannis]

Not sure how to sanitize $construct because we don't see how it is built but for $id or $per_page something like

$id=intval($_REQUEST['id]);
if ($id<1){
echo "error message about starting with an id that is at lest one'; // or redirect to an error page
die();
} 

[Tasos]

Here is the complete script.

<?php 
$button = $_GET ['submit'];
$search = $_GET ['search']; 
echo " ";

include 'extern/connectsearch.php';
$search_exploded = explode (" ", $search);
foreach($search_exploded as $funny)
{
$x++;
if($x==1)
$construct .="title LIKE '%Funny%'";
else
$construct .="AND title LIKE '%Funny%'";
                    
$constructs ="SELECT * FROM videos WHERE $construct";
$run = mysql_query($constructs);
   
$foundnum = mysql_num_rows($run);
   
if ($foundnum==0)
                
echo "Sorry, there are no matching result for <b>$search</b>.</br></br>1. ";

$per_page = 36;  
$id = ($_GET['id']);
$max_pages = ceil($foundnum / $per_page);
if(!$id)
$id=0;                          
$getquery = mysql_query("SELECT * FROM videos WHERE $construct ORDER BY date DESC LIMIT $id, $per_page");
            
$thumbs = $runrows ['thumbs'];
$title = $runrows ['title'];
$channel = $runrows ['channel'];
$url = $runrows ['url'];
$duration = $runrows ['duration'];

 while($runrows = mysql_fetch_assoc($getquery))
{
echo ' ';

 }
echo "<center>";

  ?>

And here below is the pagination.

 <?php
//Pagination ids
echo "<center>";
$prev = $id - $per_page;
$next = $id + $per_page;
                       
$adjacents = 5;
$last = $max_pages - 1;
  
if($max_pages > 1)
{   

//previous button
if (!($id<=0)) 
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$prev'>Prev</a> </div>";    
          
//pages 
if ($max_pages < 7 + ($adjacents * 2))   //not enough pages to bother breaking it up
{
$i = 0;   
for ($counter = 1; $counter <= $max_pages; $counter++)
{
if ($i == $id){
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$i'><font color=orange><b>$counter</b></font></a></div> ";
}
else {
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$i'>$counter</a></div> ";
}  
$i = $i + $per_page;                 
}
}
elseif($max_pages > 5 + ($adjacents * 2))    //enough pages to hide some
{
//close to beginning; only hide later pages
if(($id/$per_page) < 1 + ($adjacents * 2))        
{
$i = 0;
for ($counter = 1; $counter < 4 + ($adjacents * 2); $counter++)
{
if ($i == $id){
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$i'><font color=orange><b>$counter</b></font></a></div> ";
}
else {
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$i'>$counter</a></div> ";
} 
$i = $i + $per_page;                                       
}
                          
}
//in middle; hide some front and some back
elseif($max_pages - ($adjacents * 2) > ($id / $per_page) && ($id / $per_page) > ($adjacents * 2))
{
echo " <div class='paginate'><a href='funny.php?search=$search&submit=search&id=0'>1</a></div> ";
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$per_id'>2</a> ....</div> ";
 
$i = $id;                 
for ($counter = ($id/$per_page)+1; $counter < ($id / $per_page) + $adjacents + 2; $counter++)
{
if ($i == $id){
echo " <div class='paginate'><a href='funny.php?search=$search&submit=search&id=$i'><font color=orange><b>$counter</b></font></a></div>";
}
else {
echo " <div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$i'>$counter</a></div> ";
}   
$i = $i + $per_page;                
}
                                  
}
//close to end; only hide early pages
else
{
echo " <div class='paginate'> <a href='funny.php?search=$search&submit=search&id=0'>1</a></div> ";
echo " <div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$per_id'>2</a> ....</div> ";
 
$i = $id;                
for ($counter = ($id / $per_page) + 1; $counter <= $max_pages; $counter++)
{
if ($i == $id){
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$i'><font color=orange><b>$counter</b></font></a></div>";
}
else {
echo " <div class='paginate'><a href='funny.php?search=$search&submit=search&id=$i'>$counter</a></div> ";   
} 
$i = $i + $per_page;              
}
}
}
          
//next button
if (!($id >=$foundnum-$per_page))
echo "<div class='paginate'> <a href='funny.php?search=$search&submit=search&id=$next'>Next</a></div> ";    
}   
echo "</center>";
} 

?>

[davidannis]

$construct contains unsanitized user input. It is easily exploited in an SQL injection attack. You need to sanitize the data. You can use http://php.net/manual/en/function.mysql-real-escape-string.php to do so. If you try to write the code, we'll help but we won't write it for you.

[Tasos]

Thanks for the link... i will try to make it  by my self, if not than i need to rent php programmer to fix the problem.

[davidannis]

I was going to re-write this for the OP since he's trying, but before I do I have something that's puzzling me. Can anybody explain why this:

$construct .="title LIKE '%Funny%'";

is not what I expected, which would have been this:

$construct .="title LIKE '%$funny%'";

which I would have rewritten as:

$construct .="title LIKE '%".mysql_real_escape_string($funny)."%'";

I'm clearly missing something.

[mac_gyver]

I was going to re-write this for the OP since he's trying, but before I do I have something that's puzzling me. Can anybody explain why this:

$construct .="title LIKE '%Funny%'";

is not what I expected,

 

 

the OP's code is pretty much just some thrown together gobbledygook that doesn't search for what was entered or output anything at all even if it found it. even the 'pagination' isn't using pages.