Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Correctly escape single quotes in json paths #28160

Merged
merged 3 commits into from Apr 10, 2019

Conversation

Projects
None yet
7 participants
@brendt
Copy link
Contributor

brendt commented Apr 10, 2019

There's a potential SQL injection vulnerability with the JSON query syntax. This PR fixes that.

Laravel will parse JSON paths to json_extract functions. Say you've got the following:

$builder->select('json_field->field');

This will be parsed to:

json_extract(`json_field`, '$."field"')

The actual parsing is done in \Illuminate\Database\Query\Grammars\Grammar::wrapJsonFieldAndPath()

It is however possible to provide a single quote as the "field" value, which will close the json_extract early. This gives an attacker the possibility to insert his own malicious SQL code. Take for example this input (indented for clarity):

lang->**"')), migrations.*

FROM users
    RIGHT OUTER JOIN migrations ON migrations.id <> null

#

By manually inserting ' after lang->**", we're able to break out of the json_extract function and inject our malicious code.

SELECT JSON_UNQUOTE(JSON_EXTRACT(`lang`, '$."**"')), migrations.*
FROM users
RIGHT OUTER JOIN migrations ON migrations.id <> null
#"')) from `users`

In this example we're joining on the migrations table, but it's possible to join on anything.

In order for this attack to work, two requirements have to be met:

  • The attacker must control the columns of a query. Chances are not a lot of people are doing this manually in their projects. Though there are popular packages which expose this functionality to provide easy API endpoints. A popular example is the JSON API spec, which specifically allows for sparse fieldsets.
  • The entry point table must have a column with JSON data. Otherwise the json_extract` function will fail, stopping the query. From the entry point though, you can access all other tables.

The solution is to escape all single quotes passed as $value in \Illuminate\Database\Query\Grammars\Grammar::wrapJsonPath(). Because attackers could potentially chain multiple backslashes, this PR will take all single quotes, with or without preceding backslashes, and replace it with \'.

I decided to use a HEREDOC in the tests, for clarity. If this is not ok for Laravel, I'll be happy to change it.

@brendt brendt force-pushed the brendt:escape-json-path branch from 5167fc6 to be1896c Apr 10, 2019

brendt added some commits Apr 10, 2019

@jmarcher

This comment has been minimized.

Copy link
Contributor

jmarcher commented Apr 10, 2019

Thanks for pointing at this, but please next time send an email to: taylor@laravel.com directly for all security vulnerabilities as described in the readme.md

@staudenmeir

This comment has been minimized.

Copy link
Contributor

staudenmeir commented Apr 10, 2019

@jmarcher This vulnerability has been disclosed and discussed privately a while ago.

Taylor decided not to fix it, so it's public now: https://murze.be/an-important-security-release-for-laravel-query-builder

@@ -1119,6 +1119,8 @@ protected function wrapJsonFieldAndPath($column)
*/
protected function wrapJsonPath($value, $delimiter = '->')
{
$value = preg_replace("/([\\\\]+)?\\'/", "\\'", $value);

This comment has been minimized.

Copy link
@staudenmeir

staudenmeir Apr 10, 2019

Contributor

With preg_replace("/(\\\\)*'/", ...), the tests still pass. Is there a difference?

This comment has been minimized.

Copy link
@brendt

brendt Apr 10, 2019

Author Contributor

You sure? Because it won't match \', this would cause only the ' to be escaped. Resulting in \\' as the end result, which in turn allows for the attack.

This comment has been minimized.

Copy link
@staudenmeir

staudenmeir Apr 10, 2019

Contributor

Don't the tests check this?

This comment has been minimized.

Copy link
@vlakoff

vlakoff Apr 13, 2019

Contributor

Same thoughts as staudenmeir, preg_replace("/\\\\*'/", ...) appears to be a strictly equivalent code. But simpler thus less error-prone.

I guess you got a bit confused between PHP and regex escaping ;)

This comment has been minimized.

Copy link
@vlakoff

vlakoff Apr 13, 2019

Contributor

Describing the code:

  • replace: quote preceded by 0 or more backslashes
  • with: quote preceded by one backslash
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Apr 10, 2019

Note that you should never allow users to control the columns of your query without a white list. PDO does not allow binding column names as parameters and thus we can't offer much real protection there.

@taylorotwell taylorotwell merged commit a056cd8 into laravel:5.8 Apr 10, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Apr 10, 2019

Even with this fix, I DO NOT encourage anyone to allow users to dictate the columns of their query without a white list.

@brendt brendt deleted the brendt:escape-json-path branch Apr 10, 2019

@brendt

This comment has been minimized.

Copy link
Contributor Author

brendt commented Apr 10, 2019

@taylorotwell I agree that these kinds of scenarios should be avoided at all costs. Have you considered adding a general warning to the docs?

Doctrine, for example, states the following:

The following APIs are designed to be SAFE from SQL injections:

For Doctrine\DBAL\Connection#insert($table, $values, $types), Doctrine\DBAL\Connection#update($table, $values, $where, $types) and Doctrine\DBAL\Connection#delete($table, $where, $types) only the array values of $values and $where. The table name and keys of $values and $where are NOT escaped.

https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/security.html

The Laravel documentation states the follwing:

The Laravel query builder uses PDO parameter binding to protect your application against SQL injection attacks. There is no need to clean strings being passed as bindings.

https://laravel.com/docs/master/queries#introduction

This statement can be confusing for beginners and even seasoned developers (our query builder package is a good example of that).

I think it would be good to add a clear warning about the use of user input as column names.

@GeraldPK

This comment has been minimized.

Copy link

GeraldPK commented Apr 11, 2019

Since this is considered a security fix, will it be backported to 5.7 and 5.5?

ian-nisbet added a commit to ian-nisbet/framework that referenced this pull request Apr 11, 2019

ian-nisbet added a commit to ian-nisbet/framework that referenced this pull request Apr 11, 2019

ian-nisbet added a commit to ian-nisbet/framework that referenced this pull request Apr 11, 2019

Tests for Json Path Escaping
Test for security update copied into 5.6 branch from 5.8 branch laravel#28160
See laravel@a056cd8
@GavG

This comment has been minimized.

Copy link

GavG commented Apr 12, 2019

Since this is considered a security fix, will it be backported to 5.7 and 5.5?

Looks like 'wrapJsonPath' was only introduced in 5.6

@staudenmeir

This comment has been minimized.

Copy link
Contributor

staudenmeir commented Apr 12, 2019

@GavG The wrapJsonPath() method was added in Laravel 5.6, but the vulnerability itself has been around since Laravel 5.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.