incubator icon indicating copy to clipboard operation
incubator copied to clipboard

(Secruity|DataHandling) MongoCollection toArray contains DI and more

Open cottton opened this issue 6 years ago • 1 comments

Related: https://github.com/phalcon/incubator/issues/908

  • Phalcon Framework version: 3.1.2
  • Phalcon Incubator version: 3.0.x
  • PHP Version: 7.1.30

Test script:

class User extends Phalcon\Mvc\MongoCollection
{
    public function getSource()
    {
        return 'user';
    }
}

$model = new User();
$model->foo = 1;
$model->create();

/** @var User $model */
$model = User::findFirst();
echo var_export($model->toArray(), true) . PHP_EOL;
// array(
//     '_id'                 => MongoDB\BSON\ObjectId::__set_state(array(
//         'oid' => '5d31e528f328bb00bc38a402',
//     )),
//     '_dependencyInjector' => array(),        <---------- oO?
//     '_modelsManager'      => array(),        <---------- oO?
//     '_source'             => null,
//     '_operationMade'      => 1,
//     '_dirtyState'         => 1,
//     '_connection'         => array(),
//     '_errorMessages'      => array(),
//     '_skipped'            => false,
//     'foo'                 => 1,
// )

This is a HUGE problem if you use toArray f.e. in a log or so. The DI contains everything. It may contains sensible data. So IMO this is a security leak.

cphalcon filters reserved keys on toArray: https://github.com/phalcon/cphalcon/blob/master/phalcon/Mvc/Collection.zep#L1107

incubator should do that too!

Example what could go wrong: user wants to log something - something went wrong with a model ...

function logSome($msg, $data)
{
    $content = $msg;
    $content .= "... bla ... . Data: " . var_export($data, true);
    // ... here the message gets send into "public" (email, public log, w/e)

    // BAM - DI inside including everything
    // 'Problem bla something wrong.... bla ... . Data: array (
    //   \'_id\' =>
    //   MongoDB\\BSON\\ObjectId::__set_state(array(
    //      \'oid\' => \'5d6bf549f328bb00e90717a2\',
    //   )),
    //   \'_dependencyInjector\' =>
    //   array (
    //   ),
    //   \'_modelsManager\' =>
    //   array (
    //   ),
    //   \'_source\' => NULL,
    //   \'_operationMade\' => 1,
    //   \'_dirtyState\' => 1,
    //   \'_connection\' =>
    //   array (
    //   ),
    //   \'_errorMessages\' =>
    //   array (
    //   ),
    //   \'_skipped\' => false,
    //   \'foo\' => 1,
    // )'
}

logSome('Problem bla something wrong.', $model->toArray());

In the example the DI is array, which is posted elsewhere as bug. If you set the DI on the model then var_export would try to break it down, PHP brings warning var_export does not handle circular references but (depends on errorhandler) it may not stop the execution.

Of curse a user should choose the data to use in public. But ... well, we should not get anything else than data on toArray.

cottton avatar Sep 01 '19 17:09 cottton

Note: for w/e reason cphalcon does return NULL on ::getReservedAttributes. It looks like all attributes WOULD be filtered in there. No idea atm why this return NULL ...

cottton avatar Sep 01 '19 19:09 cottton

Try to use phalcon/[email protected]

Jeckerson avatar Jul 09 '23 21:07 Jeckerson