Is this safe?

[1Unreal]

I'm making an ajax autocomplete search and I want to make sure the server side is safe.

Is there anything wrong with this?

PHP Code:

<?php
// ####################### SET PHP ENVIRONMENT ###########################
error_reporting(E_ALL & ~ E_NOTICE);

// #################### DEFINE IMPORTANT CONSTANTS #######################
define('THIS_SCRIPT', 'ajax_search');
define('CSRF_PROTECTION', true);
define('LOCATION_BYPASS', 1);
define('NOPMPOPUP', 1);

// ################### PRE-CACHE TEMPLATES AND DATA ######################
// get special phrase groups
$phrasegroups = array();

// get special data templates from the datastore
$specialtemplates = array();

// pre-cache templates used by all actions
$globaltemplates = array();

// ######################### REQUIRE BACK-END ############################
require_once ('./global.php');
$vbulletin->input->clean_gpc('r', 'q', TYPE_STR);
$query = $_REQUEST['q']; 

if(strlen($query) >= 3){
    $results = $vbulletin->db->query_read("SELECT `query` FROM `search` WHERE `query` LIKE '$query%' LIMIT 1");
    $result = $vbulletin->db->fetch_array($results);
    echo $result[query];
}
?>

[Dismounted]

That script is open to SQL injection. You're not safely escaping input. Apart from that, it could possibly cause some load issues (depending on how often AJAX calls the script).

[1Unreal]

Doesn't $vbulletin->input->clean_gpc() prevent injection?

[Dismounted]

No, it does not. $db->escape_string() prevents SQL injection.

[1Unreal]

Oh, I would have thought vB would automatically clean up all user input by default. Kind of a security flaw for us ignorant few.

[Dismounted]

Not all input needs to be escaped for SQL, as not all input is used in SQL queries. The same happens for XSS. Not all input needs to be escaped for XSS.

[1Unreal]

But there's no harm in escaping it though right?

[Dismounted]

Example

PHP Code:

$foo = 'Some Random Quote: "PHP is better than Rails"';
$bar = mysql_real_escape_string($foo);

// Some Random Quote: "PHP is better than Rails"
echo $foo;

// Some Random Quote: \"PHP is better than Rails\"
echo $bar; 


I think you can see the difference.

[1Unreal]

Sorry, you're right. I was presuming everything was being put in the database.

[Marco van Herwaarden]

Not only that, the input cleaner should not "clean" variables that still are used in their raw form (calculations for example). It will clean according to the class that is used.

Some examples:
TYPE_STR - Any string, including non-safe characters.
TYPE_NOHTML - Same as above, but cleaned of all HTML special characters. Safe to display on page (XSS), but not SQL-safe.

TYPE_INT - Non-numeric data is stripped, safe to display or use in query.
TYPE_UINT - Same as above, but only positive integers allowed.