[5.2] Wrap field name in MySQL JSON path expressions

[sbusch]

Without this, MySQL throws an error e.g. when using reserved names as field names.

At least in theory, this should also reduce attack surface for SQL injection! This weakness may apply to other database grammars (Postgres?), not tested.

Example behavior without this fix:

~/Code/backend$ php artisan tinker

Psy Shell v0.7.2 (PHP 7.0.3-13+deb.sury.org~trusty+1 — cli) by Justin Hileman

>>> App\User::where('values->test', "1")->get()

Illuminate\Database\QueryException with message 'SQLSTATE[42000]: Syntax
error or access violation: 1064 You have an error in your SQL syntax; check the
manual that corresponds to your MySQL server version for the right syntax to
use near '->"$.test" = ?' at line 1 (SQL: select * from `users` where
values->"$.test" = 1)'

[sbusch]

Just noticed the failing tests, fixing it...

[taylorotwell]

@themsaid thoughts?

[themsaid]

Yes correct, an error will be thrown if the field name is not wrapped for preserved keys.

select order->"$.en" from products LIMIt 1 // OK
select order->"$.en" from products LIMIt 1 // Syntax error

[taylorotwell]

So you're saying this should be merged @themsaid ?

[themsaid]

I think this can be simplified:

protected function wrapJsonSelector($value)
    {
        $path = explode('->', $value);

        return '`'.array_shift($path).'`->'.'"$.'.implode('.', $path).'"';
    }

No need to call wrapValue() again.

[GrahamCampbell]

Wrapping the value surely isn't sufficient. We'd need to use prepared statements?

[themsaid]

I'm not at my disk now, I may look into that in detail later today.

[sbusch]

@themsaid sure, proposed simplification works for now, but each future improvement to wrapValue() - which may be security-related, for example when applying/changing escaping - must then be incorporated here.

@GrahamCampbell As this PR is about wrapping column names: doesn't the mechanism of prepared statements apply to values only? Are prepared statements also applied to column names?

[taylorotwell]

We wouldn't use prepared statements for column names @GrahamCampbell

[taylorotwell]

I don't see how this is related to SQL injection at all in any way shape or form.

[themsaid]

It's only to wrap column names to be safe from conflicting with reserved keywords.

[sbusch]

@taylorotwell Before this PR, code like

App\User::where($request->get('searchfield').'->selector', 'value')->delete()

was vulnerable to SQL injection as it circumvented wrapValue(), which wraps everything in backticks and escapes any contained backticks.

I know such code is a dangerous idea but now it's correctly wrapped (escaped). There's still whereRaw for anyone intentionally writing dangerous code. 😈

Examples:

Query as intended: ?searchfield=foo

delete from `users` where foo->selector=value

Malicious query: ?searchfield=%3Dfoo+or+foo

SQL injection without fix:

delete from `users` where foo=foo or foo->selector=value

With fix a regular SQL error should be triggered:

delete from `users` where `foo=foo or foo`->selector=value

In addition, the attacker cannot break out of the backticks because wrapValue() escapes any contained backticks.

If you think I'm correct then similar implementations for other grammars, e.g. PostgreSQL, could be vulnerable aswell and should be checked (I don't know PostgreSQL well enough)

Note: I'm currently working on additionally wrapping/escaping the JSON path for the same reason.