Saving Model Data and Security

In google groups there are quite a few discussions every month about security against (primary key) injection, xss or other things.
And yes, the default templates will not protect you from any of this. They are meant to produce quick "standard" output. But they are not prepared for the real world, at all.
It is actually quite irresponsible not to use custom templates or manually adjust the forms/views.
UPDATE: I could finally convince them to use h() in their default templates! A big WIN for security!

A while ago i wrote about how to create custom templates. See this article for details.
You will also notice, that the controller actions and views are protected against basic – and only basic – injections.
That is mainly xss-attacks. By using h() in the views we can make sure that dangerous strings get destroyed. The custom templates also ensure, that the record we edit/delete exists.

What it still doesn’t do is protecting the model data.

Note: some of the following could be achieved by adding the security component. But sometimes, it is not possible or not desired.

Protection against too many fields

Another tricky one. Again, Firebug can be used to violate your forms. Lets say, a user wants to have more rights – and he happens to know that User.role_id must be 2 instead of 1 in order to be "admin".
In the User edit form (besides "email", "birthday" and stuff) he could then inject a form input called "role_id" which usually is not there.
With a normal $this->save() operation this would actually work! After the next login his role would be 2 (= admin).
What can we do about something like that?

Cake has a "fieldList" as third parameter of the save() function build in:

if ($this->User->save($this->data, true, array('id', 'gender', 'birthday'))) { # note the id (needed here)

Id, gender and birthday are passed, any other field in $this->data will be omitted (especially our role_id^^).
I use this in all sensitive forms (user and role related).

Careful: If you don’t pass the id, it will be omitted and the model cannot set its primary key (and might perform "add" instead of "edit"!

Primary key injection on edit actions

This is the not quite the same as the above topics. All edit actions have the primary key in the forms. I usually remove them, but sometimes I am too lazy to do that (because you need to alter the form url then as well).
So the id actually exists – but won’t be checked prior to the save.
Well, here are some custom solutions – there are more general ones, of course. But I just want to point out the problem and a way to fix it. These could be added to your bake templates, for instance.

Lets take our custom template from above (see link) and modify it:

public function edit($id = null) {
    if (empty($id) || !($data = $this->User->find('first', array('conditions' => array('User.id' => $id)))) {
        //error flash message
        $this->redirect(array('action' => 'index'));
    }

    if (!empty($this->data)) {
        # solution 1: just override with the "correct" one:
        $this->data['User']['id'] = $id;

        # solution 2: check against "correct" one
        if ($this->data['User']['id'] != $id) {
            //BLACKHOLE
        }

        if ($this->User->save($this->data)) {
            //OK
        } else {
            //ERROR
        }
    }
}

Solution 1 is self-explanatory and cannot go wrong.
Solution 2:
After we made sure the record exists, we check the passed id against the record id.
If they don’t match someone tries to trick us. Otherwise the record can be saved.
Solution 3:
Remove the id from the passed arguments (third parameter of save() – but makes it more complicated)
Solution 4:
Put the check in beforeValidate() of your app_model.php. Usually – with the above template you have just "read" the record, so the model has the id in $this->id. It could then return false if the passed id and $this->id do not match. Thats actually my favorite. But I am not sure if it works in all cases. You may only check if you are actually saving a record – meaning: both ids have to be not empty. Only then it makes sense to apply this specific piece of validation.

Protection against missing fields

This is a tricky one. Most beginners (I didn’t either) dont realize that a form can be modified with tools like and Firebug (Firefox Addon) and that Cake would not bother.
Lets say we have a registration form and your gender has to be provided. But you don’t want to. So you simply remove this input from the form and submit it. Violà – it worked!

Why does it work? Cake automatically applies the validation "needed" for this form. All fields that are passed (it doesn’t matter if they are empty or not – they just have to exist on submit) will be checked. Validation rules for fields that did not get passed will be omitted. And thats our problem. Right here.

OK, we need to make sure that the form fields are actually part of the model data before the validation kicks in.
I wrote a app model extension which can be used – you could also write a model function which one by one adds the required fields if the field does not exist.

Put this in your app model:

/**
 * set + guaranteeFields!
 * extends the core set function (only using data!!!)
 * 2010-03-11 ms
 */
public function set($data, $data2 = null, $requiredFields = array()) {
    if (!empty($requiredFields)) {
        $data = $this->guaranteeFields($requiredFields, $data);
    }
    return parent::set($data, $data2);
}

/**
 * make sure required fields exists - in order to properly validate them
 * @param array: field1, field2 - or field1, Model2.field1 etc
 * @param array: data (optional, otherwise the array with the required fields will be returned)
 * @return array
 * 2010-03-11 ms
 */
public function guaranteeFields($requiredFields, $data = null) {
    $guaranteedFields = array();
    foreach ($requiredFields as $column) {
        if (strpos($column, '.') !== false) {
            list($model, $column) = explode('.', $column, 2);
        } else {
            $model = $this->alias;
        }
        $guaranteedFields[$model][$column] = ''; # now field exists in any case!
    }
    if ($data === null) {
        return $guaranteedFields;
    }
    if (!empty($guaranteedFields)) {
        $data = Set::merge($guaranteedFields, $data);
    }
    return $data;
}

We can now make sure removing any form field will have no effect 🙂 I call it "enforced whitelisting".

Case 1: Using $this->Model->set($this->data) and $this->Model->save() – Important: note the NULL in save(), we may not pass the data a second time!

$this->User->set($this->data, null, array('gender', 'birthday', ...));
$this->User->save();

Case 2: Using directly $this->Model->guaranteeFields($fields, $this->data) and $this->Model->save($this->data)

$data = $this->User->guaranteeFields(array('gender', 'birthday', ...), $this->data);
$this->User->save($data);

Lets combine them

$this->data['User']['id'] = $id; // on edit
$this->User->set($this->data, null, array('gender', 'birthday', ...));
if ($this->User->save(null, true, array('id', 'gender', 'birthday', ...))) { # note the id (needed here)
    //OK
} else {
    //ERROR
}

Now thats protection 🙂
Only the fields that you want to are passed and all the fields that should be there are there. And the primary key is the one it is supposed to be. If your validation rules are correct, you got the perfect form handling, I guess.

Blacklisting

// put this in your app_model.php
public function blacklist($blackList = array()) {
    return array_diff(array_keys($this->schema()), $blackList);
}

Although not recommended for general usage, this might be useful for some rare occasions.
It returns all fields of the current database table except the ones you blacklisted.

Usage:

$this->User->save(null, true, $this->blacklist(array('birthday'));

If you want to pass non-existing fields to the model for validation you need to array_merge() them!

$this->User->save(null, true, array_merge(array('virtual_field'), $this->blacklist(array('birthday')));

Server-side and Client-side Validation

First one is what we just discussed. Client-side usually is JS or browser-specific validation. You should NEVER rely on client-side validation – it can easily be tricked. It is nice to have it – "additionally" in order to prevent unnecessary posts, but the real deal should be what your model validation returns.

SQL Injection

I always smile about other webprojects that are vulnerable to this attack. Its just hilarious. Thats why you use a framework. Because it takes care of the basic problems. And this is one of it.
There are only very few things to be aware of:
Always manually escape manual SQL queries!
CakePHP only takes care of the queries run by $this->save(), $this->saveAll() etc. So if you ever have to write your own SQL script, you will need to do that yourself.

The myth about "required" and "allowEmpty"

They seem to be the same, don’t they? But there is a huge difference between them (the cookbook does mention it here).
"required" is like my "Protection against missing fields", only hard-coded. These fields will validate false, if this field is not present on validation. It does not matter if the field is empty or not. The word "required" only applies to the database field, not its content.
"allowEmpty" is equal to "minLength 1" and ONLY validates this field if the field has been passed to the model. If not, it will not return false. You cannot make sure that a specific field is always filled out with this parameter alone.

If you want to make sure, that the field cannot be avoided AND has to be filled out, you need to combine both rules:

public $validate = array(
    'status' => array(
        'maxLength' => array( # this can be any label you want
            'required' => true, # careful!
            'allowEmpty' => false,
            'rule' => array('maxLength', 5),
            'message' => array('valErrMaxCharacters %s', 5), # a modification of mine to allow better i18n support
            'last' => true # important for more than one rule per field!!!
        ),
        ... # other rules
    ),
    ... # other fields
);

The "careful" stands for: this can get ugly really fast. I never use "required" hard-coded. As soon as you use ajax toggling of some fields or forms which only change a small part of the record, you will get validation errors because the required fields are not present (although you don’t even want to change them). You would need to unset() those rules manually. See the above "Protection against missing fields" for a cleaner approach.

Anyway, i hope this clears up things a little bit.

If you really feel that you need to use "required" you should only use it combined with ‘on’ => ‘create’, as well. In 99% of all cases you end up with never validating "edit" validations otherwise. That’t because you often save only a part of the data where you normally wouldn’t provide those required fields.
So as a guideline:

  • Either don’t use required attribute at all and use my whitelisting approach from above
  • Use required with "on create" IF (!) the field always has to be present for all create actions. For other actions use whitelisting and guaranteeFields

A few words to validation rules in general

You can add rules for "imaginary fields", as well. The fields don’t have to exist in the database model. This way you can easily use inputs which are validated separately and combined later on (eg: pwd and pwd_repeat resulting in the final sha1-hashed "password").
Always use "last"=>true for your rules. Unfortunately, the default value is still false here. "true" makes sure that after the first rule already returned false, the following ones are not checked as well (overhead and totally useless).
Also sort your rule array properly. The rules are checked from top to bottom, so it would make sense to put all complicated stuff at the bottom and first check on simple "notEmpty" or "email" rules. The most likely ones to fail at the beginning. Why should you check "isUnique" (database query!) before "notEmpty"? Why should you check an email address exists in the database if it is invalid in the first place?

Last words

Why didn’t I use Security Component for some of the above tasks? Well, it was a bitch to use in the first months. All the time some kind of black holes (white screen of death) without any indication what went wrong. Login/Logout, some other forms as well. With the first ajax forms it got even more messy and i deactivated it at last. Works quite well without it, if you protect the forms manually. With lots of ajax stuff going on you have to do that yourself, anyway…
And against certain attacks the Security Component won’t help anyway (PrimaryKey injection in edit forms etc). So at some point you will have to deal with it yourself. You have to decide to what point. But better sooner than later. If you adjust your bake templates before you start a new project it will save you lots and lots of time while having secure forms and procedures.

Note – 2012-06-02

For Cake2.x please use $this->request->data instead of $this->data in the controller scope.

5.00 avg. rating (95% score) - 2 votes

14 Comments

  1. I’ve found that the security component is worth getting used to. You’d get free CSRF protection with it too :).

    Custom bake templates are good – saves the time adding back in security after the fact. Maybe you could submit some secured ones as a patch?

  2. How is the overhead with forms that contain some ajax fields? do you manually check on fields in this case?

  3. brilliant stuff. I wish there was more of this kind of introspection combined with well-written reasoning and instruction that anyone can grasp out there. I’m glad I found your blog.

    thank you!

  4. Hey Mark

    Nice article. I came up with a smaller solution for the "missing fields" problem:

      function beforeValidate() {
        if($this->isNewRecord()) {
          $this->__guaranteeFields();
        }
        return parent::beforeValidate();
      }
      
      function isNewRecord() {
        return empty($this->id);
      }
      
      function __guaranteeFields() {
        $allFields = array();
        foreach($this->_schema as $key => $value) {
          $allFields[$key] = null;
        }
        unset($allFields['id']);
        $this->data = $allFields;
      }

    What do you think about it?

    By the way, it would be nice to get a notice when you reply to one of the comments here in the blog…

  5. Hey Euromark,

    i’ve got a question. How do you secure following:
    As example you have a select list renderered from

    $this->Model->find('list');

    as a result you get a select list like this:

    Test 1
    Test 2
    Test 3
    Test 4

    in Firebug (as you mentioned before) it’s very easy to manipulate the select box and add some new values to the select box. Cake would save this new value.

    My approach was to secure this trough the validate function in model with this rule:

    var $validate = array(
          'function' => array(
          	'allowedChoice' => array(
          		'rule' => array('inList', array('Foo', 'Bar')),
          		'message' => 'Enter either Foo or Bar.'
          	)

    but it seems not to be the best idea. Because i didn't can dynamically add some new values.

    Maybe you have an easy hint to me.

    Greetz Patrick

      )
    );
    
  6. @josh
    you always need to secure the form for both add and edit.
    you also might override any set variable.
    so yours won’t work either way i guess

    @patrick
    simply create a custom validation rule with uses find(‘first’).
    If it returns something, the value is valid, otherwise not.

  7. Hey Mark, thanks for that.

    My solution now includes a custom validation with find(‘count’) in
    app_model.php.

    Works fine.

  8. I would disagree with your opinion to the "last" parameter. It’s quite useful that this parameter isn’t "true" on default.

    If a user submit a form, only half filled, i would present him all fields with the useful error messages. If I would have set "last" to "true", he would get only one error – corrects it, submit the form again and get another error.

  9. you are talking about cake1.3 aren’t you?
    well, than you should know that the form will always be validated completely. regardless of true/false of last (and only 1 message can be displayed at once per field).
    the last parameter is handling the validation rules PER field. for you I will try to clarify:
    without it it will validate ALL rules for a field. with last=>true it will stop to validate this field as soon as an error is returned.
    this is not only logical but also less time consuming and less confusing (without last=>true you would end up with the error message of the last rule, which in most cases does not make sense if even the first one already failed).

    in cake1.3 there is always only 1 error message that can be returned. so your statement doesn’t make sense. there is no way it could display all error messages for a single field at once.

    in cake2.0 there is the option to return an array and therefore last => false does make sense again in some rare cases – for the first time!
    but in 99% of all cases this is overhead. why making a DEEP email check with dns lookup for an empty string if the first rule NotEmpty is already throwing an error?

  10. Hi Mark!

    I’m trying to implement protection against Primary key injection attack but I probably did something wrong.

    I have an edit form:

    public function edit($id = null) {
    $this->Restaurant->id = $id;
    if (!$this->Restaurant->exists()) {
       throw new NotFoundException(__('Invalid restaurant'));
    }
    if ($this->request->is('post') || $this->request->is('put')) {
    if ($this->request->data['Restaurant']['id'] != $id) {
    throw new NotFoundException(__('Invalid restaurant'));
    }
       if ($this->Restaurant->save($this->request->data)) {
       	$this->Session->setFlash(__('ok!'));
       	$this->redirect(array('action' => 'edit', $id));
       } else {
       	$this->Session->setFlash(__('Error'));
       }
    } else {
    // find stuff to populate the form
    }

    As you can see I’m trying to use your method #2 but I’m getting exception on every request (even with the correct ID) and also I’m getting notice "Undefined index: id ".

    Btw, you wrote that you usually remove primary key in the forms – how do you do that?
    Thanks in advance!

  11. This usually happens if you touch the create() method.
    Dont. If you only use $this->Form->create('User') it will automatically post to itself (hence not lose the id in the form.
    In your case it probably posts to /edit and therefore loses the id.
    also make sure that you have your hidden field for the id in your form. that could also trigger what you described.

    using debug() you can easily find out what your variables contain.

    since cake2 there is no need to remove them anymore – if you use the security component all hidden fields and its contents cannot be tempered with anymore.

  12. Hi Mark

    Your article is very interesting.

    Usually I have a different approach: Instead of struggling around with whitelisting (and blacklisting) I never save the request data directly.

    I copy the request data to a new array which would be used for saving. By doing this I could specify exactly which data I would like to save. Additional I could make some restrictions to the data like filtering the role_id to the allowed ones in one step.

    What do you think about this approach? Does it have some pitfalls? Why make it more complicated as needed?

    Thanks,
    Frank

  13. I don’t like it for the code overhead it produces. Even though you are more verbose, this verbosity can also get quite out of hand soon.

    Better have a simple whitelisting array and be done with it 🙂

Leave a Reply

Your email address will not be published. Required fields are marked *

This site uses Akismet to reduce spam. Learn how your comment data is processed.