63

I just inherited a project because the last developer left. The project is built off of Code Igniter. I've never worked with Code Igniter before.

I took a quick look at the code and I see database calls in the controller like this:

$dbResult = $this->db->query("SELECT * FROM users WHERE username = '".$_POST['user_name']."'");

or calls like this:

$dbResult = $this->db->query("SELECT * FROM users WHERE username = '".$this->input->post('username')."'");

Does code igniter automatically sanitize these queries to prevent sql injection?

Abdulla Nilam
  • 36,589
  • 17
  • 64
  • 85
John
  • 32,403
  • 80
  • 251
  • 422

12 Answers12

72

CodeIgniter DOES ESCAPE the variables you pass by when using the $this->db->query method. But ONLY when you pass the variables as binds, here's an example:

$dbResult = $this->db->query("SELECT * FROM users WHERE username = ?", array($this->input->post('username')));

Also remember that $_POST shouldn't be preferred over $this->input->post since what it does is check if the variables exists to prevent errors.

MarioRicalde
  • 9,131
  • 6
  • 40
  • 42
  • 2
    I think there should be a 2nd parameter passed to the $this->input->post() function which basically tells if the input is to be sanitized or not. `$dbResult = $this->db->query("SELECT * FROM users WHERE username = '?'", array($this->input->post('username', TRUE)));` – thephpx Jun 06 '11 at 06:33
  • 7
    This is no longer valid. Now CodeIgniter escapes everything as long as you use their activerecords. – David 天宇 Wong Feb 05 '13 at 20:02
  • 8
    @David天宇Wong The fact that you can use Active Record as an *alternative* solution doesn't really stop this answer being valid. The code in the question is using neither solution, so is open to SQL injection. – IMSoP Mar 04 '14 at 18:07
  • 1
    The question mark should not be quoted. "SELECT * FROM users WHERE username = ?". Otherwise it will pass the question mark as parameter, not the value itself – Afrig Aminuddin Oct 18 '19 at 03:21
20

CodeIgniter provides a few string escaping functions in its database layer.

Excerpt from CI Manual:

It's a very good security practice to escape your data before submitting it into your database. CodeIgniter has three methods that help you do this:

  1. $this->db->escape() This function determines the data type so that it can escape only string data. It also automatically adds single quotes around the data so you don't have to:

    $sql = "INSERT INTO table (title) VALUES(".$this->db->escape($title).")";

I'd post the other two examples, but I wouldn't want to take all the fun out of reading the manual.

approxiblue
  • 6,982
  • 16
  • 51
  • 59
John Himmelman
  • 21,504
  • 22
  • 65
  • 80
  • 28
    +1 for *"but I wouldn't want to take all the fun out of reading the manual."* :) -- a sweeter version of RTFM.. – DMin May 13 '11 at 07:43
  • Link broken: Is this a suitable replacement? http://ellislab.com/codeigniter/user-guide/database/queries.html – Prusprus Jan 30 '14 at 23:59
14

No, the code you posted is susceptible to SQL injection. You need to use query binding to construct your SQL queries. If you're using the CI DB library, you would code it something like this (example from the user guide):

$sql = "SELECT * FROM some_table WHERE id = ? AND status = ? AND author = ?";

$this->db->query($sql, array(3, 'live', 'Rick'));
Funkatron
  • 912
  • 5
  • 13
  • 3
    @David天宇Wong Please clarify. Are you saying that CI will magically escape the input in the code provided (despite not having any way to know it is being used in an SQL context)? Or just that query binding is no longer the recommended approach to this problem? – IMSoP Mar 04 '14 at 18:05
  • 2
    man I have no idea, I posted this a year ago and I'm using django now. But I admit I should have wroten why, well now it's too late. – David 天宇 Wong Mar 05 '14 at 12:58
3

No, CodeIgniter will not magically sanitize queries which have been built like this.

Ben James
  • 121,135
  • 26
  • 193
  • 155
3

According to CI's docs here, the framework filters POST on controller construction. It also optionally does XSS filtering either by manually calling the function or setting a global config.

I've never used CI either except just to play with it, so I'm not sure how far I'd trust this.

Josh Lindsey
  • 8,455
  • 3
  • 24
  • 25
  • 2
    XSS filtering and SQL injection are not the same thing. There is no way to correctly escape for SQL until you know what DB you are connecting to (and, ideally, its current locale), so this will always be part of a DB layer, not an input layer. – IMSoP Mar 04 '14 at 18:02
2

Use active record for safety and easier coding:

Rather than:

  $dbResult = $this->db->query("SELECT * FROM users WHERE username'".$_POST['user_name']."'");

Use (same result):

$this->db->where('username',$this->input->post('user_name'));
$dbResult = $this->db->get('users');
Teej
  • 12,764
  • 9
  • 72
  • 93
Rid Iculous
  • 3,696
  • 3
  • 23
  • 28
2

That doesn't escape anything. You are better off changing it to the bind syntax or the active record syntax

Teej
  • 12,764
  • 9
  • 72
  • 93
2

You should use $this->input->post, query binding and active record to have the safer data and then still, test test test to be sure.

stef
  • 26,771
  • 31
  • 105
  • 143
0

Optimized with a second post param (TRUE) to filter XSS on the input level:

$this->db->where('username',$this->input->post('user_name', TRUE);
$dbResult = $this->db->get('users');

libraries/input.html

BeKa
  • 1
  • 2
  • Oh, lord, CI reinvented "magic quotes"? You can only escape something *for a particular context*, so a global "XSS filter" makes no sense. However, your post also includes an active record-style dynamic `where()`, which probably *will* do the right kind of escaping. – IMSoP Mar 04 '14 at 17:58
  • Maybe a little bit more than ol' "magic quotes", details in the class 'CI_Security', method 'xss_clean', cf. [github](https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Security.php) – BeKa Mar 04 '14 at 18:23
  • Yes, I was being a bit sarcastic, I admit. But like magic quotes, it gives false confidence to have something that claims to "make input safe", as demonstrated by you even mentioning it in a post about SQL injection, which it will do absolutely nothing about (but magic quotes attempted to, badly). – IMSoP Mar 04 '14 at 18:50
  • I agree! Just added this INPUT-thing but without connection to the main question "sanitize queries". – BeKa Mar 06 '14 at 16:40
0

The docs for (at least) 2.2 state, in a big red box:

Although Active Record will try its best to properly quote any field and table names that you feed it, note that it is NOT designed to work with arbitrary user input. DO NOT feed it with unsanitized user data.

Which to this programmer means "do not rely on Active Record to quote anything".

Madbreaks
  • 19,094
  • 7
  • 58
  • 72
0

Using escape function to injection of CI

<?php $username = $this->input->post('username');
$query = 'SELECT * FROM subscribers_tbl WHERE user_name = '.$this->db->escape($email);
$this->db->query($query);?>
help-info.de
  • 6,695
  • 16
  • 39
  • 41
Akbor
  • 1,280
  • 1
  • 9
  • 11
0

It may be a pain but you should convert your queries to active record.

I'm copying from the CodeIgniter manual: "Beyond simplicity, a major benefit to using the Active Record features is that it allows you to create database independent applications, since the query syntax is generated by each database adapter. It also allows for safer queries, since the values are escaped automatically by the system."

And like some people already said, yes this code is susceptible to SQL injection

VangelisB
  • 438
  • 5
  • 10