Jump to content

Sanitising old PHP functions with functions and params from functions in sql


Adamhumbug

Recommended Posts

HI All,

I am going through some old code and trying to sanitise sql injection problems and need some help on the best method here.

The project does come with some caveats, which i cant change.

I have to use Mysqli and cannot use PDO.  Because the project is so large, i am going to need to deal with all code changes inside the function and not changing or removing other functions.

I always appreciate the help on my personal stuff that i am doing and you guys are amazing but please on this one can we move past the fact that the code is rubbish and old and full of problems.

I have a sql query that looks like this - as you can see there are variables in here as well as functions

"select op_id, ".$opsrch['op_fullname'].", op_role 
from prs_op 
where ".$opsrch['srch'].(is_numeric($srch) ? " 
                         or op_id='".$srch."' 
                         or op_id like '".$srch."%'" : "")." 
                         order by ".$opsrch['sort']." 
                         limit 40";

I know how to write a prepared statement but i cannot do this simply as some of the variables contain more sql rather than just a simple variable.

Once the sql and all of the variables and functions have been evaluated, this is what i get

select op_id, CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) as op_fullname, op_role 
from prs_op 
where (CONCAT(TRIM(op_firstname),' ',TRIM(op_middlenames),' ',TRIM(op_lastname)) like 'adam h%' 
       or CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) like 'adam h%' 
       or CONCAT(TRIM(op_middlenames),' ',TRIM(op_lastname)) like 'adam h%' 
       or op_lastname like 'adam h%') 
order by op_firstname, op_lastname 
limit 40

All references to adam have been passed in from the front end.

I have been looking at this -https://stackoverflow.com/questions/56135017/building-a-dynamic-php-prepared-statement-from-user-entry

but that is putting everything in one place and im not sure if it can work here.

I would really appreciate any help with info to tidy this mess - thanks in advance for helping me with crappy code.

 

Link to comment
Share on other sites

in your example, does $opsrch['srch'] contain all that WHERE ... sql that also contains the $srch value and if so, where is $opsrch['srch'] built relative to the code you have posted?

what php version will this be used with, because as of php8.1, you can simply supply an array of values to the ->execute([...]) call end eliminate all the explicit input binding and the type format string. all you would need to do is build an array with the input values that corresponds to the sql that is being built. edit: as long as all the values being a string data type are okay. i.e. won't work with a LIMIT clause which requires integers.

If you are not using php8.1+, to convert this to a prepared query, using mysqli, you need to build the type format string and an array of input parameters that matches the sql that is being built.

Edited by mac_gyver
Link to comment
Share on other sites

43 minutes ago, Adamhumbug said:

HI All,

I am going through some old code and trying to sanitise sql injection problems and need some help on the best method here.

The project does come with some caveats, which i cant change.

I have to use Mysqli and cannot use PDO.  Because the project is so large, i am going to need to deal with all code changes inside the function and not changing or removing other functions.

I always appreciate the help on my personal stuff that i am doing and you guys are amazing but please on this one can we move past the fact that the code is rubbish and old and full of problems.

I have a sql query that looks like this - as you can see there are variables in here as well as functions

"select op_id, ".$opsrch['op_fullname'].", op_role 
from prs_op 
where ".$opsrch['srch'].(is_numeric($srch) ? " 
                         or op_id='".$srch."' 
                         or op_id like '".$srch."%'" : "")." 
                         order by ".$opsrch['sort']." 
                         limit 40";

I know how to write a prepared statement but i cannot do this simply as some of the variables contain more sql rather than just a simple variable.

Once the sql and all of the variables and functions have been evaluated, this is what i get

select op_id, CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) as op_fullname, op_role 
from prs_op 
where (CONCAT(TRIM(op_firstname),' ',TRIM(op_middlenames),' ',TRIM(op_lastname)) like 'adam h%' 
       or CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) like 'adam h%' 
       or CONCAT(TRIM(op_middlenames),' ',TRIM(op_lastname)) like 'adam h%' 
       or op_lastname like 'adam h%') 
order by op_firstname, op_lastname 
limit 40

All references to adam have been passed in from the front end.

I have been looking at this -https://stackoverflow.com/questions/56135017/building-a-dynamic-php-prepared-statement-from-user-entry

but that is putting everything in one place and im not sure if it can work here.

I would really appreciate any help with info to tidy this mess - thanks in advance for helping me with crappy code.

 

If it is numeric i get this.

 

select op_id, CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) as op_fullname, 
op_role from prs_op 
where (CONCAT(TRIM(op_firstname),' ',TRIM(op_middlenames),' ',TRIM(op_lastname)) like '9999%' 
       or CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) like '9999%' 
       or CONCAT(TRIM(op_middlenames),' ',TRIM(op_lastname)) like '9999%' 
       or op_lastname like '9999%') 
       or op_id='9999' 
       or op_id like '9999%' 
       order by op_firstname, op_lastname 
       limit 40

 

Link to comment
Share on other sites

4 minutes ago, mac_gyver said:

in your example, does $opsrch['srch'] contain all that WHERE ... sql that also contains the $srch value and if so, where is $opsrch['srch'] built relative to the code you have posted?

what php version will this be used with, because as of php8.1, you can simply supply an array of values to the ->execute([...]) call end eliminate all the explicit input binding and the type format string. all you would need to do is build an array with the input values that corresponds to the sql that is being built.

If you are not using php8.1+, to convert this to a prepared query, using mysqli, you need to build the type format string and an array of input parameters that matches the sql that is being built.

$opsrch['srch'] contains

"(CONCAT(TRIM(op_firstname),' ',TRIM(op_middlenames),' ',TRIM(op_lastname)) like '".apos($sterm)."%' or CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) like '".apos($sterm)."%' or CONCAT(TRIM(op_middlenames),' ',TRIM(op_lastname)) like '".apos($sterm)."%' or op_lastname like '".apos($sterm)."%')";

 

Link to comment
Share on other sites

6 minutes ago, mac_gyver said:

in your example, does $opsrch['srch'] contain all that WHERE ... sql that also contains the $srch value and if so, where is $opsrch['srch'] built relative to the code you have posted?

what php version will this be used with, because as of php8.1, you can simply supply an array of values to the ->execute([...]) call end eliminate all the explicit input binding and the type format string. all you would need to do is build an array with the input values that corresponds to the sql that is being built.

If you are not using php8.1+, to convert this to a prepared query, using mysqli, you need to build the type format string and an array of input parameters that matches the sql that is being built.

I believe that it is php 7.3.

Link to comment
Share on other sites

9 minutes ago, mac_gyver said:

in your example, does $opsrch['srch'] contain all that WHERE ... sql that also contains the $srch value and if so, where is $opsrch['srch'] built relative to the code you have posted?

what php version will this be used with, because as of php8.1, you can simply supply an array of values to the ->execute([...]) call end eliminate all the explicit input binding and the type format string. all you would need to do is build an array with the input values that corresponds to the sql that is being built.

If you are not using php8.1+, to convert this to a prepared query, using mysqli, you need to build the type format string and an array of input parameters that matches the sql that is being built.

My issue with this is that i will have to have several arrays as i am putting the code in several places in the query which i struggled to work out when referencing the stack overflow post i quoted.

Link to comment
Share on other sites

you can build the sql query syntax with the same usage as now, with the $opsrch elements.

what would change when converting to a prepared query will involve putting ? place-holders in the sql query statement where each value is at, building a type format string, and an array of variables that correspond to the place-holders. if your code is structured so that you are building parts of the query separately, you would build separate type format strings and arrays of input variables for each section, then conditionally concatenate the separate parts of the type format string and conditionally use array_merge() to produce the complete array of inputs that are supplied to the bind_param() statement.  

another subtle detail, any wild-card characters must be concatenated with the value in the variable, since the bind_param() uses references to the variables.

Link to comment
Share on other sites

it doesn't seem like when the search is for a numerical value, that the name columns should be searched? will any of the name columns ever start with a numerical value?

based on the snippets of the implied problem, i would implement this like the following -

$opsrch['op_fullname'] = "CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) as op_fullname";
$opsrch['sort'] = "op_firstname, op_lastname";

// the search input from wherever it is coming from
$srch = '9999'; // a numerical value
$srch = 'adam h'; // a string value

// since mysqli requires variables in the bind_param statement, produce the search with a trailing wild-card in its own variable
$sterm = "$srch%";

$types = []; // array to hold the type format strings
$params = []; // array to hold the input variables
$where_terms = []; // array to hold the where terms

if(!is_numeric($srch))
{
	// do a name search
	$types[] = 's';
	$params[] = $sterm;
	$where_terms[] = "CONCAT(TRIM(op_firstname),' ',TRIM(op_middlenames),' ',TRIM(op_lastname)) like ?";

	$types[] = 's';
	$params[] = $sterm;
	$where_terms[] = "CONCAT(TRIM(op_firstname),' ',TRIM(op_lastname)) like ?";

	$types[] = 's';
	$params[] = $sterm;
	$where_terms[] = "CONCAT(TRIM(op_middlenames),' ',TRIM(op_lastname)) like ?";

	$types[] = 's';
	$params[] = $sterm;
	$where_terms[] = "op_lastname like ?";
}
else
{
	// do an id search
	$types[] = 'i';
	$params[] = $srch;
	$where_terms[] = "op_id= ?";
	
	$types[] = 's';
	$params[] = $sterm;
	$where_terms[] = "op_id like ?";
}

// build the sql query
$sql = 
"SELECT op_id, {$opsrch['op_fullname']}, op_role 
	FROM prs_op 
	WHERE ".implode(' OR ',$where_terms)." 
	ORDER BY {$opsrch['sort']} 
	LIMIT 40";

// examine the result
echo $sql;
echo '<br>';
echo implode($types);
echo '<pre>';
print_r($params);

 

Link to comment
Share on other sites

This has worked for one of my queries but actually for the one in the example it has not.

Your example would require that i re-write all of the output of all of the queries that come back from the other functions that are run.

Is there a way for me to do this without having to manually evaluate the functions and then hard code what they produce for the prepared statements?

Link to comment
Share on other sites

Just now, Adamhumbug said:

This has worked for one of my queries but actually for the one in the example it has not.

Your example would require that i re-write all of the output of all of the queries that come back from the other functions that are run.

Is there a way for me to do this without having to manually evaluate the functions and then hard code what they produce for the prepared statements?

Sorry to be more clear about what i mean - the where_terms i dont really want to have to write out explicitly if there is a way around it.

I dont know if there is a good way of doing this, especially as they include params that have been passed in from the front end.  Can you think of another way or do you think i am beat?

Link to comment
Share on other sites

because you only posted a small snippet of the problem, you got a small snippet of a solution. if you want a complete top down solution, you will need to post all the code that demonstrates the extent and limits of this problem.

at a minimum, the code for each part of the query will need to be re-written so that it produces the sql query syntax, with the prepare query place-holders in it, the type format string for that part of the query, and an array of the input parameters for that part of the query. you can then merge the  dynamically built parts together to produce the compete set of data needed to prepare, bind the input parameters, and execute the query. all the cases where there is conditional logic inside the sql query. e.g. the ternary operator, will need to be rewritten so that the logic can conditionally merge the three pieces of data for each part of the query, together in the correct order.

have you actually used the mysqli extension with a prepared query so that you know what the end result will need to be?

Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.