phpmysqlsql-injectionaddslashes

Upgrading PHP - Need to remove get_magic_quotes_gpc


We're upgrading from php 5.3 to 5.4, which is not backwards compatible for 'get_magic_quotes_gpc'. I understand the code will still work, sort of, but just bring back a FALSE each time.

However, I think the time has come to scrub this from our code.

Here's a typical example:

     $product_id = "0";
        if (isset($HTTP_GET_VARS["id"])) {
          $rid = (get_magic_quotes_gpc()) ? $HTTP_GET_VARS["id"] : addslashes($HTTP_GET_VARS["id"]);
        }

       //then $product_id gets used all over the place in many different queries

I've been researching how to fix this and this is what I came up with:

    $rid = "0";
    if (isset($HTTP_GET_VARS["id"])) {
    $rid = addslashes($HTTP_GET_VARS["id"]);
    }

I'm a little over my head here. I know this all has to do with SQL injection and such. Is my solution an reasonable/acceptable one?

Thanks in advance.

<<<< EDIT - ADDITIONAL INFORMATION >>>>

Thanks for the replies. Actually, we did a bunch of conversion to PDO about 18 mos ago (mostly due to this type of advice on stackoverflow :)

So, I may have some reduntant, pointless code going on. Here's the full picture of what is happening below the code I posted above that gets the variable from the URL.

You'll see, there is the (get_magic_quuotes_gpc) that used to be there, now commented out and replaced by the (addslashes). But that variable is passed on to a PDO query.

$product_id = "0";
if (isset($HTTP_GET_VARS["id"])) {
  //$product_id = (get_magic_quotes_gpc()) ? $HTTP_GET_VARS["id"] : addslashes($HTTP_GET_VARS["id"]);
  $product_id = addslashes($HTTP_GET_VARS["id"]);
}

// NEW QUERIES - BEG xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

try {  
    # MySQL with PDO_MYSQL  
    $pdo = new PDO("mysql:host=$hostname_db;dbname=$database_db", $username_db, $password_db);  
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

    // Query 1:  product details
    $stmt = $pdo->prepare('SELECT 
    [a bunch of stuff here, fields, joins, etc]
    WHERE product_id = ? ');  
    $stmt -> execute(array($rid));
    $row_count_resto_details = $stmt->rowCount();
    $row_resto_details = $stmt->fetch(PDO::FETCH_ASSOC);


}  
// Error message for pdo
catch(PDOException $e) {  
    echo $e->getMessage();  
}  


// END QUERY xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

Can I just get rid of all the stuff in the first 4-5 lines of code and just make it this:

$product_id = $HTTP_GET_VARS["id"];

Solution

  • No, it's not reasonable.

    The problem is, the "magic quotes" feature was a bad idea in the first place, so trying to replicate it only means that you've relied on a broken solution until now, and your application is without a doubt vulnerable to SQL injection attacks.

    "Why is magic quotes a broken solution?", you'd probably ask. And the answer is kind of hidden in the question itself - security and IT in general aren't and can't be magic. Whenever you see a solution that advertises itself or at least seems to work in a "magic" way, know that it is bad and you should never trust it.

    What you need instead are context-aware solutions, and in the case of preventing SQL injections - that's parameterized queries. You should read this to learn more: How can I prevent SQL-injection in PHP?

    I'd also urge you to upgrade straight to PHP 5.6, mostly for two reasons:

    Update (to answer the extended question):

    You not only can remove the addslashes() logic, but you MUST do that - if you leave it, it will add slashes to some of your input data and these slashes will be a part of the data itself.
    What you want to do instead is to validate input data - check if it is in the proper format in the first place. For example, if you expect a numeric ID - check if it only contains digits before using it.

    Also, $HTTP_GET_VARS has been deprecated since PHP 4.1 and you should be using $_GET instead.