Jump to content

How can i remove the SQL injection vulnerability in this code.


Kashmenon
Go to solution Solved by QuickOldCar,

Recommended Posts

My website is always under constant attack and i had to purchase sucuri, i want to put this code back up but i need to eliminate the SQL injection

<?php
 
require_once('meta/header.php');
 
 
 
$model_id = $_REQUEST['mod_id'];
 
$search_for = $_REQUEST['search_for'];
 
$category = $_REQUEST['category'];
 
$search_gender = $_REQUEST['search_gender'];
 
 
 
 
 
$session_index_for_card = $_REQUEST['session_index'];
 
 
 
$session_index_next = $session_index_for_card + 1;
 
$mod_id_next = $_SESSION['model_array'][$session_index_next];
 
 
 
$session_index_prev = $session_index_for_card - 1;
 
$mod_id_prev = $_SESSION['model_array'][$session_index_prev];
 
 
 
 
 
 
 
$model_imgQuery = "SELECT image_name FROM model_image WHERE fmodel_id = '".$model_id."' ORDER BY disp_priority DESC";
 
$model_imgQuery_exec = $db->query($model_imgQuery);
 
$model_imgQuery_result = $db->fetch_all_array($model_imgQuery_exec);
 
 
 
?>
 
<script>
 
$(document).ready(function(){
 
$('#contentHeading').html('Full Catalog');
 
$('#breadcrumb').html('<a href="<?=BASE_URL?>">Home</a>><a href="#">Full Catalog</a>');
 
 
 
<?php 
 
if($session_index_prev == '-1'){ 
 
?>
 
$('#prev_card').removeAttr('href');
 
<?php
 
}
 
if($session_index_next == count($_SESSION['model_array'])){
 
?>
 
$('#next_card').removeAttr('href');
 
<?php } ?> 
 
 
 
});
 
</script>
 
    <link rel="stylesheet" type="text/css" href="assets/js/full_catalog_slider/css/style.css" />
 
    <link rel="stylesheet" type="text/css" href="assets/js/full_catalog_slider/fancybox/jquery.fancybox-1.3.1.css" media="screen" />
 
<style>
 
.bigImg{overflow:auto; float:right !important; width:735px; max-height:320px; overflow-y: hidden;}
 
/*.big_img img{float:left !important;}*/
 
.catalague{left:375px; top:670px;}
 
iframe{height:100%; width:330%; border:0;}
 
.links_detail{margin-right:0 !important;}
 
</style>
 
<div id="content">
 
<div class="links_detail">
 
<a href="<?=BASE_URL?>full_catalog.php?mod_id=<?=$mod_id_next?>&search_for=<?=$search_for?>&category=<?=$category?>&session_index=<?=$session_index_next?>" id="next_card">NEXT CARD</a>
 
|
 
<a href="<?=BASE_URL?>full_catalog.php?mod_id=<?=$mod_id_prev?>&search_for=<?=$search_for?>&category=<?=$category?>&session_index=<?=$session_index_prev?>" id="prev_card">PREVIOUS CARD</a>
 
|
 
<a href="<?=BASE_URL.$search_for?>.php">START NEW SEARCH</a>
 
|
 
<a href="javascript:window.history.go(-1)">BACK</a>
 
</div>
 
<div class="bigImg">
 
 
 
    <div class="gallery-wrap">
 
        <div class="gallery clearfix">
 
    <?php foreach($model_imgQuery_result as $img): ?>
 
    
 
        <div class="gallery__item">
 
            <a rel="example_group" href="<?=BASE_URL?>model/<?=$img['image_name']?>" class="gallery__link">
 
            <img src="<?=BASE_URL?>model/<?=$img['image_name']?>" class="gallery__img" alt="" />
 
            </a>
 
        </div>
 
        
 
    <?php endforeach; ?>
 
        </div> <!-- .gallery -->
 
        
 
    </div>
 
</div>
 
<div class="gallery__controls clearfix">
 
    <div href="#" class="gallery__controls-prev">
 
        <img src="assets/js/full_catalog_slider/images/prev.png" alt="" />
 
    </div>
 
    <div href="#" class="gallery__controls-next">
 
        <img src="assets/js/full_catalog_slider/images/next.png" alt="" />
 
    </div>
 
</div> <!-- .gallery__controls -->
 
<div class="catalague">
 
 
 
<!--<img alt="" src="<?=BASE_URL?>assets/images/fullcatalogue.png"> 
 
<a href="<?=BASE_URL?>full_catalog.php?mod_id=<?=$model_id?>&search_for=<?=$search_for?>"> SEE FULL CATALOGUE</a>
 
       -->  
 
<?php require_once("meta/search_tools.php"); ?>
 
</div>
 
 
 
 </div>
 
    
 
</div>
 
 
 
</div>
 
 
 
    <script type="text/javascript">
 
    // Only run everything once the page has completely loaded
 
    $(window).load(function(){
 
 
 
        // Set general variables
 
        // ====================================================================
 
        var totalWidth = 0;
 
 
 
        // Total width is calculated by looping through each gallery item and
 
        // adding up each width and storing that in `totalWidth`
 
        $(".gallery__item").each(function(){
 
            totalWidth = totalWidth + $(this).outerWidth(true);
 
        });
 
 
 
        // The maxScrollPosition is the furthest point the items should
 
        // ever scroll to. We always want the viewport to be full of images.
 
        var maxScrollPosition = totalWidth - $(".gallery-wrap").outerWidth();
 
 
 
        // This is the core function that animates to the target item
 
        // ====================================================================
 
        function toGalleryItem($targetItem){
 
            // Make sure the target item exists, otherwise do nothing
 
            if($targetItem.length){
 
 
 
                // The new position is just to the left of the targetItem
 
                var newPosition = $targetItem.position().left;
 
 
 
                // If the new position isn't greater than the maximum width
 
                if(newPosition <= maxScrollPosition){
 
 
 
                    // Add active class to the target item
 
                    $targetItem.addClass("gallery__item--active");
 
 
 
                    // Remove the Active class from all other items
 
                    $targetItem.siblings().removeClass("gallery__item--active");
 
 
 
                    // Animate .gallery element to the correct left position.
 
                    $(".gallery").animate({
 
                        left : - newPosition
 
                    });
 
                } else {
 
                    // Animate .gallery element to the correct left position.
 
                    $(".gallery").animate({
 
                        left : - maxScrollPosition
 
                    });
 
                };
 
            };
 
        };
 
 
 
        // Basic HTML manipulation
 
        // ====================================================================
 
        // Set the gallery width to the totalWidth. This allows all items to
 
        // be on one line.
 
        $(".gallery").width(totalWidth);
 
 
 
        // Add active class to the first gallery item
 
        $(".gallery__item:first").addClass("gallery__item--active");
 
 
 
        // When the prev button is clicked
 
        // ====================================================================
 
        $(".gallery__controls-prev").click(function(){
 
            // Set target item to the item before the active item
 
            var $targetItem = $(".gallery__item--active").prev();
 
            toGalleryItem($targetItem);
 
        });
 
 
 
        // When the next button is clicked
 
        // ====================================================================
 
        $(".gallery__controls-next").click(function(){
 
            // Set target item to the item after the active item
 
            var $targetItem = $(".gallery__item--active").next();
 
            toGalleryItem($targetItem);
 
        });
 
    });
 
    </script>
 
 
 
    <script type="text/javascript" src="assets/js/full_catalog_slider/fancybox/jquery.fancybox-1.3.1.js"></script>
 
    <script type="text/javascript">
 
    // Fancybox specific
 
    // To make images pretty. Not important
 
    $(document).ready(function(){
 
        $(".gallery__link").fancybox({
 
            'titleShow'     : false,
 
            'transitionIn'  : 'elastic',
 
            'transitionOut' : 'elastic'
 
        });
 
    });
 
    </script>
 
 
 
 
 
<?php
 
require_once('meta/footer.php');
 
?>

Edited by Ch0cu3r
Link to comment
Share on other sites

  • Solution

Well the code is lacking in many ways.

not checking if anything is set or data you expect is just small flaws in this

 

 

Is $model_id always a number?

 

If so can check for that and only run the script or query if is true.

if(isset($_REQUEST['mod_id']) && ctype_digit(trim($_REQUEST['mod_id']))){
//execute
}else{
//do something else
}

Other ways to protect yourself would be to use prepared statements or mysqli_real_escape_string

 

You should check everything and take alternate actions like a redirect or produce errors if it fails before attempting to actually use them.

Link to comment
Share on other sites

Your code lacks any data validation and/or sanitation so there is no surprise your site is constantly under sql injection and most likely other forms of attacks.

 

Whenever you receive any type of user input ($_GET, $_POST, $_COOKIE etc) you must validate it, checking to make sure it does exist and checking the value is what you expect it to be if you know the format of the data. Such as if you require a date then validate it is a date and it is in the format you require, such in ie YYYY/MM/DD format or MM/DD/YYYY/ format. If you require a number then validate that it is a number. If you require a certain value then check that it is that value.

 

For input your cant check the format of such a persons name, contents of a blog post etc then the only validation you can apply is checking it does exist. What you must do though is sanitize it. If you are displaying the input on your webpage then apply htmlentities to prevent XSS (cross-site scripting). To prevent SQL injection the use your database api escape method, ie for mysqli use mysqli_real_escape_string or better yet use prepared statements.

 

Try not to use $_REQUEST in your, it will receive input from $_GET, $_POST and $_COOKIE at the same time. You should always use the appropriate super global variable for where your data is coming from, such as if the data is from the querystring in the url then use $_GET. If its from a form using the post method then use $_POST. If the data is from a cookie then use $_COOKIE.

Link to comment
Share on other sites

Thank you Ch0cu3r and QuickOldCar for your inputs. Appreciate the help a lot. I am just a beginner at coding but i would like to learn more about this.

 

@ QuickOldCar :- Yes the model ID is always a number which is either 4 or 5 digits. So I will use the ctype to validate the ID. Hopefully there wont be any attacks with that.

@Ch0cu3r :- I will look into the links mentioned and will try to get a workaround for my code. XSS Cross Scripting errors showed up on the Malware cleanup.

 

I did not think how this would logically pan out when this was done. Thank you again.

Link to comment
Share on other sites

I don't want to get into XSS and have this thread meander away from your subject.

 

SQL Injection!

 

The answer was provided for you by Quick Old Car.

 

Use prepared statements! http://php.net/manual/en/pdo.prepared-statements.php

 

You are using some sort of database class which we aren't privy to.

 

At this juncture, you should either be using one of the well known ORM's available (Doctrine, Propel, Eloquent) or just using vanilla mysqli or PDO. For someone starting out, I like Doctrine2 because you can drop down and just use DBAL and get the main benefits you want of using a db library while also having the option of moving up to the full ORM later.

 

Regardless, using prepared statements solves the problems of having to escape input AND eliminates SQL injections.

 

It's a 2 for one, that makes your code simpler and better at the same time. Accept no substitute.

Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.