# Closure::call() to access private data, really ?

- Thread: https://externals.io/message/85855
- Original archive: https://news-web.php.net/php.internals/85855
- Started by Julien Pauli on 2015-04-17T16:11:01+00:00
- Messages: 9
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #85855 — Julien Pauli — 2015-04-17T16:11:01+00:00

URL: https://externals.io/message/85855#85855

Just a simple use case showing how dangerous that is :

<?php
$p = function($p) { $this->$p = new Stdclass; };
$p->call($e = new Exception, 'trace');
throw $e;


That nearly crashes PHP7.
http://3v4l.org/fJj22

(The same trick using Reflection with PHP5 crashes the engine, this is a
known bug that we chose not to fix).


I am absolutely not confident at all having Closure::call() beeing able to
access private data.

Private is private.

For internal classes, this is even worse, and could lead to crash (look at
the example about Exception).
Changing a variable that is private, is absolutely NOT expected from the
class designer.

For User classes, this could lead to information leaks or bad behaviors,
and for internal classes this is even worse and could easilly segfault or
with a little bit of more magic and brain sauce, lead to memory disclosures.

I would like we re-open the debate about accessign private data with
external code (closure), especially in a write context.


Julien.P

---

## #85856 — Ferenc Kovacs — 2015-04-17T16:43:49+00:00

URL: https://externals.io/message/85855#85856 · In reply to: #85855

> Just a simple use case showing how dangerous that is :
>
> <?php
> $p = function($p) { $this->$p = new Stdclass; };
> $p->call($e = new Exception, 'trace');
> throw $e;
>
>
> That nearly crashes PHP7.
> http://3v4l.org/fJj22
>
> (The same trick using Reflection with PHP5 crashes the engine, this is a
> known bug that we chose not to fix).
>
>
> I am absolutely not confident at all having Closure::call() beeing able to
> access private data.
>
> Private is private.
>
> For internal classes, this is even worse, and could lead to crash (look at
> the example about Exception).
> Changing a variable that is private, is absolutely NOT expected from the
> class designer.
>
> For User classes, this could lead to information leaks or bad behaviors,
> and for internal classes this is even worse and could easilly segfault or
> with a little bit of more magic and brain sauce, lead to memory
> disclosures.
>
> I would like we re-open the debate about accessign private data with
> external code (closure), especially in a write context.
>
>
> Julien.P
>

For the record this isn't exclusive to Closure::call, afaik the same thing
is possible with Closure::bind() since 5.4
http://3v4l.org/hlFS4
And there are/were a decent amount of discussion (and AFAIR even hydrate
libraries using this trick) about this, for example:
http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
So while I think that for 7.0 we could remove this behavior, the cat is
already out of the bag, as you can see from my paste you can use that code
to cause segfaults for php >=5.4.0, so I think it would be better to fix
the internal classes to properly handle/validate their properties instead
of blindly trusting their types.

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #85857 — Andrea Faulds — 2015-04-17T17:11:40+00:00

URL: https://externals.io/message/85855#85857 · In reply to: #85856

Hi,

I'm no longer subscribed to internals so I don't know if this email will show up there. If not, feel free to forward it.

> 
> 
> 
> 
> > […]
> 
> For the record this isn't exclusive to Closure::call, afaik the same thing is possible with Closure::bind() since 5.4
> http://3v4l.org/hlFS4
> And there are/were a decent amount of discussion (and AFAIR even hydrate libraries using this trick) about this, for example:
> http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
> So while I think that for 7.0 we could remove this behavior, the cat is already out of the bag, as you can see from my paste you can use that code to cause segfaults for php >=5.4.0, so I think it would be better to fix the internal classes to properly handle/validate their properties instead of blindly trusting their types.

Right, what Closure::call() does isn't particularly new. I'm not sure if it's a good thing that you can arbitrarily re-scope closures, but that's how it is. 

Also, if information leaks are a worry, we've had things like `ob_start()` and `var_dump()`, and (array) for a while now.

A further thought: type hints for class properties would be helpful here.

Thanks.

--
Andrea Faulds
http://ajf.me/

---

## #85858 — Marco Pivetta — 2015-04-17T20:03:56+00:00

URL: https://externals.io/message/85855#85858 · In reply to: #85856

> For the record this isn't exclusive to Closure::call, afaik the same thing
> is possible with Closure::bind() since 5.4
> http://3v4l.org/hlFS4
> And there are/were a decent amount of discussion (and AFAIR even hydrate
> libraries using this trick) about this, for example:
>
> http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
>

Also note that this is the only way to access private properties by-ref
(yes, I do need that for my horrible use-cases. No, don't ask about it.)

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

---

## #85871 — kelerest123@gmail.com — 2015-04-19T20:23:10+00:00

URL: https://externals.io/message/85855#85871 · In reply to: #85858

My opinion is, that we simply have to change PHP closures' scoping to be
more similar with JavaScript functions' scope. That would solve the current
challenge pointed on by @jpauli, thus closure would have access to private
object's members only if it was defined in that object's class / method
definition, so then engine can know that it was programmers intention. I
mean something like this:

<?php

$p = function() { $this->privateMember = new stdClass; };

class C {
    private $privateMember;
    private $closure;
    function __construct() {
        $this->closure = function() {
            $this->privateMember = 32;
        };
    }
    function callClosure() {
        $this->closure->call($this);
    }
}

$c = new C;
$c->callClosure(); // would succeed
$p->call($c); // Fatal error

?>

What do you think about this kind of behavior?


Best regards,
Kubo2


2015-04-17 22:03 GMT+02:00 Marco Pivetta <ocramius@gmail.com>:

> 
>
> > […]
> thing
> > […]
> http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
> > […]
>
> Also note that this is the only way to access private properties by-ref
> (yes, I do need that for my horrible use-cases. No, don't ask about it.)
>
> Marco Pivetta
>
> http://twitter.com/Ocramius
>
> http://ocramius.github.com/

---

## #85873 — Stanislav Malyshev — 2015-04-19T22:21:04+00:00

URL: https://externals.io/message/85855#85873 · In reply to: #85855

Hi!

> Just a simple use case showing how dangerous that is :
> 
> <?php
> $p = function($p) { $this->$p = new Stdclass; };
> $p->call($e = new Exception, 'trace');
> throw $e;

Yes, this is not good, and this is the consequence of allowing to rebind
closures. I'm not sure though how to fix it except for banning closures
from assuming scope of internal classes. If you assigned the scope of
Exception to it, it should have access to Exception - that's how the
scope works.

> For User classes, this could lead to information leaks or bad behaviors,

I'm not sure what you mean by "information leaks", but the behavior is
on whoever wrote that code. People can write bad code, we can't disallow
this. We can restrict things that would really break (like segfault) but
I'm not sure what else we can do here. If you have the scope of the
class, that implies access to the private members of the class. So we
either have to not let the closure have the scope, or invent some
additional term of scope' that is not like real scope. I don't think
it'd be good.

-- 
Stas Malyshev
smalyshev@gmail.com

---

## #85900 — Julien Pauli — 2015-04-22T12:18:21+00:00

URL: https://externals.io/message/85855#85900 · In reply to: #85873

On Mon, Apr 20, 2015 at 12:21 AM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> Hi!
>
> > […]
>
> Yes, this is not good, and this is the consequence of allowing to rebind
> closures. I'm not sure though how to fix it except for banning closures
> from assuming scope of internal classes. If you assigned the scope of
> Exception to it, it should have access to Exception - that's how the
> scope works.
>
> > […]
>
> I'm not sure what you mean by "information leaks", but the behavior is
> on whoever wrote that code. People can write bad code, we can't disallow
> this. We can restrict things that would really break (like segfault) but
> I'm not sure what else we can do here. If you have the scope of the
> class, that implies access to the private members of the class. So we
> either have to not let the closure have the scope, or invent some
> additional term of scope' that is not like real scope. I don't think
> it'd be good.
>


Yup Stas.

Perhaps the best thing to do is to forbid rebinding a Closure to an
internal class ?

I'm sure we could find segfaulting behaviors using such a trick on most of
our internals classes, aka mysqli, simplexmlelement, PDO or SPL classes.

Thoughts ?

Julien.P

---

## #85908 — Andrea Faulds — 2015-04-23T04:08:34+00:00

URL: https://externals.io/message/85855#85908 · In reply to: #85900

Hey Julien,

> 
> 
> Perhaps the best thing to do is to forbid rebinding a Closure to an internal class ?
> 
> I'm sure we could find segfaulting behaviors using such a trick on most of our internals classes, aka mysqli, simplexmlelement, PDO or SPL classes.
> 
> Thoughts ?

Sounds good to me! There’s already a precedent for having internal classes special with respect to Closure binding, as you can’t bind a method of an internal class to an object not of that class.

Thanks.
--
Andrea Faulds
http://ajf.me/

---

## #85983 — Stanislav Malyshev — 2015-04-28T05:30:55+00:00

URL: https://externals.io/message/85855#85983 · In reply to: #85900

Hi!

> Perhaps the best thing to do is to forbid rebinding a Closure to an
> internal class ?

So I have checked into this further, and the good news is that we don't
have too many internal classes that declare private/protected
properties. Actually, the only one where you could mess stuff up was
Exception, and that seems to be recently fixed by adding typechecks for
(almost) unrelated unserialize issue. So for now, I do not see a lot of
ways to mess things up in core PHP. Of course, third-party extensions
may be different.

Still, I don't like the idea of rebinding to internal classes too much
(in fact, I'm not sure I like the whole rebinding thing as such in
general, but that's another story) so here's a pull that forbids this:

https://github.com/php/php-src/pull/1253

Please review and comment.
-- 
Stas Malyshev
smalyshev@gmail.com