Cphalcon: serious security problem with whitelist

[yasharrashedi]

if form->isValid() is executed before $model->save($data,$whiteList); whitelist wont work and causes serious security problem
following is a simple example:
````php

class TestController extends Controller
{

public function testAction()
{
    $test = Test::findFirst(1);


    $form = new Form($test);
    $form->add(new Text('name'));
    $form->add(new Text('name2'));

    $whiteList = ['name'];

    $data = ['name' => 'this is allowed, no problem', 'name2' => 'changed by hacker'];

    $form->isValid($data);

    $test->save($data, $whiteList);

    $test = Test::findFirst(1);

    print_r($test->toArray());

    $this->view->disable();
}

}
````
outputs

````
Array
(
[id] => 1
[name] => this is allowed, no problem
[name2] => changed by hacker
)

````

Phalcon version 3.0.1
OS: Windows
PHP 5.5

[Jurigag]

isValid in form is binding data to entity.

[yasharrashedi]

ok what do you say about:
php $form->bind($data,$test,$whiteList);
same results!

[yasharrashedi]

````php
class TestController extends Controller
{

public function testAction()
{
    $test = Test::findFirst(1);


    $form = new Form($test);
    $form->add(new Text('name'));
    $form->add(new Text('name2'));

    $whiteList = ['name'];

    $data = ['name' => 'this is allowed, no problem', 'name2' => 'changed by hacker3'];

    $form->bind($data,$test,$whiteList);
    if($form->isValid()){
         $test->save($data, $whiteList);
    }
    $test = Test::findFirst(1);

    print_r($test->toArray());

    $this->view->disable();
}

}
outputs
Array
(
[id] => 1
[name] => this is allowed, no problem
[name2] => changed by hacker3
)
````

[Jurigag]

I told you, isValid just bind data anyway, remove isValid.

https://github.com/phalcon/cphalcon/blob/master/phalcon/forms/form.zep#L270

I guess whitelist probably should be an property of form and there should be added whitelist parameter to isValid.

[yasharrashedi]

@Jurigag I can't remove it because I'm using it, isValid has no whitelist parameter, this code is a simple example of problem not the main code I'm using

[Jurigag]

Yes i know it doesn't have it. It should be added

[yasharrashedi]

I'm using form for edit action, so I need entity data passed to form. problem is not seen when not setting entity to form, but I also tested following code and got the same problem

````php
class TestController extends Controller
{

public function testAction()
{
    $test = Test::findFirst(1);


    $form = new Form();
    $form->add(new Text('name'));
    $form->add(new Text('name2'));
    $form->setEntity($test);

    $whiteList = ['name'];

    $data = ['name' => 'this is allowed, no problem', 'name2' => 'changed by hacker 5'];
    $form->bind($data,$test,$whiteList);


    if($form->isValid()){
        $test->save();
    }


    $test = Test::findFirst(1);

    print_r($test->toArray());

    $this->view->disable();
}

}
````
outputs

````
Array
(
[id] => 1
[name] => this is allowed, no problem
[name2] => changed by hacker 5
)

````

[mbrostami]

setEntity is different than binding entity with bind method (I don't know why!!).
Calling bind method doesn't set entity to the form!

Look at this code:

$form = new Form();
$form->bind($data, $test, $whiteList);
var_dump('1', $form->getEntity());
$form->setEntity($test);
var_dump('2', $form->getEntity());

Outputs:

null

2
Test Object()

Any way...
When you are using setEntity or by constructor parameter you are actually giving an object by reference to the form. In isValid method phalcon calls bind method again without whitelist, so all data will be appended to the form entity.
https://github.com/phalcon/cphalcon/blob/master/phalcon/forms/form.zep#L281
As you know entity is by reference. Now if you want to save your model, consider that fields value already are changed, so even whitelist in save() method doesn't take effect because model is changed in IsValid().

In your last example, you are binding $test model to the form with bind method. If you print out $test model before isValid you will see that all goes well and whitelist is considered. But after isValid you will see that $test model is changed without considering your whitelist.

But you can try this code:

$form = new Form();
$form->add(new Text('name'));
$form->add(new Text('name2'));
// $form->setEntity($test); // Do not setEntity since in isValid method $test will be changed without whitelist!
$whiteList = ['name'];
$data = ['name' => 'this is allowed, no problem', 'name2' => 'changed by hacker 5'];
$form->bind($data, $test, $whiteList);
if ($form->isValid($data)) {
     $test->save(); // $test object is changed (with whitelist) in bind() method. We can save that. 
}

Or

$form = new Form();
$form->add(new Text('name'));
$form->add(new Text('name2'));
$whiteList = ['name'];
$data = ['name' => 'this is allowed, no problem', 'name2' => 'changed by hacker 5'];
if ($form->isValid($data)) {
     $test->save($data, $whiteList);
}

I think this is a bug in Phalcon form.

[yasharrashedi]

I solved problem by extending Form class and overriding bind method as following:

````php

class Form extends PhalconForm {
public function bind(Array $data, $entity, $whiteList = null)
{
$allowedData = [];
if($whiteList && is_array($whiteList)){
foreach ($whiteList as $whiteItem){
if(array_key_exists($whiteItem,$data)){
$allowedData[$whiteItem] = $data[$whiteItem];
}
}

        $data = $allowedData;
    }
    parent::bind($data,$entity,$whiteList);
}

}

[yasharrashedi]

@mbrostami as I said before, I'm passing form data to view, this form is used in a edit page, so user must see previous data (current entity data) before edit.

[mbrostami]

@yasharrashedi You can also try pass entity after isValid method.

$form = new Form();
$form->add(new Text('name'));
$form->add(new Text('name2'));
$whiteList = ['name'];
$data = ['name' => 'this is allowed, no problem', 'name2' => 'changed by hacker 5'];
if ($form->isValid($data)) {
     $test->save($data, $whiteList);
}
$form->setEntity($test);

[Jurigag]

Well as i wrote above - $whiteList should be added to form as property imho, and it should be added to isValid method as argument as well, if we will either use isValid or bind method it should save this whitelist for form and use it everywhere in code.

[yasharrashedi]

I just did a trick and again changed my code to following

````php
public function testAction()
{
$test = Test::findFirst(1);

    $form = new Form(clone $test);
    $form->add(new Text('name'));
    $form->add(new Text('name2'));

    $whiteList = ['name'];

    $data = ['name' => 'this is allowed, no problem', 'name2' => 'changed by hacker 5'];

    if($form->isValid()){
        $test->save( $data, $whiteList);
    }


    $test = Test::findFirst(1);

    print_r($test->toArray());

    $this->view->disable();
}

````

this solved the problem, but bug is still remaining in Phalcon.

[stale[bot]]

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues