Skip to content

[5.8] Strip slashes for unique rule helper #27940

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

Merged
merged 1 commit into from
Mar 20, 2019
Merged

[5.8] Strip slashes for unique rule helper #27940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Illuminate/Validation/Concerns/ValidatesAttributes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,8 @@ public function validateUnique($attribute, $value, $parameters)

if (isset($parameters[2])) {
[$idColumn, $id] = $this->getUniqueIds($parameters);

$id = stripslashes($id);
}

// The presence verifier is responsible for counting rows within this store
Expand Down
8 changes: 4 additions & 4 deletions src/Illuminate/Validation/Rules/Unique.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ public function ignore($id, $idColumn = null)
return $this->ignoreModel($id, $idColumn);
}

$this->ignore = str_replace(',', '', $id);
$this->idColumn = str_replace(',', '', $idColumn ?? 'id');
$this->ignore = addslashes($id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Stupid q: is this escaping so the value can be literally used in a DB query, where no further escaping happens?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using addslashes() to escape a value for a DB query is quite dangerous.

Copy link
Contributor

@mfn mfn Mar 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

My thoughts, thus I'm asking. Not very familiar with the exact code, though.

But I know at least for Postgres it doesn't even work correctly.

Copy link
Member

@taylorotwell taylorotwell Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We aren't adding slashes for the DB query. We are adding slashes for the str_getcsv call in the validator. This has nothing to do with trying to escape data for a DB query.

To clarify, the vulnerability is that a user can construct a string that is sent through the str_getcsv call by the validator and uses a column of their choosing in the DB query. The value is bound as a PDO parameter. However, columns are not. Therefore, we need to ensure that the "id" portion of the unique validation rule is a single string and can't be manipulated with commas / quotes to create multiple values when parsed by str_getcsv.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for clarification.

I think however that the code could use some comments as to why all these special handling.

Copy link

Choose a reason for hiding this comment

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

Because a vulnerability was reported. It was possible to perform a XSS using the ID

Copy link
Member

Choose a reason for hiding this comment

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

It's not XSS.

Copy link

@Jurigag Jurigag Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I know this is old article but i guess it tells you why addslashes is not perfect solution here - http://shiflett.org/blog/2006/addslashes-versus-mysql-real-escape-string Not sure though how it's realted to what @taylorotwell wrote, i guess someone needs to check it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that Taylor already clarified, this is not for escaping for the database but for another case.

Also, the code already changed in the meantime and the addslashes was moved to the __toString and only for the ignore and not the column.

$this->idColumn = $idColumn ?? 'id';

return $this;
}
Expand All @@ -50,8 +50,8 @@ public function ignore($id, $idColumn = null)
*/
public function ignoreModel($model, $idColumn = null)
{
$this->idColumn = str_replace(',', '', $idColumn ?? $model->getKeyName());
$this->ignore = str_replace(',', '', $model->{$this->idColumn});
$this->idColumn = addslashes($idColumn ?? $model->getKeyName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would addslashes be relevant for the column, which I would expect is never a user provided input?

Copy link

Choose a reason for hiding this comment

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

Hmm I wonder why a “user” would request anything with any special characters.... maybe because they are maliciously trying things to a site?

Copy link
Contributor

@deleugpn deleugpn Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think mfn's point is the same as the one Taylor made in the blog post: the developer using Laravel should never expose the ability for their user to define the database column through an input.
Worst case scenario the user input should be decoupled from the database, which would never be vulnerable.

Copy link
Contributor

@gocanto gocanto Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

this could have simple avoided by type-hinting the method

$this->ignore = $model->{$this->idColumn};

return $this;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/Validation/ValidationUniqueRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public function testItCorrectlyFormatsAStringVersionOfTheRule()
$rule = new Unique('table', 'column');
$rule->ignore('Taylor, Otwell', 'id_column');
$rule->where('foo', 'bar');
$this->assertEquals('unique:table,column,"Taylor Otwell",id_column,foo,bar', (string) $rule);
$this->assertEquals('unique:table,column,"Taylor, Otwell",id_column,foo,bar', (string) $rule);

$rule = new Unique('table', 'column');
$rule->ignore(null, 'id_column');
Expand Down