Details
-
Type:
Sub-task
-
Status:
Closed
-
Priority:
Blocker
-
Resolution: Fixed
-
Affects Version/s: 2.0
-
Fix Version/s: 2.0
-
Component/s: Portfolio API
-
Labels:None
-
Affected Branches:MOODLE_20_STABLE
-
Fixed Branches:MOODLE_20_STABLE
Description
Following code from portfolio/add.php is a security nightmare - allowing anybody (including guests) to include ANY moodle file from dirroot and instantiate ANY class with ANY parameters is unacceptable, please note that there is also no sesskey CSRF protection!
$callbackfile = optional_param('callbackfile', null, PARAM_PATH); // callback file eg /mod/forum/lib.php - the location of the exporting content
$callbackclass = optional_param('callbackclass', null, PARAM_ALPHAEXT); // callback class eg forum_portfolio_caller - the class to handle the exporting content.
$callbackargs = array();
foreach (array_keys(array_merge($_GET, $_POST)) as $key) {
if (strpos($key, 'ca_') === 0) {
if (!$value = optional_param($key, false, PARAM_ALPHAEXT)) {
if (!$value = optional_param($key, false, PARAM_NUMBER)) {
$value = optional_param($key, false, PARAM_PATH);
}
}
// strip off ca_ for niceness
$callbackargs[substr($key, 3)] = $value;
}
}
// righto, now we have the callback args set up
// load up the caller file and class and tell it to set up all the data
// it needs
require_once($CFG->dirroot . $callbackfile);
$caller = new $callbackclass($callbackargs);
Hi Petr,
Adding sesskey protection is trivial and a simple oversight, thank you for catching it. All the portfolio add buttons are handled through the portfolio_add_button's to_html method, so adding sesskey there and a check in portfolio/add.php will change all cases.
Regarding your other concern, do you have any better solution? I followed Martin's original spec for this, please see http://docs.moodle.org/en/index.php?title=Development:Portfolio_API&oldid=23088.
Also, it's not "ANY" class, those classes are all subclasses of portfolio_caller (this can easily be checked in portfolio/add.php).