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

[Kashmenon]

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 August 31, 2015 by Ch0cu3r

[QuickOldCar]

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.

[Ch0cu3r]

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.

[Kashmenon]

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.

[gizmola]

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.

[Kashmenon]

Thank you Gizmola. Will follow Quick Old Car's prepared statements