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

SQL injection or bad documentation #2428

Closed
noguespi opened this Issue Oct 7, 2013 · 8 comments

Comments

Projects
None yet
4 participants
@noguespi

noguespi commented Oct 7, 2013

The Laravel documentation states :

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

The Query Builder only protect the where('', '') binding statment but not the others. By others parameters I mean orderBy, groupBy, limit and so.

If a user follow the Laravel documentation, he may think the following statement are secured :

DB::table('users')->select('email')->limit(Input::get('injection'));

or with Eloquent:

User::groupBy(Input::get('injection'))->get();

However they are not secure and they lead to sql injection, the user must sanitize the input before usage. (intval() for limit, in_array() of valid columns for order by and group by).

So I think the Laravel documentation should be more complete about this problem.

EDITED: removed reference to DBAL/Doctrine because it's not used but the problem is still here.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 7, 2013

Member

We don't use Doctrine DBAL for query building at all. It is only used for renaming columns.

Member

taylorotwell commented Oct 7, 2013

We don't use Doctrine DBAL for query building at all. It is only used for renaming columns.

@noguespi

This comment has been minimized.

Show comment
Hide comment
@noguespi

noguespi Oct 7, 2013

You closed to fast, forgot about DBAL/Doctrine, and reread the issue please.

noguespi commented Oct 7, 2013

You closed to fast, forgot about DBAL/Doctrine, and reread the issue please.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 7, 2013

Member

Ah, right, limit and offset. Yes, we can add a note about that.

Member

taylorotwell commented Oct 7, 2013

Ah, right, limit and offset. Yes, we can add a note about that.

@taylorotwell taylorotwell reopened this Oct 7, 2013

@noguespi

This comment has been minimized.

Show comment
Hide comment
@noguespi

noguespi Oct 7, 2013

limit, offset, orderBy, groupBy (and probably some others, I don't know perfectly your query builder).

noguespi commented Oct 7, 2013

limit, offset, orderBy, groupBy (and probably some others, I don't know perfectly your query builder).

@noguespi

This comment has been minimized.

Show comment
Hide comment
@noguespi

noguespi Oct 7, 2013

The better would be to say the binding in where/like statement are safe to use without sanitization but not the others.

Or we could make the ORM/QueryBuilder more secure by removing backtick/backquote in orderBy/groupBy/... (or triggering an exception) and forcing cast to int in limit()/offset() ?

noguespi commented Oct 7, 2013

The better would be to say the binding in where/like statement are safe to use without sanitization but not the others.

Or we could make the ORM/QueryBuilder more secure by removing backtick/backquote in orderBy/groupBy/... (or triggering an exception) and forcing cast to int in limit()/offset() ?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 8, 2013

Member

Added note to documentation. Added casting to int for limit and offset.

Member

taylorotwell commented Oct 8, 2013

Added note to documentation. Added casting to int for limit and offset.

@crissi

This comment has been minimized.

Show comment
Hide comment
@crissi

crissi Oct 9, 2013

Contributor

why not ecape orderby and group by?

Contributor

crissi commented Oct 9, 2013

why not ecape orderby and group by?

@shithappend

This comment has been minimized.

Show comment
Hide comment
@shithappend

shithappend Nov 11, 2017

because useres input does not have any role on that statement

shithappend commented Nov 11, 2017

because useres input does not have any role on that statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment