★ An important security release for laravel-query-builder
Tags: laravel, php
Published: 2020-01-02 23:20:38

---
title: ★ An important security release for laravel-query-builder
published: true
date: 2019-04-09 21:16:13 UTC
tags: laravel, php
canonical_url: https://freek.dev/1317-an-important-security-release-for-laravel-query-builder
---

Our [laravel-query-builder](https://github.com/spatie/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.

```php
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:

```php
User::orderBy('email')->get();
```

And Laravels internal query builder will transform that to the following SQL query:

```sql
select * from `users` order by `email` asc
```

That’s all working as intended. However, the problem occurs when somebody enter the following sort query:

```html
https://mysite.com/users?sort=email->"%27))%23injectedSQL
```

Under the hood our package will transform that to this:

```php
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.

```sql
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](https://github.com/spatie/laravel-query-builder-security-issue-poc/blob/d28d40fd3814c49eee3e83e282396fd225e88cf5/tests/Unit/SqlInjectionTest.php#L12-L23) 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](https://github.com/spatie/laravel-query-builder/commit/3aa483b63c79d9fabcb8653fe837a7736eb93bea) 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](https://laravel.com/docs/master/queries#introduction) 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](https://laravel.com/docs/master/queries#introduction) was added in the laravel docs.

Many thanks to my colleagues [Alex](https://twitter.com/alexvanderbist) and [Brent](https://twitter.com/brendt_gd) for investigating this and to Jonas Staudenmeir for reporting the original issue. Also thank you [Nuno Maduro](https://twitter.com/enunomaduro) and [Dries Vints](https://twitter.com/driesvints) for proofreading this blogpost.