Addressing SQL Injection Vulnerabilities with Raw Methods.

[craigfrancis]

While the documentation notes "Laravel can not guarantee that any query using raw expressions is protected against SQL injection vulnerabilities", unfortunately not everyone will read/understand/remember this; and even the best developers make mistakes.

While Injection Vulnerabilities are becoming rarer (thanks to database abstractions like yours), they have not been solved, and these mistakes are still found in penetration tests (as reflected in the OWASP Top 10). They also tend to go un-reported, as it's kinda embarrassing, especially for developers who think they are more "experienced" and believe they should use the "powerful" methods for everything (sigh).

A simple example - I found someone had used $sort = $request->input('sort') with ->orderBy($sort), all good, fairly normal. But a junior developer had been asked to split the name into first/last fields, so $sort was set to name_first,name_last; this caused an SQL error (good), but instead of updating their code to call ->orderBy() for each field, they simply changed it to ->orderByRaw($sort), it "worked", and was uploaded to the live site.

And then you have these (abbreviated) classics, would you say these mistakes are obvious?

$users = DB::table('user')->whereRaw('CONCAT(name_first, " ", name_last) LIKE "' . $search . '%"'); // INSECURE
$users = DB::table('user')->whereRaw('CONCAT(name_first, " ", name_last) LIKE ?', $search . '%');

$users = DB::select('SELECT * FROM users WHERE active = ' . $_GET['active']); // INSECURE
$users = DB::select('SELECT * FROM users WHERE active = ?', [$_GET['active']]);

I believe Laravel should provide a way to check unsafe user input is not used with these sensitive parameters... i.e. these parameters should expect a "developer defined string", known as the literal-string type in Static Analysis tools PHPStan and Psalm (also supported by PhpStorm 2022.3).

This is used by Nette, Propel, RedBean; and it does find mistakes.

As an aside, Pradeep Srinivasan and Graham Bleaney (Facebook/Meta) introduced the LiteralString type in Python 3.11; and this technique can be used in other programming languages.

---

I can put together a PR, but want to check that you (the Laravel developers) want to address this issue, and are happy with this approach, e.g.

--- a/src/Illuminate/Database/Query/Builder.php
+++ b/src/Illuminate/Database/Query/Builder.php
@@ -1024,7 +1024,7
     /**
      * Add a raw where clause to the query.
      *
-     * @param  string  $sql
+     * @param  literal-string  $sql
      * @param  mixed  $bindings
      * @param  string  $boolean
      * @return $this
      */
     public function whereRaw($sql, $bindings = [], $boolean = 'and')
@@ -2353,7 +2353,7
     /**
      * Add a raw "order by" clause to the query.
      *
-     * @param  string  $sql
+     * @param  literal-string  $sql
      * @param  array  $bindings
      * @return $this
      */
     public function orderByRaw($sql, $bindings = [])

I'd prefer to start with a few of the simpler methods (to get some feedback), before we look at methods like DB::raw(), and DB::select().

[Niki-Crawley]

Huh, that would solve some issues - last week I found selectRaw() being mis-used (don't ask).

Have you found any issues with this approach? maybe false-positives?

[craigfrancis]

Hi @Niki-Crawley; yeah, unfortunately these mistakes happen quite often.

Good question about issues, oddly I've not really found any so far... I've used this technique on 5 Laravel projects (via stubs), and it found a few more vulnerabilities I'd missed during my manual code reviews; and it highlighted 1 bit code that was less than ideal, where a developer had used strtoupper() for some unknown reason.

[tpetry]

That's a good approach! It will make potential SQL injection vulnerabilities a lot less likely if teams use PHPStan to analyze their code. And with my different PRs for query expressions, we can now even get rid of those raw expressions entirely and replace them with safe escaped generated sql constructs.

[craigfrancis]

Yep, agreed. While it would be nice for people to not use the Raw methods at all (i.e. using something safer, like your Query Expressions), the literal-string type works with how code exists today, and identifies mistakes that lead to Injection Vulnerabilities.