Skip to content

Commit

Permalink
MDL-33791 Portfolio: Fixed security issue with passing file paths.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mdjnelson @stronk7
mdjnelson authored and stronk7 committed Nov 7, 2012
1 parent f89a1a8 commit 3774324
Show file tree
Hide file tree
Showing 22 changed files with 486 additions and 386 deletions.
3 changes: 2 additions & 1 deletion lang/en/portfolio.php
View file
Expand Up @@ -145,7 +145,8 @@
$string['mustsetcallbackoptions'] = 'You must set the callback options either in the portfolio_add_button constructor or using the set_callback_options method';
$string['noavailableplugins'] = 'Sorry, but there are no available portfolios for you to export to';
$string['nocallbackclass'] = 'Could not find the callback class to use ({$a})';
$string['nocallbackfile'] = 'Something in the module you\'re trying to export from is broken - couldn\'t find a required file ({$a})';
$string['nocallbackcomponent'] = 'Could not find the component specified {$a}.';
$string['nocallbackfile'] = 'Something in the module you\'re trying to export from is broken - couldn\'t find a required portfolio file';
$string['noclassbeforeformats'] = 'You must set the callback class before calling set_formats in portfolio_button';
$string['nocommonformats'] = 'No common formats between any available portfolio plugin and the calling location {$a->location} (caller supported {$a->formats})';
$string['noinstanceyet'] = 'Not yet selected';
Expand Down
5 changes: 3 additions & 2 deletions lib/db/install.xml 100644 → 100755
View file
Expand Up @@ -2303,8 +2303,9 @@
<FIELD NAME="time" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false" COMMENT="time of transfer (in the case of a queued transfer this is the time the actual transfer ran, not when the user started)" PREVIOUS="userid" NEXT="portfolio"/>
<FIELD NAME="portfolio" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false" COMMENT="fk to portfolio_instance" PREVIOUS="time" NEXT="caller_class"/>
<FIELD NAME="caller_class" TYPE="char" LENGTH="150" NOTNULL="true" SEQUENCE="false" COMMENT="the name of the class used to create the transfer" PREVIOUS="portfolio" NEXT="caller_file"/>
<FIELD NAME="caller_file" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="path to file to include where the class definition lives. (relative to dirroot)" PREVIOUS="caller_class" NEXT="caller_sha1"/>
<FIELD NAME="caller_sha1" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="sha1 of exported content as far as the caller is concerned (before the portfolio plugin gets a hold of it)" PREVIOUS="caller_file" NEXT="tempdataid"/>
<FIELD NAME="caller_file" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="path to file to include where the class definition lives. (relative to dirroot)" PREVIOUS="caller_class" NEXT="caller_component"/>
<FIELD NAME="caller_component" TYPE="char" LENGTH="255" NOTNULL="false" SEQUENCE="false" COMMENT="the component name responsible for exporting" PREVIOUS="caller_file" NEXT="caller_sha1"/>
<FIELD NAME="caller_sha1" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="sha1 of exported content as far as the caller is concerned (before the portfolio plugin gets a hold of it)" PREVIOUS="caller_component" NEXT="tempdataid"/>
<FIELD NAME="tempdataid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="old id from portfolio_tempdata. This is so that we can gracefully catch a race condition between an external system requesting a file and causing the tempdata to be deleted, before the user gets the &quot;your transfer is requested&quot; page" PREVIOUS="caller_sha1" NEXT="returnurl"/>
<FIELD NAME="returnurl" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="the original &quot;returnurl&quot; of the export - takes us to the moodle page we started from" PREVIOUS="tempdataid" NEXT="continueurl"/>
<FIELD NAME="continueurl" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="the url the external system has set to view the transfer" PREVIOUS="returnurl"/>
Expand Down
15 changes: 15 additions & 0 deletions lib/db/upgrade.php
View file
Expand Up @@ -1399,5 +1399,20 @@ function xmldb_main_upgrade($oldversion) {
upgrade_main_savepoint(true, 2012110201.00);
}

if ($oldversion < 2012110700.01) {

// Define field caller_component to be added to portfolio_log.
$table = new xmldb_table('portfolio_log');
$field = new xmldb_field('caller_component', XMLDB_TYPE_CHAR, '255', null, null, null, null, 'caller_file');

// Conditionally launch add field caller_component.
if (!$dbman->field_exists($table, $field)) {
$dbman->add_field($table, $field);
}

// Main savepoint reached.
upgrade_main_savepoint(true, 2012110700.01);
}

return true;
}
46 changes: 30 additions & 16 deletions lib/portfolio/exporter.php
View file
Expand Up @@ -66,10 +66,10 @@ class portfolio_exporter {
public $instancefile;

/**
* @var string the file to include that contains the class definition of
* @var string the component that contains the class definition of
* the caller object used to re-waken the object after sleep
*/
public $callerfile;
public $callercomponent;

/** @var int the current stage of the export */
private $stage;
Expand Down Expand Up @@ -115,16 +115,16 @@ class portfolio_exporter {
*
* @param portfolio_plugin_base $instance portfolio instance (passed by reference)
* @param portfolio_caller_base $caller portfolio caller (passed by reference)
* @param string $callerfile path to callerfile (relative to dataroot)
* @param string $callercomponent the name of the callercomponent
*/
public function __construct(&$instance, &$caller, $callerfile) {
public function __construct(&$instance, &$caller, $callercomponent) {
$this->instance =& $instance;
$this->caller =& $caller;
if ($instance) {
$this->instancefile = 'portfolio/' . $instance->get('plugin') . '/lib.php';
$this->instance->set('exporter', $this);
}
$this->callerfile = $callerfile;
$this->callercomponent = $callercomponent;
$this->stage = PORTFOLIO_STAGE_CONFIG;
$this->caller->set('exporter', $this);
$this->alreadystolen = array();
Expand Down Expand Up @@ -395,7 +395,13 @@ public function process_stage_confirm() {
foreach ($previous as $row) {
$previousstr .= userdate($row->time);
if ($row->caller_class != get_class($this->caller)) {
require_once($CFG->dirroot . '/' . $row->caller_file);
if (!empty($row->caller_file)) {
portfolio_include_callback_file($row->caller_file);
} else if (!empty($row->caller_component)) {
portfolio_include_callback_file($row->caller_component);
} else { // Ok, that's weird - this should never happen. Is the apocalypse coming?
continue;
}
$previousstr .= ' (' . call_user_func(array($row->caller_class, 'display_name')) . ')';
}
$previousstr .= '<br />';
Expand Down Expand Up @@ -522,15 +528,16 @@ public function process_stage_send() {
public function log_transfer() {
global $DB;
$l = array(
'userid' => $this->user->id,
'portfolio' => $this->instance->get('id'),
'caller_file' => $this->callerfile,
'caller_sha1' => $this->caller->get_sha1(),
'caller_class' => get_class($this->caller),
'continueurl' => $this->instance->get_static_continue_url(),
'returnurl' => $this->caller->get_return_url(),
'tempdataid' => $this->id,
'time' => time(),
'userid' => $this->user->id,
'portfolio' => $this->instance->get('id'),
'caller_file'=> '',
'caller_component' => $this->callercomponent,
'caller_sha1' => $this->caller->get_sha1(),
'caller_class' => get_class($this->caller),
'continueurl' => $this->instance->get_static_continue_url(),
'returnurl' => $this->caller->get_return_url(),
'tempdataid' => $this->id,
'time' => time(),
);
$DB->insert_record('portfolio_log', $l);
}
Expand Down Expand Up @@ -681,7 +688,14 @@ public static function rewaken_object($id) {
if ($exporter->instancefile) {
require_once($CFG->dirroot . '/' . $exporter->instancefile);
}
require_once($CFG->dirroot . '/' . $exporter->callerfile);
if (!empty($exporter->callerfile)) {
portfolio_include_callback_file($exporter->callerfile);
} else if (!empty($exporter->callercomponent)) {
portfolio_include_callback_file($exporter->callercomponent);
} else {
return; // Should never get here!
}

$exporter = unserialize(serialize($exporter));
if (!$exporter->get('id')) {
// workaround for weird case
Expand Down

0 comments on commit 3774324

Please sign in to comment.