2

Reading the Laravel documentation I see that:

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.

Does that still apply if I only craft queries in the following manner?

DB::query("SELECT * from table WHERE id like " . $id);
WildBill
  • 9,143
  • 15
  • 63
  • 87
  • Why do you use `like` operator here? – zerkms Apr 03 '14 at 23:04
  • Yes I know I could use Wuery Builder for such a simple query but there are times when I have to run a query again a string I dynamically build. – WildBill Apr 03 '14 at 23:08
  • why cannot you build a query using query builder dynamically? – zerkms Apr 03 '14 at 23:09
  • It is my understanding that I cannot build a query where the number of WHERE clauses is defined at runtime. For example, I may have anywhere from 1-5 things I need to evaluate at runtime. – WildBill Apr 03 '14 at 23:11
  • `$query = DB::table('table'); if (...) { $query->where(...) }` ? Why cannot you run `->where()` in runtime? What's special with this method? – zerkms Apr 03 '14 at 23:11
  • So I could do that if I knew how many possible where clauses I would need. If I needed anywhere from 1 - 20 I would have to write 20 IF statements, correct? For example, if the user sends one parameter I have one where clause. If they send two I have two where clauses. etc. – WildBill Apr 03 '14 at 23:14
  • New question addressing this issue found here: http://stackoverflow.com/questions/22850774/can-you-use-query-builder-to-build-a-query-with-a-dynamic-where-clause-at-runtim – WildBill Apr 03 '14 at 23:20
  • I honestly don't see **any** difference between assembling an sql string in runtime or calling some methods. – zerkms Apr 03 '14 at 23:20

3 Answers3

4

Let's take that sentence and emphasise the key phrase:

There is no need to clean strings being passed as bindings.

In your example, $id is not being passed as a binding, it is just being injected into the raw SQL, so it is not protected.

You should follow standard practice for preventing SQL injection:

  • in cases like this, where the input is always an integer, you could use intval($id)
  • you could get the underlying PDO object with DB::getPdo()/DB::getReadPdo() and use PDO::quote() to correctly escape strings
  • although the documentation is rather poor, Laravel's DB facade can run fully parameterised queries, such as DB::select('SELECT * FROM users WHERE users.id = ?', array($userId));

Parameterised queries are usually considered the gold standard in injection prevention, and are what Eloquent is using internally when you use the query builder. The idea is that you first give the database (or, at minimum, the database driver) the complete query with no user input at all, so there is no doubt which tables and columns should be in use. You then pass in the user input as completely separate data, which is never actually written into the SQL, just applied to the query you already sent.

Parameterised queries can't do everything for you, though - for instance, most libraries, including PDO, can't bind a table or column name as a parameter. That's because it will actually create a different query every time it is run, negating the separation between query and data. If you want to do that, you therefore need some other method of ensuring safety - usually, a whitelist of allowed values is the best idea.

Community
  • 1
  • 1
IMSoP
  • 89,526
  • 13
  • 117
  • 169
  • Ok, I'll confess: What does 'string being passed as bindings' mean? :) – WildBill Apr 03 '14 at 23:22
  • 1
    @WildBill It means passing the variable representing user input as a separate variable to the DB code from the SQL. This is the essence of "parameterised queries" - outside of frameworks, you will often see people recommending the use of [`PDO::prepare()`](http://php.net/manual/en/pdo.prepare.php) to prepare an SQL statement with placeholders in. The database can see which parts of the query are intended to be table and column names, functions, etc, and which is user input, so there is not way for a malicious user to make input act as SQL. – IMSoP Apr 03 '14 at 23:26
  • @WildBill I've added some more info to my answer. – IMSoP Apr 03 '14 at 23:37
  • So what do you do IF the user input does define the columns? For example, if you use JQGrid the user will define the columns one will search on. If you have a grid that has 4 columns and the user can chose 0-4 of those columns, there are 16 different possible combinations they can send. – WildBill Apr 03 '14 at 23:39
  • @WildBill You don't need to worry about the different combinations, just that there are no columns mentioned in their input that aren't one of the 4 you expected. – IMSoP Apr 03 '14 at 23:42
  • Think I get it. Just one last question. You say "If you want to do that, you therefore need some other method of ensuring safety - usually, a whitelist of allowed values is the best idea." I assume that means I cannot use a user-define val for a column name in the DB::SELECT statement you have above? – WildBill Apr 03 '14 at 23:50
  • @WildBill You can't use `?` as a placeholder for a column name. You could write `DB::select("SELECT * FROM users WHERE $columnName = ?", array($value))` but you would want to first check that `$columnName` was what you were expecting - for instance `if ( ! in_array($columnName, array('id', 'name', 'address')) { die('Bad input!); }` – IMSoP Apr 03 '14 at 23:53
  • Got it. Doing so would still ensure the values in the array are clean from SQL injection, right? – WildBill Apr 04 '14 at 00:05
  • @WildBill The array is something you hard-code. There's no way for a string to simultaneously be equal to `'id'` and contain an attack on your database. – IMSoP Apr 04 '14 at 00:18
  • Sorry, meant to say this array: DB::select("SELECT * FROM users WHERE $columnName = ?", array($value)) – WildBill Apr 04 '14 at 00:20
  • @WildBill Ah, I see, yes. Think of it from the point of view of the `DB::select` call: it doesn't know whether you hard-coded the whole SQL query, or built part of it dynamically, it just sees the string `'SELECT * FROM users WHERE id = ?'`, which contains some SQL to prepare straight away, and a placeholder for where the user input will get applied. – IMSoP Apr 04 '14 at 00:22
1

No, DB::query() is not part of the concept of Query Builder. Instead, this will be protected:

DB::table('table')->where('id', 'like', $id)->get();
Octavian
  • 4,519
  • 8
  • 28
  • 39
0

But the best way to protect your queries is to use Query Builder at its max:

DB::table('table')->where('id', 'like', $id)->get();

Another way to protect your queries, if you really are forced to write a raw query, is to cast your data to the type they are supposed to be:

DB::query(DB::raw("SELECT * from table WHERE id like " . (int) $id));

In this case if $id is 'some exploit' the query will be:

SELECT * from table WHERE id like 0

In Query Builder you can also pass your parameters (bindings) like this to harden the security of your queries:

DB::select('SELECT * FROM users WHERE users.id = ?', array($userId));
Antonio Carlos Ribeiro
  • 86,191
  • 22
  • 213
  • 204
  • Why do you think it is protected? – zerkms Apr 03 '14 at 23:01
  • Not completely, the best way to protect your queries is to use Query Builder at its max. – Antonio Carlos Ribeiro Apr 03 '14 at 23:02
  • Right. And OP asked if it's protected or not. "Not completely" --> "Definitely not" – zerkms Apr 03 '14 at 23:02
  • Let's say I just say NO to him, right? Would I really force him to write only things using QueryBuilder? It's more probable that in the first time he needs a raw query he might just create something really unsecure for his app. IMHO it's better to teach him how to be secure if there is no other way. Laravel is still not perfect in terms of database queries and sometimes you are just forced to do thing the way you would not do. Take a simple `select count(*) as total` as an example, as far as I can tell you have to do it by using `select(DB::raw('count(*) as total'))`. – Antonio Carlos Ribeiro Apr 03 '14 at 23:16
  • What protection are you expecting `DB::raw` in your first example to provide? Since you're providing it a string, and explicitly telling Laravel that it's raw SQL, any bad data in `$id` will already have been injected. – IMSoP Apr 03 '14 at 23:20
  • @IMSoP, you are right, sir. I just take a look at `Illuminate\Database\Query\Expression` (DB::raw) and it really does nothing to protect against injections. Removed that part. – Antonio Carlos Ribeiro Apr 03 '14 at 23:27