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.

Sign up for GitHub

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

Jump to bottom

[6.x] Limit expected bindings #35865

Merged
merged 2 commits into from Jan 13, 2021
+31 −21
Merged

[6.x] Limit expected bindings #35865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7ccd2b
limit expected bindings
taylorotwell Jan 13, 2021
02e304b
limit more bindings
taylorotwell Jan 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/Illuminate/Database/Query/Builder.php
View file
Open in desktop
Expand Up @@ -698,7 +698,7 @@ public function where($column, $operator = null, $value = null, $boolean = 'and'
);

if (! $value instanceof Expression) {
$this->addBinding($value, 'where');
$this->addBinding(is_array($value) ? head($value) : $value, 'where');
}

return $this;
Expand Down Expand Up @@ -1043,7 +1043,7 @@ public function whereBetween($column, array $values, $boolean = 'and', $not = fa

$this->wheres[] = compact('type', 'column', 'values', 'boolean', 'not');

$this->addBinding($this->cleanBindings($values), 'where');
$this->addBinding(array_slice($this->cleanBindings($values), 0, 2), 'where');

return $this;
}
Expand Down Expand Up @@ -1111,6 +1111,8 @@ public function whereDate($column, $operator, $value = null, $boolean = 'and')
$value, $operator, func_num_args() === 2
);

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

if ($value instanceof DateTimeInterface) {
$value = $value->format('Y-m-d');
}
Expand Down Expand Up @@ -1150,6 +1152,8 @@ public function whereTime($column, $operator, $value = null, $boolean = 'and')
$value, $operator, func_num_args() === 2
);

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

if ($value instanceof DateTimeInterface) {
$value = $value->format('H:i:s');
}
Expand Down Expand Up @@ -1189,6 +1193,8 @@ public function whereDay($column, $operator, $value = null, $boolean = 'and')
$value, $operator, func_num_args() === 2
);

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

if ($value instanceof DateTimeInterface) {
$value = $value->format('d');
}
Expand Down Expand Up @@ -1232,6 +1238,8 @@ public function whereMonth($column, $operator, $value = null, $boolean = 'and')
$value, $operator, func_num_args() === 2
);

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

if ($value instanceof DateTimeInterface) {
$value = $value->format('m');
}
Expand Down Expand Up @@ -1275,6 +1283,8 @@ public function whereYear($column, $operator, $value = null, $boolean = 'and')
$value, $operator, func_num_args() === 2
);

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

if ($value instanceof DateTimeInterface) {
$value = $value->format('Y');
}
Expand Down Expand Up @@ -1583,7 +1593,7 @@ public function whereJsonLength($column, $operator, $value = null, $boolean = 'a
$this->wheres[] = compact('type', 'column', 'operator', 'value', 'boolean');

if (! $value instanceof Expression) {
$this->addBinding($value);
$this->addBinding((int) $value);
}

return $this;
Expand Down Expand Up @@ -1732,7 +1742,7 @@ public function having($column, $operator = null, $value = null, $boolean = 'and
$this->havings[] = compact('type', 'column', 'operator', 'value', 'boolean');

if (! $value instanceof Expression) {
$this->addBinding($value, 'having');
$this->addBinding(is_array($value) ? head($value) : $value, 'having');
}

return $this;
Expand Down
34 changes: 17 additions & 17 deletions tests/Database/DatabaseQueryBuilderTest.php
View file
Open in desktop
Expand Up @@ -301,24 +301,24 @@ public function testBasicWheres()
public function testWheresWithArrayValue()
{
$builder = $this->getBuilder();
$builder->select('*')->from('users')->where('id', [12, 30]);
$builder->select('*')->from('users')->where('id', [12]);
$this->assertSame('select * from "users" where "id" = ?', $builder->toSql());
$this->assertEquals([0 => 12, 1 => 30], $builder->getBindings());

$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());

$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());

$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());
$this->assertEquals([0 => 12], $builder->getBindings());

// $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());

// $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());

// $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 •
edited

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 •
edited

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.

#35894

Copy link

@jaylinski jaylinski Jan 14, 2021 •
edited

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

Copy link

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?

Copy link

Choose a reason for hiding this comment

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

@genesiscz https://kanecohen.com/blog/laravel-security-update-jan-13/

Copy link
Contributor

@mpyw mpyw Jan 21, 2021 •
edited

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.

Copy link

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.

}

public function testMySqlWrappingProtectsQuotationMarks()
Expand Down