0

I was wondering if code I have written is open to attack.

    $.ajax({
        url: site_url+"/customer/update",
        type: 'POST',
        dataType: "json",
        async: true,
        data: {
            'id':$('#id').val(),
            'cuFirstname':$('#firstname').val(),
            'cuLastname':$('#lastname').val(),
            'cuPersonalnr':$('#personalnr').val(),
        },
    });

On the server it looks like this:

    $this->db->where('cuID = '.$customerid);
    $this->db->update('customers',$_POST);

So I'm thinking that maybe if someone could change the variables (cuFirstname, cuLastname, cuPersonalnr) in the data part of the ajax post, that they would be able to write sql-code there.

"update customers set cuFirstname = 'charlie', cuLastname = 'brown', cuPersonalnr = '7012230303' where cuID = 1000"

So if they changed cuLastname to something else it could look like this:

update customers set cuFirstname = 'charlie', [cuShouldnotbechanged] = 'brown', cuPersonalnr = '7012230303' where cuID = 1000

So my question is: Is it possible for an attacker to change those variable names, and if so, how?

Andreas Larsson
  • 555
  • 3
  • 5
  • 13
  • 2
    *Anyone can send any data.* It’s the server’s job to process them properly. – Gumbo Mar 26 '14 at 15:14
  • Yup. Anyone can can post any data to your server, no need to mess with your ajax call to do it. – superphonic Mar 26 '14 at 15:15
  • if you're asking if it is possible to change the variables: YES it is! anyone can act on javascript in order to change the data before the POST call. – ponciste Mar 26 '14 at 15:15
  • This seems to be vulnerable to [mass assignment (CWE-915)](https://cwe.mitre.org/data/definitions/915.html). – Gumbo Mar 26 '14 at 15:16
  • **[Duplicate of very recent very hot question](http://stackoverflow.com/questions/22534183/do-i-have-to-guard-against-sql-injection-if-i-used-a-dropdown)** – Your Common Sense Mar 26 '14 at 15:20
  • Actually, he's asking something slightly different - not really about SQL injection per se, but about whether a user could update arbitrary columns in the query. – Kryten Mar 26 '14 at 15:22
  • What database API do you use? – Gumbo Mar 26 '14 at 15:22
  • 1
    Injection is always an injection. Such trifle differencies in particular queries do not make them safe. – Your Common Sense Mar 26 '14 at 15:23
  • @YourCommonSense: They're caused (and attacked, and fixed) in entirely different ways. The obvious fixes for SQL injection simply won't work here, if you *have* to build a query based on what's in the POST data. – cHao Mar 26 '14 at 15:40
  • @cHao how come? My lib works perfectly and flawlessly for the case :) – Your Common Sense Mar 26 '14 at 15:42
  • @YourCommonSense: If you say beforehand which columns you care about, then OK. :) But if you just accept anything and build a query from it, you may well be vulnerable. Even if you use prepared statements. Even if everything is escaped perfectly. – cHao Mar 26 '14 at 15:46
  • 1
    @cHao if we're talking of just insert/update, it can be pretty safe against conventional injection. The only possible vulnerability is ability to write into columns a user disallowed to. For this case whitelisting is required. But if it's not the case, formatting alone is all right. If query is just arbitrary, you're right of course – Your Common Sense Mar 26 '14 at 16:29

2 Answers2

1

The client can change any aspect of the AJAX call, simply by making their own HTTP request to your URL with their own parameters. So, yes, they could conceivably change any part of the request.

In your code, the question really boils down to "how does my database library handle the update?". You're doing the following:

$this->db->where('cuID = '.$customerid);
$this->db->update('customers',$_POST);

which is, presumably, building a query like:

UPDATE customers SET column1='some value', column2='some other value', ... WHERE cuID='whatever';

based on the keys and values of the $_POST array. To address your specific question about what happens if a client changes the keys n the $_POST array, it seems to me there are two possibilities:

  1. if they enter a column name that does not exist, the database library is either going to ignore it (and update the stuff it is able to) or throw an error (because an UPDATE statement with a non-existent column name is an SQL error).

  2. if they enter a column name that exists but that you did not intend to update, then that new column name will probably be used and updated (unless your database library has protection in place for that - some require you to explicitly state which columns can be updated in this way).

Kryten
  • 15,230
  • 6
  • 45
  • 68
0

Can a user write SQL code into those variabiles? The answer is yes.

Is it open to attack? That entirely depends on your method of sanitization/SQL input.

You can use prepared statements such as PDO (properly) to prevent the possibility.

Otherwise sanitize/check the sent data: It looks as the cuPersonalnr, should be numeric? check to make sure:

if (!is_numeric ($_POST['cuPersonalnr'])) 
    exit();    //script stops, not a number

first name and last name, im assuming need to be alphanumeric only? well create a check, or sanitize any other values that are not alphanumeric:

if(!ctype_alnum($_POST['cuFirstname'])) {
    exit();    //script stops, contains unsafe characters
}

instead of exit() you can create an error variable, and return the error.

Josh Crozier
  • 233,099
  • 56
  • 391
  • 304
JimmyBanks
  • 4,178
  • 8
  • 45
  • 72
  • Is the SQL code interpreted as SQL? The answer is: probably not. It would be dump to provide an `update` method that can’t format the data properly. – Gumbo Mar 26 '14 at 15:20
  • ^"probably not" is not good enough for me in regards to security. – JimmyBanks Mar 26 '14 at 15:22
  • BTW, `exit()` is fscking *evil*. It makes testing a royal pain. Die with an error message at the very least. But better would be to throw an exception. (PHPUnit, as an example, can catch an exception, and continue on to the next test. But `exit()` stops the process dead in its tracks, without you even getting a hint as to why.) – cHao Mar 26 '14 at 15:56