Achievo/ATK - Bugzilla – Bug 624
SQL injection vulnerability in class.employee.inc
Last modified: 2006-05-27 00:26:08
You need to log in before you can comment on or make changes to this bug.
When an employee is deactivated, the WHERE clause describing the employee ID is passed to the action via the URL, then in function 'action_deactivate' (line 152 of /achievo-1.1.0/modules/employee/class.employee.inc) that clause is passed directly to a SQL query without checking its validity or scope. The URL can be crafted to pass SQL statements directly to the db without checking, allowing an attacker to inject destructive SQL. As an exemple here is how to deactivate the logged in user (which is normaly not possible): In the Employee - Administration page, copy the URL from the 'Deactivate' link of any other employee but yourself. Paste the URL in your browser bar and change the id to your own. Launch the URL and you are now deactivated! /achievo-1.1.0/dispatch.php?atknodetype=employee.employee&atkaction=deactivate&atkselector=person.id%3D%27<<<CHANGE_THIS_TO_YOUR_ID>>>%27&atklevel=1&atkprevlevel=0&atkstackid=4429bc8d28436&&atkprevlevel=0&atkstackid=4429bc8d28436& Of course an attacker would change the whole &atkselector=person.id%3D%272%27 to something more destructive. Mitigating factor: The user has to be logged in and has to have access to this screen in order for this to work. I didn't check the rest of the code to see if a similar trick was used to a less protected area of the application. Possible solution: If the field name can be different than 'person', pass an identifier to represent the field name and the id of the employee then in the 'action_deactivate', use that identifier to select the right field name from a predetermined list and build the WHERE clause that way. If the field name is always 'person' then there is no reason to pass it via the URL. In addition verify that the ID is indeed a number only (since it will be used as part of the SQL query). I'd be happy to write a patch to fix this if you'd like.
Here is a patch to fix the injection. It's quick and dirty but I've tested it against my installation and it seams to do the trick: --- modules/employee/class.employee.inc.orig 2004-07-04 11:25:08.000000000 -0500 +++ modules/employee/class.employee.inc 2006-03-28 18:41:21.000000000 -0600 @@ -149,7 +150,16 @@ global $g_db; if ($this->m_postvars["atkselector"]!="") { - $g_db->query("UPDATE person SET status='nonactive' WHERE ".$this->m_postvars["atkselector"]); + $select = $this->m_postvars["atkselector"]; + $query = "UPDATE person SET status='nonactive' WHERE $select"; + + // Verify that the selector or id were not tampered with before passing on to the db + if(preg_match("/person\.id='\d+'/i", $select) === 1) { + $g_db->query($query); + } else { + // TODO: give the name of the module, the logged userid and the IP address of the offending user in the debug entry so it gets logged properly + atkdebug("SQL injection attack detected in 'action_deactivate', not executed. The selector was '$select' and the resulting query would have been '$query'", true); + } } $this->redirect(); }
Thanks, we will evaluate your patch. Usually, actions first select a record, then pass that to atkNode's allowed($action, $record) method, and this method determines whether the action is allowed. In this case, the deactivation is not implemented on the security level, only on the interface level. There may be more parts of the app that rely on where clauses like this, I think we should create a generic anti-url-tampering construct. Reassigning to sandy so he can decide to apply the workaround for now.
Fixed in CVS (achievo 1.2.1)