opis / closure

Serialize closures (anonymous functions)
https://opis.io/closure
MIT License
2.5k stars 81 forks source link

No warning about Object Injection #5

Closed kenjis closed 9 years ago

kenjis commented 9 years ago

I think it is better to add warning about object injection to documentation. If a closure is injected, attacker can execute arbitrary code. It is very dangerous.

See https://www.owasp.org/index.php/PHP_Object_Injection

maciejmrozinski commented 9 years ago

Can You post any example of this issue?

msarca commented 9 years ago

The process of serialization/deserialization is not a dangerous thing nor the using of Opis Closure library. Of course, passing user submitted content to the unserialize function is not a very smart thing to do.

file_put_contents($_POST['filename'], $_POST['content']);

The same thing would be true if someone would write a piece of code like above, but this doesn't mean that web forms are an evil thing and they should never be used ever. It doesn't mean neither that the $_POST variable is very "unsecure" or very dangerous to use.

Returning back to the topic: Opis Closure is a library that can be used to serialize/unserialize closures, not a compilation of "good practice" advices. Therefore, I believe that adding such a warning is beyond library's purposes.

kenjis commented 9 years ago

The eval() language construct is very dangerous because it allows execution of arbitrary PHP code. http://php.net/manual/en/function.eval.php

Opis Closure library is very dangerous because it allows execution of arbitrary PHP code if you misuse.

not a compilation of "good practice" advices

Yes, it is. But if there is something dangerous or pitfalls, it is kind to note to users like eval() in PHP manual.

kenjis commented 9 years ago

@maciejmrozinski If you unserialize data from user input (and execute it), attacker can send arbitrary data, so can execute arbitrary code.

In normal object injection, attacker can't write arbitrary code. But attacker can write arbitrary code in closure.

maciejmrozinski commented 9 years ago

@msarca I must agree with You that Opis Closure library is not responsible for problems like Object Injection.

@kenjis I thought that You find some code bug in library that can be used by attacker. If there is none, Your proposal about adding a warning is unfounded. As msarca said, the programmer mistakes is out of scope of this library and i fully agree with him.

kenjis commented 9 years ago

@maciejmrozinski No, I just say about general warning like eval() in PHP. eval() is a statement to execute PHP code. PHP is not responsible for problems like code injection attack.

Yes, Opis Closure library is not responsible for problems like Object Injection.

But there are a lot of PHP users who don't know about object injection. https://github.com/slimphp/Slim/blob/3a2ac723f17b5d81607287ff28575d38b9fbc70e/Slim/Middleware/SessionCookie.php#L127