Hi Yasuo
These sound good to me!
There’s still a smaller vulnerability of defining variables beforehand:
```
$data = ['hasAccess' => true];
export($data);
if ($user->isAdmin()) {
$hasAccess = true;
}
if (isset($hasAccess) && $hasAccess === true) {
print 'Bingo';
}
```
but code like this should be rather rare.
Regards
On 15 Sep 2017, 23:17 +0200, Yasuo Ohgaki <yohg...@ohgaki.net>, wrote:
> Hi all,
>
> On Sat, Sep 16, 2017 at 2:50 AM, Sara Golemon <poll...@php.net> wrote:
>
> > On Fri, Sep 15, 2017 at 1:35 PM, <ilija.tov...@me.com> wrote:
> > > The `extract` function takes an associative array and
> > > puts it into the local symbol table.
> > > http://php.net/manual/en/function.extract.php
> > >
> > > I seriously doubt the usefulness of this function,
> > > especially looking at the potential risks. The fact
> > > that overwriting the local variables is the default
> > > behaviour doesn’t make it any better. I suggest
> > > deprecating it in PHP 7.3 and removing it in 8.
> > >
> > Preface: I despise extract() as well. It's breaks assumptions for
> > both the developer and the runtime. I save some of my frowniest of
> > faces for extract().
> >
> > That said...
> >
> > > I can see it’s usefulness in this case.
> > > But wouldn’t it be better to implement this by hand
> > > in these rare cases (it’s 3 lines of code) instead of
> > > encouraging the pollution of the symbol table by
> > > unknown input? It’s also clearer since people who
> > > don’t know the `extract` function probably don’t
> > > expect it to mutate the local symbol table.
> > >
> > Let's be clear on what that looks like: foreach ($data as $key =
> > $value) { $$key = $value; }
> >
> > This is SO MUCH WORSE for several reasons, no least of all what
> > happens when $data contains keys named 'data', 'key', or 'value'.
> >
> > I'd like to kill extract(), but it does have a reason for being, and I
> > couldn't in any good conscience support removing it without a
> > replacement that's at least marginally better.
> >
>
> The security issue regarding extract() are:
> - Unintended variable creation. e.g. $admin_flag = ture
> - Unintended variable modification. e.g. $admin_flag = ture
>
> Instead of removing/deprecating extract(),
> - Add EXTR_EXCEPTION flag that throws exception for overwriting.
> (There are many flags, but no error/exception flag. This isn't good)
> - Require prefix. (User may still set '' as prefix)
>
> We may do:
> - Add EXTR_EXCEPTION flag in 7.3
> - Make all three parameters required parameter set EXTR_ECEPTION a default
> flag in 8.0
>
> With this way, we'll have better compatibility with older PHP and better
> security with extract().
>
> https://github.com/php/php-src/blob/master/ext/standard/array.c#L2459
> Since it checks valid flags, we wouldn't have compatibility for current
> versions unless
> released versions code is modified for it.
>
> Regards,
>
> --
> Yasuo Ohgaki
> yohg...@ohgaki.net
