Preventing SQL injection in PHP Code (code included)

Question

First timer here, so I'll try to be as thorough as possible.

I'm a rather new programmer and started working at a new company where I've been attempting to prevent some SQL injection in a specific piece of PHP code that was leftover from an old project, but is needed in my rewrite of it.

This project is in Laravel as a backend and AngularJS as the frontend.

I've attempted several ways to prevent the SQL injection but have gotten the same error in all of them.

Error:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'll.created_at BETWEEN '?' AND '?' UNION SELECT 1 as type, ll.id, ll.name, ll.ema' at line 1

Here is the original code in question:

   if (!empty($request['inst'])) {
        $institution = $request['inst'];
        $inst = ' ll.inst_id = ' . $request['inst'] . ' AND ';
        $instCount = ' customers.inst_id = :inst AND ';
        $instEvent = ' location_id = :inst AND ';
    } else {
        $inst = ' ';
        $instCount = ' ';
        $instEvent = ' ';
        $institution = "";
    }
    $startTime = $request['start'];
    $endTime = $request['end'];

$sql = " SELECT 0 as type ,ll.id as lead_name, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM customers ll LEFT JOIN leads_statistics ls  ON ll.id = ls.lead_id"
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ll.created_at BETWEEN '" . $request['start'] . "' AND '" . $request['end']
            . "' UNION SELECT 1 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.meeting_set_first BETWEEN '" . $request['start'] . "' AND '" . $request['end']  
            . "' UNION SELECT 2 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.is_active = 1 AND ls.occurred_date BETWEEN '" . $request['start'] . "' AND '" . $request['end']
            . "' UNION SELECT 3 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.is_active = 1 AND ls.not_show_date BETWEEN '" . $request['start'] . "' AND '" . $request['end']
            . "' UNION SELECT 4 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.is_active = 1 AND ls.cancel_meeting_date BETWEEN '" . $request['start'] . "' AND '" . $request['end']
            . "' UNION SELECT 5 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.is_active = 1 AND ls.sale_date BETWEEN '" . $request['start'] . "' AND '" . $request['end']
            . "' UNION SELECT 6 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.is_active = 1 AND ls.cancel_sale_date BETWEEN '" . $request['start'] . "' AND '" . $request['end']
            . "' UNION SELECT 7 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.extended_date BETWEEN '" . $request['start'] . "' AND '" . $request['end']
            . "' UNION SELECT 8 as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id "
            . " LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE $inst ls.is_active = 1 AND ls.meeting_set_date BETWEEN '" . $request['start'] . "' AND '" . $request['end'] . "'";

$leads = DB::select($sql);

Here are some of the techniques I've used after perusing the internet and SO at length.

Placeholders (? instead of all the variables and creating an array at the end with all the variables 9 times over)
Named Bindings example: DB::select($sql, ["inst" => $institution, "start" => $startTime, "end" => $endTime,])
Named bindings Named bindings with PDO::ATTR_EMULATE_PREPARES => true, so I can use :inst, :start, :end multiple times
SQL variable bindings with @ symbols

Results of techniques:

No protection - works fine, I receive the proper results
Placeholders - I get the above error, but the SQL has the proper data in the placeholders. I can take the resulting SQL and run it through the phpmyadmin server SQL query and I receive the data as expected.
Named Bindings - Doesn't work at all. I get an error stating SQLSTATE[HY093]: Invalid parameter number: parameter was not defined
Named Bindings withPDO::ATTR_EMULATE_PREPARES => true, - works, however it affects all the data I then pull from my server turning things such as numbers to strings or worse. This plays havoc with typing all over the place
SQL variable binding - didn't work either, received the above error

I am unsure if it's possible to turn this giant SQL query into Laravel Eloquent or Query Builder, though I have tried.

Anyone have any ideas? Any help would be immensely appreciated, I've been wrestling with this for a few weeks now.

Try to only use placeholders where you set your variables (like $request['inst']) — N69S, Dec 25 '19 at 14:03
Does this answer your question? [How can I prevent SQL injection in PHP?](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) — ChrisGPT was on strike, Dec 25 '19 at 14:19
@Chris Unfortunately not, I've tried placeholders and named bindings to no effect. I get an error stating `SQLSTATE[HY093]: Invalid parameter number: parameter was not defined` — Ari Cashriel, Dec 25 '19 at 14:24
Why do you have 8 referencess to every table here? Seems like the query could also use some changes itself. — Thom A, Dec 25 '19 at 14:27
@Larnu It's an old query from someone else's work. I'm open to suggestions on how to change it as each case type is very similar with 1 or 2 differences between each case. Each case is pulling customer information, but each one has a different filter. Would splitting all these unions into separate SQL's be a bad idea? — Ari Cashriel, Dec 25 '19 at 14:31
Would have to have a closer look at the query; but I suspect you could do it with 1 or 2 references. — Thom A, Dec 25 '19 at 15:20
Nevermind, I just noticed that this has *nothing* to do with SQL Server... You've tagged it, but the error is clearly MySQL. Please don't tag products you aren't using. — Thom A, Dec 25 '19 at 15:37

N69S · Accepted Answer · 2019-12-25T15:15:52.687

$institution = $request['inst']??'';

$start = $request['start'];
$end = $request['end'];
$joins = [
    1 => ['date_field' => 'meeting_set_first', 'is_active' => false],
    2 => ['date_field' => 'occurred_date', 'is_active' => true],
    3 => ['date_field' => 'not_show_date', 'is_active' => true],
    4 => ['date_field' => 'cancel_meeting_date', 'is_active' => true],
    5 => ['date_field' => 'sale_date', 'is_active' => true],
    6 => ['date_field' => 'cancel_sale_date', 'is_active' => true],
    7 => ['date_field' => 'extended_date', 'is_active' => false],
    8 => ['date_field' => 'meeting_set_date', 'is_active' => true],
];

$sql = 'SELECT 0 as type ,ll.id as lead_name, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM customers ll LEFT JOIN leads_statistics ls  ON ll.id = ls.lead_id LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE '.($institution?" ll.inst_id = :inst0 AND ":'').' ll.created_at BETWEEN :start0 AND :end0 UNION ';
if ($institution) {
    $placeHolders[':inst0'] = $institution;
}
$placeHolders[':start0'] = $start;
$placeHolders[':end0'] = $end;

$queryJoin = [];
foreach($joins as $key => $join) {
    $queryJoin[] = 'SELECT '.$key.' as type, ll.id, ll.name, ll.email, ll.updated_at, u.employee_name, ls.meeting_set_date, ls.meeting_set_first FROM leads_statistics ls LEFT JOIN customers ll ON ls.lead_id = ll.id LEFT JOIN users u ON ll.userUpdate = u.employee_id  WHERE '.($institution?" ll.inst_id = :inst$key AND ":'').($join['is_active']?' ls.is_active = 1 AND ':'').' ls.'.$join['date_field'] ." BETWEEN :start$key AND :end$key";
    if ($institution) {
        $placeHolders[':inst'.$key] = $institution;
    }
    $placeHolders[':start'.$key] = $start;
    $placeHolders[':end'.$key] = $end;
}

$sql .= implode(' UNION ', $queryJoin);

$leads = DB::select($sql, $placeHolders);

Hey, I've tried this solution (as I mentioned in the OP), but receive an error stating 'SQLSTATE[HY093]: Invalid parameter number: parameter was not defined '. I was under the impression that you cannot reuse placeholders more than once and need to define them again if you do ex:DB::select($sql, [':inst' => $institution, ':start' => $request['start'], ':end' => $request['end'],':inst2' => $institution,]); and so on — Ari Cashriel, Dec 25 '19 at 14:20
Thanks! Your edited answer worked. Marked it as the answer. This logic will also help me out with another SQL issue I have that is very similar. Thank you very much! — Ari Cashriel, Dec 25 '19 at 16:11

Preventing SQL injection in PHP Code (code included)

1 Answers1