|
Users viewing this topic:
none
|
|
Login | |
|
Preventing SQL Injection in PHP Script - 4/6/2008 7:01:44 PM
|
|
|
NeoGenesis
Posts: 6
Joined: 3/21/2006
Status: offline
|
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!
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/8/2008 10:59:07 PM
|
|
|
TheNextBillGates
Posts: 53
Joined: 4/11/2005
Status: offline
|
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.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/9/2008 8:42:05 PM
|
|
|
NeoGenesis
Posts: 6
Joined: 3/21/2006
Status: offline
|
Ok, thanks. But how exactly should that look? I don't really write PHP very well.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/9/2008 10:58:04 PM
|
|
|
TheNextBillGates
Posts: 53
Joined: 4/11/2005
Status: offline
|
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!
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/10/2008 12:54:00 AM
|
|
|
NeoGenesis
Posts: 6
Joined: 3/21/2006
Status: offline
|
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.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/10/2008 8:01:31 AM
|
|
|
iluvatar
Posts: 1275
Joined: 4/12/2005
Status: offline
|
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.
_____________________________
It's not that I'm lazy, it's that I just don't care.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/10/2008 10:22:51 PM
|
|
|
TheNextBillGates
Posts: 53
Joined: 4/11/2005
Status: offline
|
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.
< Message edited by TheNextBillGates -- 4/10/2008 10:35:47 PM >
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/11/2008 2:02:13 AM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
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.
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/11/2008 11:23:24 AM
|
|
|
NeoGenesis
Posts: 6
Joined: 3/21/2006
Status: offline
|
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?
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/11/2008 6:41:52 PM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
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.
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/11/2008 6:52:41 PM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
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
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/11/2008 10:38:46 PM
|
|
|
NeoGenesis
Posts: 6
Joined: 3/21/2006
Status: offline
|
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
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/11/2008 11:38:06 PM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
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.
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/12/2008 12:30:37 AM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
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 215quote:
case V_STR: $variable_data = trim( $variable_data ); break; with thisquote:
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.
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/12/2008 1:40:30 AM
|
|
|
NeoGenesis
Posts: 6
Joined: 3/21/2006
Status: offline
|
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!
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/12/2008 8:45:27 AM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
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.
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/13/2008 7:18:47 AM
|
|
|
PreserveWildlife
Posts: 1429
Joined: 4/13/2005
From: Tennessee
Status: offline
|
There are many solutions. The simplest is to just use stored procedures and not build up sql strings.
_____________________________
Neil's Photo Tips
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/13/2008 10:25:31 AM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
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.
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/13/2008 12:33:10 PM
|
|
|
LoyalGypsy
Posts: 1507
Joined: 4/12/2005
Status: offline
|
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
_____________________________
Ex 19:5 Now therefore, if you will indeed obey My voice ...So the Persians ask that the 300 drop their arms. Leonidas responds; "Persians! Come and get them!" 300 The Movie
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/13/2008 8:52:10 PM
|
|
|
walterquez
Posts: 1398
Joined: 4/12/2005
Status: offline
|
You can always check Microsoft own website about their SQL server at http://msdn2.microsoft.com/en-us/sqlserver/default.aspx
_____________________________
St. Athanasius the Great For our Canons and our forms were not given to the Churches at the present day, but were wisely and safely transmitted to us from our forefathers.
|
|
|
|
RE: Preventing SQL Injection in PHP Script - 4/13/2008 10:04:52 PM
|
|
|
PreserveWildlife
Posts: 1429
Joined: 4/13/2005
From: Tennessee
Status: offline
|
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.
_____________________________
Neil's Photo Tips
|
|
|
|
New Messages |
No New Messages |
Hot Topic w/ New Messages |
Hot Topic w/o New Messages |
Locked w/ New Messages |
Locked w/o New Messages |
|
Post New Thread
Reply to Message
Post New Poll
Submit Vote
Delete My Own Post
Delete My Own Thread
Rate Posts |
|
|