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

[6.x] Limit expected bindings #35865

Merged
merged 2 commits into from
Jan 13, 2021
Merged

[6.x] Limit expected bindings #35865

merged 2 commits into from
Jan 13, 2021

Conversation

taylorotwell
Copy link
Member

Limit the bindings to the number of bindings we actually expect for the given query.

@GrahamCampbell GrahamCampbell changed the title Limit expected bindings [6.x] Limit expected bindings Jan 13, 2021
@taylorotwell taylorotwell merged commit d0954f4 into 6.x Jan 13, 2021
@taylorotwell taylorotwell deleted the limit-expected-bindings branch January 13, 2021 13:35
// $builder = $this->getBuilder();
// $builder->select('*')->from('users')->where('id', '<>', [12, 30]);
// $this->assertSame('select * from "users" where "id" <> ?', $builder->toSql());
// $this->assertEquals([0 => 12, 1 => 30], $builder->getBindings());
Copy link
Contributor

@u01jmg3 u01jmg3 Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentional? (To clarify I'm talking about the ^^ above ^^ commented out tests)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This test checks something that is a security vulnerability works.

Copy link
Contributor

@u01jmg3 u01jmg3 Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's commented out... 🤷🏻‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@jaylinski jaylinski Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@u01jmg3 As far as I can tell, the whereIn()-function should be used when querying if a value is in an array:

https://github.com/laravel/framework/blob/v8.22.1/src/Illuminate/Auth/DatabaseUserProvider.php#L118-L122

Since devs may encounter situations where they don't know if the argument is a string or an array, it is convenient to be able to use where() with an array.

But apparently this convenience function can cause unexpected behavior which can be exploited: https://blog.laravel.com/security-laravel-62011-7302-8221-released

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly would such behaviour be exploitable? In the worst case, it should throw an error, shouldn't it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@mpyw mpyw Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taylorotwell @driesvints @GrahamCampbell @KaneCohen @jaylinski @genesiscz

Do you really think that the following patch fixes the vulnerability? I don't think so.

$value = is_array($value) ? head($value) : $value;

According to Laravel Security Advisory - January 13 2021 | Kane Cohen, the following exploitation should work.

// HTTP Request Query: https://laravel.com/users?id[]=1&id[]=1
$id = Request::input('id');
User::where('id', $id)->where('is_admin', 0)->first();
// This could lead to a query where "is_admin" column is set to 1.

After the patch, attackers can also do like this:

// HTTP Request Query: https://laravel.com/users?id[0][]=1&id[0][]=1
$id = Request::input('id');
User::where('id', $id)->where('is_admin', 0)->first();
// This could lead to a query where "is_admin" column is set to 1.

Since PHP input arrays can be nested multiple times, this is not a fundamental fix.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a validation this that you should do before, as it is a standard to always validate your data before using them , and Laravel Validation rules can validate values to check it is a string or array or boolean and can also do casts, but more importantly it can know when an array is inserted instead of a string for example. so this should be a valid Fix for this error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants