phalcon / incubator

Incubator adapters/functionality for the Phalcon PHP Framework
Other
730 stars 339 forks source link

(Secruity|DataHandling) MongoCollection toArray contains DI and more #922

Closed cottton closed 1 year ago

cottton commented 5 years ago

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

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 commented 5 years ago

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 ...

Jeckerson commented 1 year ago

Try to use phalcon/incubator-mongodb@v2.0.0