CODEWITHSUNDEEP
securitycakephp
Michaelkay
Michaelkay

Reputation: 258

cakePHP and authorization for CRUD operations

I have a cakephp 1.3 application and I have run into a 'data leak' security hole. I am looking for the best solution using cake and not just something that will work. The application is a grade tracking system that lets teachers enter grades and students can retrieve their grades. Everything is working as expected but when I started to audit security I found that the basic CRUD operations have leaks. Meaning that student X can see student Y's grades. Students should only see their own grades. I will limit this questions to the read operation.

Using cake, I have a grade_controller.php file with this view function:

function view($id = null) {
   // Extra, not related code removed
   $this->set('grade', $this->grade->read(null, $id));
}

And 

http://localhost/grade/view/5

Shows the grade for student $id=5. That's great. But if student #5 manipulates the URL and changes it to a 6, person #6's grades are shown. The classic data leak security hole.

I had two thoughts on the best way to resolve this. 1) I can add checks to every CRUD operations called in the controller. Or 2) add code to the model (for example using beforeFind()) to check if person X has access to that data element.

Option #1 seems like it is time consuming and error prone. Option #2 seem like the best way to go. But, it required calling find() before some operations. The read() example above never executes beforeFind() and there is no beforeRead() callback.

Suggestions?

Upvotes: 1

Views: 733

Answers (2)

Ajay K
Ajay K

Reputation: 354

If CakePHP supports isAuthorized, here's something you could do:

Create a column, that has the types of users (eg. 'student', 'teacher', ...)

Now, it the type of User is 'student', you can limit their access, to view only their data. An example of isAuthorized is as follows. I am allowing the student to edit only their profile information. You can extend the concept.

if ((($role['User']['role'] & $this->user_type['student']) == $this->user_type['student']) {
      if (in_array($this->action, array('view')) == true) {
                $id = $this->params->pass[0];
                if ($id == $user_id) {
                    return (true);
                }
            } 
        }
}

Upvotes: 0

Dave
Dave

Reputation: 29121

Instead of having a generic read() in your controller, you should move ALL finds, queries..etc into the respective model.

Then, go through each model and add any type of security checks you need on any finds that need to be restricted. 1) it will be much more DRY coding, and 2) you'll better be able to manage security risks like this since you know where all your queries are held.

For your example, I would create a getGrade($id) method in my Grade model and check the student_id field (or whatever) against your Auth user id CakeSession::read("Auth.User.id");

You could also build some generic method(s) similar to is_owner() to re-use the same logic throughout multiple methods.

Upvotes: 2

Related Questions