sapplica / sentrifugo

Sentrifugo is a FREE and powerful Human Resource Management System (HRMS) that can be easily configured to meet your organizational needs.
http://www.sentrifugo.com/
GNU General Public License v3.0
486 stars 426 forks source link

Security issues - SQL injection #41

Open f806674 opened 8 years ago

f806674 commented 8 years ago

It looks as though the application doesn't enforce security and SQL injection is possible, amongst other vulnerabilities.

E.g. look at https://github.com/sapplica/sentrifugo/blob/4c5f09eb34e7df835d1eaa098b701b8d83adb6ae/application/modules/default/models/Users.php#L95:

            $userData = $db->select()
            ->from(array('a' => 'main_users'),array('aid' => 'a.id'))
            ->joinInner(array('r'=>'main_roles'), 'r.id=a.emprole',array("def_status" => "if(r.group_id in (1,5) and a.userstatus = 'new','old',a.userstatus)"))
            ->where("a.isactive = 1 AND r.isactive = 1 AND a.emptemplock = 0 AND (a.employeeId = '".$corpEmail."' OR a.emailaddress = '".$corpEmail."')");
            $new_userdata = $db->select()
            ->from(array('ac'=>$userData),array('count'=>'count(*)'))
            ->where("ac.def_status = 'old'");

Or here - https://github.com/sapplica/sentrifugo/blob/4c5f09eb34e7df835d1eaa098b701b8d83adb6ae/application/modules/default/models/States.php#L28:


        $statesData = $this->select()
                           ->setIntegrityCheck(false)   
                           ->from(array('s'=>'main_states'),array('s.*'))
                           ->joinLeft(array('c'=>'tbl_countries'), 's.countryid=c.id',array('country_name'=>'c.country_name'))                         
                           ->where($where)
                           ->order("$by $sort") 
                           ->limitPage($pageNo, $perPage);

By inputting malicious parameters values, the application can be forced to perform arbitrary SQL queries which can compromise the entire HRM.

Am I missing something here? is there some global input validation or security module used to protect against this that I didn't notice?

SiNONiMiTY commented 8 years ago

Hi,

I think there is a level of protection as the developer used the Zend_Db_Select object to manage the select queries performed by the application.

Zend_Db_Select

The Zend_Db_Select object represents a SQL SELECT query statement. The class has methods for adding individual parts to the query. You can specify some parts of the query using PHP methods and data structures, and the class forms the correct SQL syntax for you. After you build a query, you can execute the query as if you had written it as a string.

The value offered by Zend_Db_Select includes

  • Object-oriented methods for specifying SQL queries in a piece-by-piece manner;
  • Database-independent abstraction of some parts of the SQL query;
  • Automatic quoting of metadata identifiers in most cases, to support identifiers containing SQL reserved words and special characters;
  • Quoting identifiers and values, to help reduce risk of SQL injection attacks.

You can see the that this class is instantiated before the actual query building.

Other queries are also passed to their respective methods.

Anyways, this was a nice find. Someone actually taking the time in scrutinizing the code... :)

f806674 commented 8 years ago

According to a developer of Zend, in an answer posted on StackOverflow - it's not sufficient to simply rely on Zend_Db_Select, but you need to sanitize input yourself - http://stackoverflow.com/a/985316

Also, Zend has been known to have SQL injection vulnerabilities because it's possible to bypass the quoting it does - https://framework.zend.com/security/advisory/ZF2016-02

In short, relying on Zend is not good enough and leaves the application open for attacks.

Given the sensitivity of information stored in the HRM - personal information, salaries, etc. - it's not safe enough to use unless proper safeguards against attacks are in place.

Can one of the Sapplica developers of Sentrifugo comment on this?

SiNONiMiTY commented 8 years ago

Correct. As I have mentioned above that there is a level of protection, you still have to add another layer of protection. The first one that comes to mind is to provide a database account for the application with limited set of privileges (e.g. SELECT, INSERT, UPDATE, DELETE)

Also, if you are not confident enough with the level of protection that the application can provide, you can always try to refactor the code base with prepared statements until the developers are able to address the issue. If you are hosting the project yourself, you could also set-up a Web Application Firewall to mitigate these types of attacks to your application.

Do keep in mind that the application was built on ZF1 (which is reaching the stage of obsolescence), particularly version 1.11.11, the patch for the said vulnerability in WHERE and ORDER clauses is available at 1.12.19. With the upcoming End Of Life of ZF1, the best course of action is to migrate to ZF3, which is not an easy task.

sapplica commented 8 years ago

Thank you for bringing this security issue to our attention! We want our users to have the best security possible and hence have initiated working on the fix. It will be included in our upcoming release Sentrifugo 3.0.

theonewolf commented 7 years ago

Has this been fixed? Sentrifugo 3.1 is out? @sapplica any comment?