Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 4 of 4
  1. #1
    New to the CF scene
    Join Date
    Oct 2012
    Posts
    3
    Thanks
    2
    Thanked 0 Times in 0 Posts

    Protecting from SQL injection

    I have a website set up using plaincart as a catalog (no order processing being done). I know i shouldnt be using plaincart, i was unaware of all the issues with it until after i had everything set up. Im looking for a fix to fix the SQL injection problem with it.
    Heres the hack:

    http://[target]/[script]/index.php?c=16&p=-3+UNION+SELECT+user_name,user_password,3,4,5+from+tbl_user—


    Here is partial index.php code:
    Code:
     
    <?php
    require_once 'library/config.php';
    require_once 'library/category-functions.php';
    require_once 'library/product-functions.php';
    require_once 'library/cart-functions.php';
    
    $_SESSION['shop_return_url'] = $_SERVER['REQUEST_URI'];
    
    $catId  = (isset($_GET['c']) && $_GET['c'] != '1') ? $_GET['c'] : 0;
    $pdId   = (isset($_GET['p']) && $_GET['p'] != '') ? $_GET['p'] : 0;
    
    
    
    if (!defined('WEB_ROOT')) {
    	exit;
    }
    
    // set the default page title
    $pageTitle = '';
    
    // if a product id is set add the product name
    // to the page title but if the product id is not
    // present check if a category id exist in the query string
    // and add the category name to the page title
    if (isset($_GET['p']) && (int)$_GET['p'] > 0) {
    	$pdId = mysql_real_escape_string($_GET['p']);
    	$sql = "SELECT pd_name
    			FROM tbl_product
    			WHERE pd_id = $pdId";
    	
    	$result    = dbQuery($sql);
    	$row       = dbFetchAssoc($result);
    	$pageTitle = $row['pd_name'];
    	
    } else if (isset($_GET['c']) && (int)$_GET['c'] > 0) {
    	$catId = mysql_real_escape_string($_GET['c']);
    	$sql = "SELECT cat_name FROM tbl_category WHERE cat_id = $catId";
    
    $result = dbQuery($sql);
    	$row       = dbFetchAssoc($result);
    	$pageTitle = $row['cat_name'];
    }


    and the dbQuery() function:
    Code:
     
    function dbQuery($sql)
    {
    	$result = mysql_query($sql) or die(mysql_error());
    	
    	return $result;
    }
    There was a fix posted on a website, but i tried it and all i got was a mysql query error, the fix was replace this line:
    Code:
     
    $pdId   = (isset($_GET['p']) && $_GET['p'] != '') ? $_GET['p'] : 0;
    with:

    Code:
     
    function valid_pdId($get)
    {
    $x = isset($_GET[$get])&&$_GET[$get]!='1' ? $_GET[$get] : '';
    if ( !ctype_digit($x) ) {
    $x = ' ';
    }
    return $x;
    }
    $pdId = valid_pdId('p');


    thanks for anyone that can help.

  • #2
    Master Coder felgall's Avatar
    Join Date
    Sep 2005
    Location
    Sydney, Australia
    Posts
    6,640
    Thanks
    0
    Thanked 649 Times in 639 Posts
    To eliminate the possibility of SQL injection simply use either PDO or mysqli_ with prepare and bind statements that keep the SQL and data completely separate and stop using the now obsolete mysql_ calls. If the SQL is in the prepare statement and the data is in the bind statement then nothing in the data will ever be treated as SQL no matter how hard they try to inject it.

    Also if you validate your data properly when it is first input you will eliminate any input field that cannot validly contain quote characters from being able to be used for injection.
    Stephen
    Learn Modern JavaScript - http://javascriptexample.net/
    Helping others to solve their computer problem at http://www.felgall.com/

    Don't forget to start your JavaScript code with "use strict"; which makes it easier to find errors in your code.

  • Users who have thanked felgall for this post:

    balance85 (11-25-2012)

  • #3
    Super Moderator
    Join Date
    May 2002
    Location
    Perth Australia
    Posts
    4,051
    Thanks
    10
    Thanked 94 Times in 92 Posts
    the below uses is_numeric() but ctype functions or filter_var functions could be used instead.
    note that there are 3 changes here, all prefixed by #HERE

    PHP Code:
     <?php
    require_once 'library/config.php';
    require_once 
    'library/category-functions.php';
    require_once 
    'library/product-functions.php';
    require_once 
    'library/cart-functions.php';

    $_SESSION['shop_return_url'] = $_SERVER['REQUEST_URI'];

    #HERE
    $catId  = (isset($_GET['c']) && is_numeric($_GET['c'])) ? $_GET['c'] : 0;
    $pdId   = (isset($_GET['p']) && is_numeric($_GET['p'])) ? $_GET['p'] : 0;



    if (!
    defined('WEB_ROOT')) {
        exit;
    }

    // set the default page title
    $pageTitle '';

    // if a product id is set add the product name
    // to the page title but if the product id is not
    // present check if a category id exist in the query string
    // and add the category name to the page title
    if (isset($_GET['p']) && (int)$_GET['p'] > 0) {
            
    #HERE
        
    $pdId mysql_real_escape_string($pdId);
        
    $sql "SELECT pd_name
                FROM tbl_product
                WHERE pd_id = $pdId"
    ;
        
        
    $result    dbQuery($sql);
        
    $row       dbFetchAssoc($result);
        
    $pageTitle $row['pd_name'];
        
    } else if (isset(
    $_GET['c']) && (int)$_GET['c'] > 0) {
            
    #HERE
        
    $catId mysql_real_escape_string($catId);
        
    $sql "SELECT cat_name FROM tbl_category WHERE cat_id = $catId";

    $result dbQuery($sql);
        
    $row       dbFetchAssoc($result);
        
    $pageTitle $row['cat_name'];
    }



    and 
    the dbQuery() function:
    Code:
    @felgall you are correct of course but that would be a complete rewrite , as it is the op needs to run through and qheck queries everywhere to look for more potential issues
    resistance is...

    MVC is the current buzz in web application architectures. It comes from event-driven desktop application design and doesn't fit into web application design very well. But luckily nobody really knows what MVC means, so we can call our presentation layer separation mechanism MVC and move on. (Rasmus Lerdorf)

  • Users who have thanked firepages for this post:

    balance85 (11-25-2012)

  • #4
    Master Coder felgall's Avatar
    Join Date
    Sep 2005
    Location
    Sydney, Australia
    Posts
    6,640
    Thanks
    0
    Thanked 649 Times in 639 Posts
    Quote Originally Posted by firepages View Post
    @felgall you are correct of course but that would be a complete rewrite , as it is the op needs to run through and qheck queries everywhere to look for more potential issues
    I learnt through experience that in this sort of situation a complete rewrite does not take significantly longer than thoroughly checking all the existing code in the hope that you have spotted all the potential security holes.

    Security isn't something that you just tack on the end of the steps you follow in creating a script or program, it is something that should be considered in the early design stages and revisited in every stage after that.

    It will be up to the OP whether they patch all the potential security holes they can find and hope they didn't miss any or spend slightly longer on a rewrite that is guranteed to remove the possibility of certain types of security issue occuring at all.
    Stephen
    Learn Modern JavaScript - http://javascriptexample.net/
    Helping others to solve their computer problem at http://www.felgall.com/

    Don't forget to start your JavaScript code with "use strict"; which makes it easier to find errors in your code.


  •  

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •