Skip to content

Fix unserialize Vulnerability #2821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 1, 2019
Merged

Fix unserialize Vulnerability #2821

merged 8 commits into from
Mar 1, 2019

Conversation

AngelFQC
Copy link
Member

No description provided.

@AngelFQC AngelFQC added the Bug label Feb 27, 2019
@AngelFQC AngelFQC added this to the 1.11.10 milestone Feb 27, 2019
@chamilo chamilo deleted a comment from FlintCIBot Feb 27, 2019
@AngelFQC AngelFQC marked this pull request as ready for review February 27, 2019 20:29
@chamilo chamilo deleted a comment from FlintCIBot Feb 27, 2019
@jmontoyaa
Copy link
Member

Qué es esto? "Fix unserialize"? Había un "unserialize" antes? Cuál es la tarea?
No estoy de acuerdo con modificar todos esos archivos dentro de main.
Son cambios muy fuertes en 1.11.x
No tengo idea de lo que hace ese código, no hay ninguna referencia 👎

@AngelFQC AngelFQC changed the title Fix unserialize Fix unserialize Vulnerability Feb 28, 2019
@AngelFQC
Copy link
Member Author

@jmontoyaa es para corregir una vulnerabilidad de unserialize.

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

@jmontoyaa
Copy link
Member

No agregar "use Brumann\Polyfill\Unserialize;" por todos lados.
Mejor centralizar todo en una función api. Modificar lo mínimo los archivos dentro de "/main".

Algo como:


$type = 'career';
$value = 'my data...';
$serialized = api_serialize_content($type, $value);

//---------------------
    function api_serialize_data($type, $value)
    {
        switch ($type) {
            case 'not_allowed_classes':
                 ///-something...
             break;
            case 'career':
                $graph = Unserialize::unserialize(
                    $value,
                    [
                        'allowed_classes' => [
                            Graph::class,
                            VerticesMap::class,
                            Vertices::class,
                            Edges::class,
                        ],
                    ]
                );
                break;
        }
    }


@AngelFQC
Copy link
Member Author

Corregido

@@ -106,7 +108,8 @@
$tpl = new Template(get_lang('Diagram'));
$html = Display::page_subheader2($careerInfo['name'].$urlToString);
if (!empty($item) && isset($item['value']) && !empty($item['value'])) {
$graph = unserialize($item['value']);
/** @var Graph $graph */
$graph = api_unserialize_content('carrer', $item['value']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

career ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bien visto!
Corregido

@@ -14,6 +14,8 @@
ALTER TABLE extra_field_values modify column value longtext null;
*/

use Fhaculty\Graph\Graph;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya no se necesita ese "use" supongo ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sí... se usa para un /** @var Graph $graph */

@ywarnier
Copy link
Member

ywarnier commented Feb 28, 2019

@jmontoyaa wrote:

No tengo idea de lo que hace ese código, no hay ninguna referencia

Pero sí estás en copia de los correos de seguridad donde hemos estado explicando todo ;-)

@jmontoyaa
Copy link
Member

@ywarnier no sabía que era el mismo tema.

Copy link
Member

@ywarnier ywarnier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now. Thanks @jmontoyaa for the additional review!

@ywarnier ywarnier merged commit ba97eda into 1.11.x Mar 1, 2019
@ywarnier ywarnier deleted the fix-unserialize branch March 1, 2019 17:33
@NicoDucou
Copy link
Member

It generates an error in the learning path, but I can only reproduce it with a scorm.
To reproduce import the course which zip is in https://11.chamilo.org/main/document/showinframes.php?cidReq=NICODU&id_session=0&gidReq=0&gradebook=0&origin=&id=2799

And then got to the learning path tool and edit the the learning path setting and click on the "Hide table of contents frame" checkbox and validate.

You then get the following error :

Fatal error: main(): The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition "scorm" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide an autoloader to load the class definition in /furanet/sites/11.chamilo.org/web/htdocs/main/lp/lp_controller.php on line 237

You can directly reproduce the error from here : https://11.chamilo.org/main/lp/lp_controller.php?cidReq=JEUXSERIEUXORTHOGRAPHE&id_session=0&gidReq=0&gradebook=0&origin=&action=edit&lp_id=36 clicking on the "Hide table of contents frame" checkbox and validating.

AngelFQC added a commit that referenced this pull request Mar 2, 2019
@AngelFQC
Copy link
Member Author

AngelFQC commented Mar 2, 2019

It should be fixed now

@NicoDucou
Copy link
Member

All good, thank you. 👍

@ywarnier
Copy link
Member

ywarnier commented Mar 4, 2019

I also have an error when restoring a backup, about CourseRestorer::restore_gradebook()
(The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition "Category" of the object you are trying to operate on was loaded before unserialize() gets called or provide an autoloader to load the class definition in /var/www/chamilo111x/src/Chamilo/CourseBundle/Component/CourseCopy/CourseRestorer.php on line 3442)

restore-gradebook

@jmontoyaa
Copy link
Member

I have the same error Yannick reported.

@AngelFQC
Copy link
Member Author

AngelFQC commented Mar 4, 2019

Fixed here be4f22b c18601c

And moving the api_unserialize_content to its own class file.

AngelFQC added a commit that referenced this pull request Mar 4, 2019
AngelFQC added a commit that referenced this pull request Mar 5, 2019
AngelFQC added a commit that referenced this pull request Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants