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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@
"knplabs/knp-components": "~1.3",
"guzzlehttp/guzzle": "~6.0",
"onelogin/php-saml": "^3.0",
"symfony/dom-crawler": "~3.4"
"symfony/dom-crawler": "~3.4",
"brumann/polyfill-unserialize": "^1.0"
},
"require-dev": {
"behat/behat": "@stable",
Expand Down
5 changes: 4 additions & 1 deletion main/admin/career_diagram.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 */


$cidReset = true;
require_once __DIR__.'/../inc/global.inc.php';

Expand Down Expand Up @@ -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('career', $item['value']);
$html .= Career::renderDiagramByColumn($graph, $tpl);
} else {
Display::addFlash(
Expand Down
2 changes: 1 addition & 1 deletion main/admin/gradebook_list.php
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@

$options = [];
if (!empty($categoryData['depends'])) {
$list = unserialize($categoryData['depends']);
$list = api_unserialize_content('not_allowed_classes', $categoryData['depends']);
foreach ($list as $itemId) {
$courseInfo = api_get_course_info_by_id($itemId);
$options[$itemId] = $courseInfo['name'];
Expand Down
5 changes: 4 additions & 1 deletion main/auth/sso/sso.Drupal.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,9 @@ public function generateProfileEditingURL($userId = 0, $asAdmin = false)
*/
private function decode_cookie($cookie)
{
return unserialize(base64_decode($cookie));
return api_unserialize_content(
'not_allowed_classes',
base64_decode($cookie)
);
}
}
5 changes: 4 additions & 1 deletion main/auth/sso/sso.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,9 @@ public function generateProfileEditingURL($userId = 0, $asAdmin = false)
*/
private function decode_cookie($cookie)
{
return unserialize(base64_decode($cookie));
return api_unserialize_content(
'not_allowed_classes',
base64_decode($cookie)
);
}
}
7 changes: 6 additions & 1 deletion main/course_home/course_home.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
/* For licensing terms, see /license.txt */

use ChamiloSession as Session;
use Fhaculty\Graph\Graph;

/**
* HOME PAGE FOR EACH COURSE.
Expand Down Expand Up @@ -392,7 +393,11 @@
);

if (!empty($item) && isset($item['value']) && !empty($item['value'])) {
$graph = unserialize($item['value']);
/** @var Graph $graph */
$graph = api_unserialize_content(
'career',
$item['value']
);
$diagram = Career::renderDiagram($careerInfo, $graph);
}
}
Expand Down
14 changes: 7 additions & 7 deletions main/exercise/hotspot_admin.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@
$objAnswer = new Answer($questionId);
}

$color = unserialize($color);
$reponse = unserialize($reponse);
$comment = unserialize($comment);
$weighting = unserialize($weighting);
$hotspot_coordinates = unserialize($hotspot_coordinates);
$hotspot_type = unserialize($hotspot_type);
$destination = unserialize($destination);
$color = api_unserialize_content('not_allowed_classes', $color);
$reponse = api_unserialize_content('not_allowed_classes', $reponse);
$comment = api_unserialize_content('not_allowed_classes', $comment);
$weighting = api_unserialize_content('not_allowed_classes', $weighting);
$hotspot_coordinates = api_unserialize_content('not_allowed_classes', $hotspot_coordinates);
$hotspot_type = api_unserialize_content('not_allowed_classes', $hotspot_type);
$destination = api_unserialize_content('not_allowed_classes', $destination);
unset($buttonBack);
}

Expand Down
5 changes: 4 additions & 1 deletion main/exercise/question.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -1145,7 +1145,10 @@ public function search_engine_edit(
$se_doc = $di->get_document((int) $se_ref['search_did']);
if ($se_doc !== false) {
if (($se_doc_data = $di->get_document_data($se_doc)) !== false) {
$se_doc_data = unserialize($se_doc_data);
$se_doc_data = api_unserialize_content(
'not_allowed_classes',
$se_doc_data
);
if (isset($se_doc_data[SE_DATA]['type']) &&
$se_doc_data[SE_DATA]['type'] == SE_DOCTYPE_EXERCISE_QUESTION
) {
Expand Down
3 changes: 2 additions & 1 deletion main/exercise/upload_exercise.php
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,8 @@ function lp_upload_quiz_action_handling()
$lpObject = Session::read('lpobject');

if (!empty($lpObject)) {
$oLP = unserialize($lpObject);
/** @var learnpath $oLP */
$oLP = api_unserialize_content('lp', $lpObject);
if (is_object($oLP)) {
if ((empty($oLP->cc)) || $oLP->cc != api_get_course_id()) {
$oLP = null;
Expand Down
8 changes: 7 additions & 1 deletion main/extra/upgrade_school_calendar.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
/* For licensing terms, see /license.txt */

// not used??

exit;

require_once '../inc/global.inc.php';
Expand All @@ -28,6 +29,11 @@
$d_number = (int) $d_number;
$sql4 = "UPDATE set_module SET cal_day_num = $d_number WHERE id = $d_id ";
Database::query($sql4);
print_r(unserialize(Security::remove_XSS($_POST['aaa'])));
print_r(
api_unserialize_content(
'not_allowed_classes',
Security::remove_XSS($_POST['aaa'])
)
);

Display::display_footer();
11 changes: 6 additions & 5 deletions main/gradebook/lib/be/category.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -264,12 +264,13 @@ public function setIsRequirement($isRequirement)
*/
public function setCourseListDependency($value)
{
$result = [];
if (@unserialize($value) !== false) {
$result = unserialize($value);
}
$this->courseDependency = [];

$this->courseDependency = $result;
$unserialized = api_unserialize_content('not_allowed_classes', $value, true);

if (false !== $unserialized) {
$this->courseDependency = $unserialized;
}
}

/**
Expand Down
117 changes: 115 additions & 2 deletions main/inc/lib/api.lib.php
Original file line number Diff line number Diff line change
@@ -1,10 +1,43 @@
<?php
/* For licensing terms, see /license.txt */

use Brumann\Polyfill\Unserialize;
use Chamilo\CoreBundle\Entity\SettingsCurrent;
use Chamilo\CourseBundle\Component\CourseCopy\Course;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Announcement;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Attendance;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\CalendarEvent;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\CourseCopyLearnpath;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\CourseCopyTestCategory;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\CourseDescription;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\CourseSession;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Document;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Forum;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\ForumCategory;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\ForumPost;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\ForumTopic;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Glossary;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\GradeBookBackup;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Link;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\LinkCategory;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Quiz;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\QuizQuestion;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\QuizQuestionOption;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\ScormDocument;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Survey;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\SurveyInvitation;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\SurveyQuestion;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Thematic;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\ToolIntro;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Wiki;
use Chamilo\CourseBundle\Component\CourseCopy\Resources\Work;
use Chamilo\CourseBundle\Entity\CItemProperty;
use Chamilo\UserBundle\Entity\User;
use ChamiloSession as Session;
use Fhaculty\Graph\Graph;
use Fhaculty\Graph\Set\Edges;
use Fhaculty\Graph\Set\Vertices;
use Fhaculty\Graph\Set\VerticesMap;
use Symfony\Component\Finder\Finder;

/**
Expand Down Expand Up @@ -2766,8 +2799,11 @@ function api_get_plugin_setting($plugin, $variable)

if (isset($result[$plugin])) {
$value = $result[$plugin];
if (@unserialize($value) !== false) {
$value = unserialize($value);

$unserialized = api_unserialize_content('not_allowed_classes', $value, true);

if (false !== $unserialized) {
$value = $unserialized;
}

return $value;
Expand Down Expand Up @@ -9298,3 +9334,80 @@ function api_get_relative_path($from, $to)

return implode('/', $relPath);
}

/**
* Unserialize content using Brummann\Polyfill\Unserialize.
*
* @param string $type
* @param string $serialized
* @param bool $ignoreErrors. Optional.
*
* @return mixed
*/
function api_unserialize_content($type, $serialized, $ignoreErrors = false)
{
switch ($type) {
case 'career':
case 'sequence_graph':
$allowedClasses = [Graph::class, VerticesMap::class, Vertices::class, Edges::class];
break;
case 'lp':
$allowedClasses = [
learnpath::class,
learnpathItem::class,
aiccItem::class,
scormItem::class,
Link::class,
LpItem::class,
];
break;
case 'course':
$allowedClasses = [
Course::class,
Announcement::class,
Attendance::class,
CalendarEvent::class,
CourseCopyLearnpath::class,
CourseCopyTestCategory::class,
CourseDescription::class,
CourseSession::class,
Document::class,
Forum::class,
ForumCategory::class,
ForumPost::class,
ForumTopic::class,
Glossary::class,
GradeBookBackup::class,
Link::class,
LinkCategory::class,
Quiz::class,
QuizQuestion::class,
QuizQuestionOption::class,
ScormDocument::class,
Survey::class,
SurveyInvitation::class,
SurveyQuestion::class,
Thematic::class,
ToolIntro::class,
Wiki::class,
Work::class,
stdClass::class,
];
break;
case 'not_allowed_classes':
default:
$allowedClasses = false;
}

if ($ignoreErrors) {
return @Unserialize::unserialize(
$serialized,
['allowed_classes' => $allowedClasses]
);
}

return Unserialize::unserialize(
$serialized,
['allowed_classes' => $allowedClasses]
);
}
2 changes: 1 addition & 1 deletion main/inc/lib/array.lib.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function array_unique_dimensional($array)
$array = array_unique($array);

foreach ($array as &$myvalue) {
$myvalue = unserialize($myvalue);
$myvalue = api_unserialize_content('not_allowed_clases', $myvalue);
}

return $array;
Expand Down
6 changes: 4 additions & 2 deletions main/inc/lib/plugin.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -322,10 +322,12 @@ public function get($name)
$settings = $this->get_settings();
foreach ($settings as $setting) {
if ($setting['variable'] == $this->get_name().'_'.$name) {
$unserialized = api_unserialize_content('not_allowed_classes', $setting['selected_value'], true);

if (!empty($setting['selected_value']) &&
@unserialize($setting['selected_value']) !== false
false !== $unserialized
) {
$setting['selected_value'] = unserialize($setting['selected_value']);
$setting['selected_value'] = $unserialized;
}

return $setting['selected_value'];
Expand Down
5 changes: 3 additions & 2 deletions main/inc/lib/plugin.lib.php
Original file line number Diff line number Diff line change
Expand Up @@ -437,8 +437,9 @@ public function getPluginInfo($plugin_name, $forced = false)
$settings_filtered = [];
foreach ($plugin_settings as $item) {
if (!empty($item['selected_value'])) {
if (@unserialize($item['selected_value']) !== false) {
$item['selected_value'] = unserialize($item['selected_value']);
$unserialized = api_unserialize_content('not_allowed_classes', $item['selected_value'], true);
if (false !== $unserialized) {
$item['selected_value'] = $unserialized;
}
}
$settings_filtered[$item['variable']] = $item['selected_value'];
Expand Down
2 changes: 1 addition & 1 deletion main/inc/lib/statistics.lib.php
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ public static function getActivitiesData(
} else {
if (!empty($row[2])) {
$originalData = str_replace('\\', '', $row[2]);
$row[2] = unserialize($originalData);
$row[2] = api_unserialize_content('not_allowed_classes', $originalData);
if (is_array($row[2]) && !empty($row[2])) {
$row[2] = implode_with_key(', ', $row[2]);
} else {
Expand Down
3 changes: 2 additions & 1 deletion main/lp/aicc_api.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@

// Is this needed? This is probabaly done in the header file.
$file = Session::read('file');
$oLP = unserialize(Session::read('lpobject'));
/** @var learnpath $oLP */
$oLP = api_unserialize_content('lp', Session::read('lpobject'));
$oItem = $oLP->items[$oLP->current];
if (!is_object($oItem)) {
error_log('New LP - scorm_api - Could not load oItem item', 0);
Expand Down
6 changes: 5 additions & 1 deletion main/lp/aicc_hacp.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@

// Is this needed? This is probabaly done in the header file.
$file = Session::read('file');
$oLP = unserialize(Session::read('lpobject'));
/** @var learnpath $oLP */
$oLP = api_unserialize_content(
'not_allowed_classes',
Session::read('lpobject')
);
$oItem = &$oLP->items[$oLP->current];
if (!is_object($oItem)) {
error_log('New LP - aicc_hacp - Could not load oItem item', 0);
Expand Down
2 changes: 1 addition & 1 deletion main/lp/learnpath.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -12690,7 +12690,7 @@ public static function getLpFromSession($courseCode, $lpId, $user_id)
$learnPath = null;
$lpObject = Session::read('lpobject');
if ($lpObject !== null) {
$learnPath = unserialize($lpObject);
$learnPath = api_unserialize_content('lp', $lpObject);
if ($debug) {
error_log('getLpFromSession: unserialize');
error_log('------getLpFromSession------');
Expand Down
3 changes: 2 additions & 1 deletion main/lp/lp_controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ function(reponse) {
if ($debug) {
error_log(' SESSION[lpobject] is defined');
}
$oLP = unserialize($lpObject);
/** @var learnpath $oLP */
$oLP = api_unserialize_content('lp', $lpObject);
if (isset($oLP) && is_object($oLP)) {
if ($debug) {
error_log(' oLP is object');
Expand Down
Loading