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

[Adamhumbug]

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.

 

[mac_gyver]

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 July 7, 2023 by mac_gyver

[Adamhumbug]

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

 

[Adamhumbug]

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)."%')";

 

[Adamhumbug]

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.

[Adamhumbug]

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.

[mac_gyver]

array_merge()

[Adamhumbug]

1 hour ago, mac_gyver said:

array_merge()

So the SQL would end up looking something like?

 

"select op_id, ".$arr[0].", op_role 
from prs_op 
where ".$arr[1] $arr[2] $arr[3]." 
                         limit 40";

 

[mac_gyver]

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.

[mac_gyver]

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);

 

[Adamhumbug]

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?

[Adamhumbug]

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?

[mac_gyver]

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?

[cyberRobot]

On 7/7/2023 at 8:51 AM, Adamhumbug said:

I believe that it is php 7.3.

Note that you can check with phpversion()
https://www.php.net/manual/en/function.phpversion.php

In case you're not aware, support for PHP 7.3 ended in December 2021.
https://www.php.net/supported-versions.php