Preventing SQL Injection in PHP Script (Full Version)

All Forums >> [Fun] >> Computers & Technology



Message

NeoGenesis -> Preventing SQL Injection in PHP Script (4/6/2008 7:01:44 PM)

I'm really just hoping there's another nerd wandering around in here somewhere...

So here it is-- I'm using a script to make downloads readily available on my website (mostly videos and audio from churches, lectures, etc.) but I've found out lately that the script has some flaws in it. Not the least of which is a vulnerability that could allow an attacker to inject SQL using the "Search" feature.

The program is PHCDownload: http://www.phpcredo.com/Products/PHCDownload/

Here is a link to the vulnerability: http://lostmon.blogspot.com/2007/12/xss-flaw-posible-sql-injection-in.html

The link above also has the line of code in question.

I'm hoping this is a quick fix, but I'll take any help I can get. I know Christians sites, especially, are subject to attacks and I'd really like to get this all cleaned up before I launch it completely.

Thanks a lot!



TheNextBillGates -> RE: Preventing SQL Injection in PHP Script (4/8/2008 10:59:07 PM)

Right before the code listed on the website parse the string and remove any non-alphabetic characters, leaving only letters and numbers. That should thwart the SQL injection and prevent any code (hexidecimal or otherwise) to be executed.



NeoGenesis -> RE: Preventing SQL Injection in PHP Script (4/9/2008 8:42:05 PM)

Ok, thanks. But how exactly should that look? I don't really write PHP very well.



TheNextBillGates -> RE: Preventing SQL Injection in PHP Script (4/9/2008 10:58:04 PM)

I've never coded in it, actually. I'd have to play around with it to give you a good answer, but I don't have an environment here to do anything with it. I'd start here:

http://us.php.net/strings

and find the functions that would allow me to parse the string to remove non-alphabetic characters.

Also, it looks like preg_match() might help. Consider this example and perhaps modify:

http://en.wikibooks.org/wiki/Programming:PHP:SQL_Injection

Hope that helps!



NeoGenesis -> RE: Preventing SQL Injection in PHP Script (4/10/2008 12:54:00 AM)

I tried and tried and... no hope. Every time I tried to edit the code I got a blank page as the result - which means I coded it wrong, I guess...
Here's the actual script:

quote:

// Prepare search query
if( $kernel->config['archive_search_mode'] == 1 )
{
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$kernel->vars['string']}*' IN BOOLEAN MODE )";
}
else
{
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$kernel->vars['string']}*' )";
}

if( $kernel->vars['category_id'] > 0 )
{
$search_category = "AND f.file_cat_id = {$kernel->vars['category_id']}";
}

if( $kernel->vars['sort'] == "file_ranking" )
{
$file_ranking_syntax = ", ( f.file_rating / f.file_votes ) AS file_ranking";
}

//Basic search query
$check_files = $kernel->db->query( "SELECT f.* {$file_ranking_syntax} FROM " . TABLE_PREFIX . "files f WHERE {$search_syntax} {$search_category} AND f.file_disabled = 0" );


I'm not sure WHERE to stick the new code to sanitize it.



iluvatar -> RE: Preventing SQL Injection in PHP Script (4/10/2008 8:01:31 AM)

quote:

ORIGINAL: NeoGenesis

I tried and tried and... no hope. Every time I tried to edit the code I got a blank page as the result - which means I coded it wrong, I guess...
Here's the actual script:

quote:

// Prepare search query
if( $kernel->config['archive_search_mode'] == 1 )
{
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$kernel->vars['string']}*' IN BOOLEAN MODE )";
}
else
{
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$kernel->vars['string']}*' )";
}

if( $kernel->vars['category_id'] > 0 )
{
$search_category = "AND f.file_cat_id = {$kernel->vars['category_id']}";
}

if( $kernel->vars['sort'] == "file_ranking" )
{
$file_ranking_syntax = ", ( f.file_rating / f.file_votes ) AS file_ranking";
}

//Basic search query
$check_files = $kernel->db->query( "SELECT f.* {$file_ranking_syntax} FROM " . TABLE_PREFIX . "files f WHERE {$search_syntax} {$search_category} AND f.file_disabled = 0" );


I'm not sure WHERE to stick the new code to sanitize it.



I've never worked with PHP, so I don't know it's peculiarities, but wouldn't you just sanitize 'string' before all of this? Or if 'string' is some sort of global (or global-ish) variable, then assign the value of 'string' to some local variable, parse the local variable ans use that for the rest of your code.

-Dan.



TheNextBillGates -> RE: Preventing SQL Injection in PHP Script (4/10/2008 10:22:51 PM)

I think I'd start here: 

// Prepare search query 
 if (preg_match("/^\w{8,20}$/", $kernel->vars['string'], $clean_string))
   $execute = "Y";
 else 
   $execute = "N";

if( $kernel->config['archive_search_mode'] == 1 and $execute = "Y") 
{ 
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$clean_string}*' IN BOOLEAN MODE )"; 
} 
else 
{ 
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$clean_string}*' )"; 
} 

if( $kernel->vars['category_id'] > 0 ) 
{ 
$search_category = "AND f.file_cat_id = {$kernel->vars['category_id']}"; 
} 

if( $kernel->vars['sort'] == "file_ranking" ) 
{ 
$file_ranking_syntax = ", ( f.file_rating / f.file_votes ) AS file_ranking"; 
} 

//Basic search query 
$check_files = $kernel->db->query( "SELECT f.* {$file_ranking_syntax} FROM " . TABLE_PREFIX . "files f WHERE {$search_syntax} {$search_category} AND f.file_disabled = 0" );


That's assuming that 'string' is the only variable the user could inject something. Trick is now anythign that doesn't pass validation (1-50 chars including numbers, letters, and underscore) will cause check_files to have a NULL or maybe an old value. Might want to default it to something to your liking.

That's the best I can come up with without knowing PHP and making it nice and pretty.



walterquez -> RE: Preventing SQL Injection in PHP Script (4/11/2008 2:02:13 AM)

Generally it is the quotes that makes SQL injection possible. So one way is to strip them, or convert them to HTML equivalent.

1. To strip single and double quotes
$kernel->vars['string'] = str_replace("'", '', $kernel->vars['string']); // single
$kernel->vars['string'] = str_replace('"', '', $kernel->vars['string']); // double

2. You can also convert them to their HTML equivalent, if needed, otherwise strip them out.
$kernel->vars['string'] = str_replace("'", ''', $kernel->vars['string']); // single
$kernel->vars['string'] = str_replace('"', '"', $kernel->vars['string']); // double

And then run the rest of your code.



NeoGenesis -> RE: Preventing SQL Injection in PHP Script (4/11/2008 11:23:24 AM)

thanks a lot! I'm to drop that code in now, walterquez.

Do you mean to replace $kernel->vars['string'] with str_replace('"', '', $kernel->vars['string']) ?

I tried that and I was still able to execute the exploit listed on http://lostmon.blogspot.com/2007/12/xss-flaw-posible-sql-injection-in.html

Chances are I'm just replacing the wrong thing... Do I need to replace the $kernel->vars['string'] everywhere on the PHP file?



walterquez -> RE: Preventing SQL Injection in PHP Script (4/11/2008 6:41:52 PM)

You're correct. It would be better to do the replacement at the source.

It looks like the variable $kenel was defined using a Class, something like the code below.

$kernel = new SomeClass();

I used "SomeClass()" as an example. You would have to search for the actual name.
Once you get the name, look for the following, "class SomeClass()". Replace "SomeClass()" with the actual name of the Class.

When you find the Class, it will look something like this,

class SomeClass() {
public $vars

public function __construct() {
...bunch of codes
}

private function doSearch() {
...bunch of codes
}

public __destruct() {
...bunch of codes
}

}

Inside the class you will have to find where the variable "vars" is assigned a value. It may be as simple as putting the code I gave you right after that line.

$this->vars['string'] = str_replace("'", '', $this->vars['string']); // single
$this->vars['string'] = str_replace('"', '', $this->vars['string']); // double

Notice I used $this, because I am referencing a variable inside a class.

In any case, I would have to see the Class definition to give you a better solution.



walterquez -> RE: Preventing SQL Injection in PHP Script (4/11/2008 6:52:41 PM)

Or it may be even easier to replace the input.

How is the search submitted? In the URL, or using POST?
For example, if it is something like, http://www.neogenesis.com/download.php?search=somefile, then it is a $_GET.
If the form's method is post, then it is $_POST.

Assuming the variable is "search", and it gets submitted through the URL, then search IN the code for "$_GET['search']".

Once you find it, then that is the variable you will strip the quotes off.

If it uses post, then find $_POST['search'], and used that to replace.

And to be sure, look for $_REQUEST['search'], just for added measure, in case they use this method in their code.

But either way, you can use the same code structure below. Just replace the variables with either $_GET, $_POST, $_REQUEST.

$_REQUEST['string'] = str_replace("'", '', $_REQUEST['string']); // single
$_REQUEST['string'] = str_replace('"', '', $_REQUEST['string']); // double



NeoGenesis -> RE: Preventing SQL Injection in PHP Script (4/11/2008 10:38:46 PM)

I did a search of Search.php and looked for "Get" and such, but I did not come up with any results. I was also unable to properly follow your instructions for changing the "class".
I've been at this for hours now and I guess now I'm willing to admit I have no idea what I'm doing.

Below is the entire code for Search.php:

quote:

require_once( "global.php" );

$kernel->clean_array( "_REQUEST", array( "page" => V_PINT, "limit" => V_PINT, "sort" => V_STR, "order" => V_STR, "start" => V_PINT, "category_id" => V_INT, "string" => V_STR ) );

if( $kernel->vars['page'] == 0 ) $kernel->vars['page'] = 1;
if( $kernel->vars['limit'] == 0 ) $kernel->vars['limit'] = $kernel->config['display_default_limit'];
if( empty( $kernel->vars['sort'] ) ) $kernel->vars['sort'] = $kernel->config['display_default_sort'];
if( empty( $kernel->vars['order'] ) ) $kernel->vars['order'] = $kernel->config['display_default_order'];

if( $kernel->vars['category_id'] > 0 )
{
$kernel->vars['page_struct']['system_page_navigation_id'] = $kernel->vars['category_id'];
}

$kernel->vars['page_struct']['system_page_navigation_html'] = array( "search.php?string={$kernel->vars['string']}" => sprintf( $kernel->ld['lang_f_searching'], $kernel->vars['string'] ) );
$kernel->vars['page_struct']['system_page_action_title'] = sprintf( $kernel->ld['lang_f_page_title_search'], $kernel->vars['string'] );

if( $kernel->session_function->read_permission_flag( 3, true ) == true )
{
$kernel->tp->call( "search" );

if( !empty( $kernel->vars['string'] ) )
{
$file_ranking_syntax = "";

// Prepare search query
if( $kernel->config['archive_search_mode'] == 1 )
{
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$kernel->vars['string']}*' IN BOOLEAN MODE )";
}
else
{
$search_syntax = "MATCH( f.file_name, f.file_description, f.file_version, f.file_author ) AGAINST ( '*{$kernel->vars['string']}*' )";
}

if( $kernel->vars['category_id'] > 0 )
{
$search_category = "AND f.file_cat_id = {$kernel->vars['category_id']}";
}

if( $kernel->vars['sort'] == "file_ranking" )
{
$file_ranking_syntax = ", ( f.file_rating / f.file_votes ) AS file_ranking";
}

//Basic search query
$check_files = $kernel->db->query( "SELECT f.* {$file_ranking_syntax} FROM " . TABLE_PREFIX . "files f WHERE {$search_syntax} {$search_category} AND f.file_disabled = 0" );

//No results
if( $kernel->db->numrows() == 0 )
{
$kernel->page_function->message_report( $kernel->ld['lang_f_search_no_files'], M_NOTICE );
}
else
{
$kernel->archive_function->construct_pagination_vars( $kernel->db->numrows( $check_files ) );

$kernel->vars['html']['file_custom_fields_headers'] = "";
$total_fields = 0;
$last_file_status = 0;

$kernel->tp->call( "search_results_header" );

// Run paginated search query
$get_files = $kernel->db->query( "SELECT f.* {$file_ranking_syntax} FROM " . TABLE_PREFIX . "files f WHERE {$search_syntax} {$search_category} AND f.file_disabled = 0 ORDER BY f.file_pinned DESC, {$kernel->vars['sort']} {$kernel->vars['order']}, f.file_name LIMIT {$kernel->vars['start']}, {$kernel->vars['limit']}" );

// Check for custom fields
$get_fields = $kernel->db->query( "SELECT * FROM `" . TABLE_PREFIX . "fields` WHERE `field_category_display` = 1 ORDER BY `field_name`" );

if( $kernel->db->numrows( $get_fields ) > 0 )
{
while( $field = $kernel->db->data( $get_fields ) )
{
$kernel->vars['html']['file_custom_fields_headers'] .= $kernel->tp->call( "file_field_subheader", CALL_TO_PAGE );
$kernel->vars['html']['file_custom_fields_headers'] = $kernel->tp->cache( "field_name", $field['field_name'], $kernel->vars['html']['file_custom_fields_headers'] );

$total_fields++;
}
}

// Manually alter the end value if any COLUMNS are manually added to the template
$fields['file_total_columns'] = $total_fields + 5;

$kernel->tp->call( "file_header" );

$kernel->tp->cache( $fields );

// fetch and display category files
while( $file = $kernel->db->data( $get_files ) )
{
if( $last_file_status == 1 AND $file['file_pinned'] == 0 )
{
$kernel->tp->call( "file_row_break" );
}
$last_file_status = $file['file_pinned'];

$kernel->tp->call( "file_row" );

$file['file_icon'] = $kernel->archive_function->construct_file_icon( $file['file_dl_url'], $file['file_icon'] );
$file['file_rank'] = $kernel->archive_function->construct_file_rating( $file['file_rating'], $file['file_votes'] );
$file['file_description'] = $kernel->archive_function->return_string_words( $kernel->format_input( $file['file_description'], T_NOHTML ), $kernel->config['string_max_length'] );
$file['file_custom_fields'] = $kernel->archive_function->construct_file_custom_fields( $file['file_id'] );

$file['file_timestamp'] = $kernel->fetch_time( $file['file_timestamp'], DF_SHORT );
$file['file_mark_timestamp'] = $kernel->fetch_time( $file['file_mark_timestamp'], DF_SHORT );

$file['file_size'] = $kernel->format_input( $file['file_size'], T_NUM );
$file['file_author'] = $kernel->format_input( $file['file_author'], T_HTML );
$file['file_downloads'] = $kernel->format_input( $file['file_downloads'], T_NUM );
$file['file_views'] = $kernel->format_input( $file['file_views'], T_NUM );
$file['file_votes'] = $kernel->format_input( $file['file_votes'], T_NUM );
$file['file_name'] = $kernel->format_input( $file['file_name'], T_NOHTML );

$file['file_prefix'] = ( $file['file_pinned'] == 1 ) ? $kernel->ld['lang_f_title_pinned'] : "";

$kernel->tp->cache( $file );
}

$kernel->tp->call( "file_footer" );

// Print pagination
$kernel->vars['pagination_urls'] = array(
"nextpage" => "search.php?string={$kernel->vars['string']}&category_id={$kernel->vars['category_id']}&sort={$kernel->vars['sort']}&order={$kernel->vars['order']}&limit={$kernel->vars['limit']}&page={\$nextpage}",
"previouspage" => "search.php?string={$kernel->vars['string']}&category_id={$kernel->vars['category_id']}&sort={$kernel->vars['sort']}&order={$kernel->vars['order']}&limit={$kernel->vars['limit']}&page={\$previouspage}",
"span" => "search.php?string={$kernel->vars['string']}&category_id={$kernel->vars['category_id']}&sort={$kernel->vars['sort']}&order={$kernel->vars['order']}&limit={$kernel->vars['limit']}&page={\$page}",
"start" => "search.php?string={$kernel->vars['string']}&category_id={$kernel->vars['category_id']}&sort={$kernel->vars['sort']}&order={$kernel->vars['order']}&limit={$kernel->vars['limit']}&page=1",
"end" => "search.php?string={$kernel->vars['string']}&category_id={$kernel->vars['category_id']}&sort={$kernel->vars['sort']}&order={$kernel->vars['order']}&limit={$kernel->vars['limit']}&page={$kernel->vars['total_pages']}",
);

$kernel->page_function->construct_pagination_menu( R_FILE, false, SCRIPT_NAME );
}
}

$kernel->page_function->construct_category_list( $kernel->vars['category_id'] );
}

$kernel->page_function->construct_output( R_HEADER, R_FOOTER, false, R_NAVIGATION );

?>


Am I supposed to be looking in Search.php for the lines I need to edit? Or elsewhere?

Also, the actual URL after a search looks like below:
http://mysite.com/r/search.php?string=kent



walterquez -> RE: Preventing SQL Injection in PHP Script (4/11/2008 11:38:06 PM)

It looks like $kernel was defined in global.php.

Giving me a sample URL also helped, thanks.

Look for one of these two, $_REQUEST['string'] or $_GET['string'] inside the global.php file.

Don't be surprised if it is not in the global.php file. That file may call other files too. You may have to search for it inside the other files. I hope there are not too many.

Either way, I'll help as much as I can.



walterquez -> RE: Preventing SQL Injection in PHP Script (4/12/2008 12:30:37 AM)

I downloaded the file and took a peek at the code. I don't think you would have found it, since they're using a variable variable. Basically, a dynamic variable that can be any name.

Open the following file.
quote:

upload/include/class_kernel.php


replace line 215
quote:

case V_STR: $variable_data = trim( $variable_data ); break;

with this
quote:

case V_STR: $variable_data = trim( $variable_data ); // break;
$variable_data = str_replace( '"', '', $variable_data );
$variable_data = str_replace( "'", '', $variable_data );
break;


I hope this works for you.



NeoGenesis -> RE: Preventing SQL Injection in PHP Script (4/12/2008 1:40:30 AM)

No, you were right... I was trying to find it with no luck.

I replaced the line, did a search to make sure it still worked and it did.
Then I tried to execute the exploit and it didn't work!

Thank you so much! You have no idea how long I was trying to fix this!

Putting up a Christian site was a little scary (with most of the opposing views willing to go to any length to shut you up) and this vulnerability had been bothering me for quite some time.

Thanks a lot for taking the time to help me!



walterquez -> RE: Preventing SQL Injection in PHP Script (4/12/2008 8:45:27 AM)

You're welcome.

Of course there are other ways too, but not knowing how the creator wrote the program, and follow every logic, I thought this would be the easiest solution.

There is a function in PHP called, mysql_real_escape_string, that prepares the string before executing it in mysql. I would probably have used this instead. But, you need the resource link of the connection to make this work.

And then there is also addslashes when you're saving it, and stripslashes when reading it, but need to make sure there are not any slashes already in place.



PreserveWildlife -> RE: Preventing SQL Injection in PHP Script (4/13/2008 7:18:47 AM)

There are many solutions. The simplest is to just use stored procedures and not build up sql strings.



walterquez -> RE: Preventing SQL Injection in PHP Script (4/13/2008 10:25:31 AM)

Stored Procedures are good since they keep the logic in the database rather than the client, but you still need to prepare the string before execution.



LoyalGypsy -> RE: Preventing SQL Injection in PHP Script (4/13/2008 12:33:10 PM)

quote:

ORIGINAL: walterquez

Stored Procedures are good since they keep the logic in the database rather than the client, but you still need to prepare the string before execution.



Greetings Walter,

Walter, I have just download SQL server 2005
I have a question, can you point me to a site that contains sample strings??

I am new at this, basically I am going to learn database managment over the next couple of years.

I heard it is a good thing to have on ones resume...IT


LG



walterquez -> RE: Preventing SQL Injection in PHP Script (4/13/2008 8:52:10 PM)

You can always check Microsoft own website about their SQL server at http://msdn2.microsoft.com/en-us/sqlserver/default.aspx



PreserveWildlife -> RE: Preventing SQL Injection in PHP Script (4/13/2008 10:04:52 PM)

quote:

ORIGINAL: walterquez

Stored Procedures are good since they keep the logic in the database rather than the client, but you still need to prepare the string before execution.

Not necessarily. if you're using parameters and not using dynamic sql in the stored proc you don't need to worry.



Page: [1]



Forum Software © ASPPlayground.NET Advanced Edition 2.5 ANSI