Our laravel-query-builder package exposed a serious security issue: it allowed SQL injection attacks. Laravel Query Builder v1.17.1, which is now available, fixes the vulnerability. If you're using the package, stop reading now and upgrade to the latest version first. For Laravel 5.6, 5.7 and 5.8 you're safe if you use v1.17.1. For Laravel 5.5 you should be on 1.16.1. Any other version is considered unsafe.
The problem
laravel-query-builder allows you to filter, sort and include eloquent relations based on the request.
Imagine you want to get all users sorted by email:
https://mysite.com/users?sort=email
On the endpoint, you can use our package to build up a query based in URL parameters.
use Spatie\QueryBuilder\QueryBuilder;
$users = QueryBuilder::for(User::class)->get();
// all `User`s sorted by email
Under the hood, we'll transform it to this:
User::orderBy('email')->get();
And Laravels internal query builder will transform that to the following SQL query:
select * from `users` order by `email` asc
That’s all working as intended. However, the problem occurs when somebody enter the following sort query:
https://mysite.com/users?sort=email->"%27))%23injectedSQL
Under the hood our package will transform that to this:
User::orderBy('email->"%27))%23injectedSQL')->get();
You might think that you would get a MySQL error because you're ordering on a non-existing column, but that's not the case. Laravel will convert the code above to this SQL query. The produced query might seem strange on first sight, but it’s 100% valid, and MySQL will execute it.
select * from `users` order by json_unquote(json_extract(`email`, '$.""'))#injectedSQL"')) asc
Laravel will parse the -> in the URL and replace with those JSON MySQL functions. The "%27)) part of the query will allow us to break out of the open string and causes the function to be closed early after which we can add any SQL statement we want. In this case, we just added a friendly comment. But somebody with ill intentions can add any malicious code they want there.
In this repository you’ll find a test that demonstrates the issue.
Fixing this issue
We fixed this issue by only allowing alphanumeric characters, - and _ anywhere the package handles column names. Here's the relevant commit made by my colleague Alex.
In closing
You might think that Laravel is to blame because it fails to produce a safe MySQL query. But in my opinion, this isn’t Laravels fault. The documentation states you shouldn't clean strings passed as bindings.
The Laravel query builder uses PDO parameter binding to protect your application against SQL injection attacks. There is no need to clean strings passed as bindings.
I can imagine that there are apps out there that pass field_name in a request like https://mysite.com/some-resource?orderby=field_name to the orderBy function. Whenever you specify column names in the query, it's your responsibility that you pass a safe string. In my opinion, the Laravel documentation could stress this fact more.
EDIT: meanwhile a warning was added in the laravel docs.
Many thanks to my colleagues Alex and Brent for investigating this and to Jonas Staudenmeir for reporting the original issue. Also thank you Nuno Maduro and Dries Vints for proofreading this blogpost.
Top comments (0)