Skip to content

Commit

Permalink
MDL-59969 admin: Warn admins if a development libs directory exists
Browse files Browse the repository at this point in the history
We can't really control the direct web access to directories in dirroot,
that is part of the server setup. So we at least warn admins as they may
not realize the risks of having directories like vendor or node_modules
exposed.

Credit goes to Petr Škoda for mentioning the PHPUnit issue CVE-2017-9841
to me.
  • Loading branch information
mudrd8mz authored and David Monllao committed Sep 7, 2017
1 parent a39d297 commit 911fcae
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 2 deletions.
9 changes: 8 additions & 1 deletion admin/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -864,10 +864,17 @@
$eventshandlers = $DB->get_records_sql('SELECT DISTINCT component FROM {events_handlers}');
$themedesignermode = !empty($CFG->themedesignermode);

// Check if a directory with development libraries exists.
if (is_dir($CFG->dirroot.'/vendor') || is_dir($CFG->dirroot.'/node_modules')) {
$devlibdir = true;
} else {
$devlibdir = false;
}

admin_externalpage_setup('adminnotifications');

$output = $PAGE->get_renderer('core', 'admin');

echo $output->admin_notifications_page($maturity, $insecuredataroot, $errorsdisplayed, $cronoverdue, $dbproblems,
$maintenancemode, $availableupdates, $availableupdatesfetch, $buggyiconvnomb,
$registered, $cachewarnings, $eventshandlers, $themedesignermode);
$registered, $cachewarnings, $eventshandlers, $themedesignermode, $devlibdir);
24 changes: 23 additions & 1 deletion admin/renderer.php
Original file line number Diff line number Diff line change
Expand Up @@ -276,12 +276,15 @@ public function upgrade_confirm_abort_install_page(array $abortable, moodle_url
* @param int|null $availableupdatesfetch timestamp of the most recent updates fetch or null (unknown)
* @param string[] $cachewarnings An array containing warnings from the Cache API.
* @param array $eventshandlers Events 1 API handlers.
* @param bool $themedesignermode Warn about the theme designer mode.
* @param bool $devlibdir Warn about development libs directory presence.
*
* @return string HTML to output.
*/
public function admin_notifications_page($maturity, $insecuredataroot, $errorsdisplayed,
$cronoverdue, $dbproblems, $maintenancemode, $availableupdates, $availableupdatesfetch,
$buggyiconvnomb, $registered, array $cachewarnings = array(), $eventshandlers = 0, $themedesignermode = false) {
$buggyiconvnomb, $registered, array $cachewarnings = array(), $eventshandlers = 0,
$themedesignermode = false, $devlibdir = false) {
global $CFG;
$output = '';

Expand All @@ -290,6 +293,7 @@ public function admin_notifications_page($maturity, $insecuredataroot, $errorsdi
$output .= $this->legacy_log_store_writing_error();
$output .= empty($CFG->disableupdatenotifications) ? $this->available_updates($availableupdates, $availableupdatesfetch) : '';
$output .= $this->insecure_dataroot_warning($insecuredataroot);
$output .= $this->development_libs_directories_warning($devlibdir);
$output .= $this->themedesignermode_warning($themedesignermode);
$output .= $this->display_errors_warning($errorsdisplayed);
$output .= $this->buggy_iconv_warning($buggyiconvnomb);
Expand Down Expand Up @@ -520,6 +524,24 @@ protected function insecure_dataroot_warning($insecuredataroot) {
}
}

/**
* Render a warning that a directory with development libs is present.
*
* @param bool $devlibdir True if the warning should be displayed.
* @return string
*/
protected function development_libs_directories_warning($devlibdir) {

if ($devlibdir) {
$moreinfo = new moodle_url('/report/security/index.php');
$warning = get_string('devlibdirpresent', 'core_admin', ['moreinfourl' => $moreinfo->out()]);
return $this->warning($warning, 'error');

} else {
return '';
}
}

/**
* Render an appropriate message if dataroot is insecure.
* @param bool $errorsdisplayed
Expand Down
1 change: 1 addition & 0 deletions lang/en/admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@
$string['deleteuser'] = 'Delete user';
$string['density'] = 'Density';
$string['denyemailaddresses'] = 'Denied email domains';
$string['devlibdirpresent'] = 'Directories with development libraries such as <em>vendor</em> or <em>node_modules</em> should not be present on public sites. See the <a href="{$a->moreinfourl}">security overview report</a> for more details.';
$string['development'] = 'Development';
$string['devicedetectregex'] = 'Device detection regular expressions';
$string['devicedetectregex_desc'] = '<p>By default, Moodle can detect devices of the type default (desktop PCs, laptops, etc), mobile (phones and small hand held devices), tablet (iPads, Android tablets) and legacy (Internet Explorer 6 users). The theme selector can be used to apply separate themes to all of these. This setting allows regular expressions that allow the detection of extra device types (these take precedence over the default types).</p>
Expand Down
6 changes: 6 additions & 0 deletions report/security/lang/en/report_security.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@
$string['check_noauth_error'] = 'The No authentication plugin cannot be used on production sites.';
$string['check_noauth_name'] = 'No authentication';
$string['check_noauth_ok'] = 'No authentication plugin is disabled.';
$string['check_nodemodules_details'] = '<p>The directory <em>{$a->path}</em> contains Node.js modules and their dependencies, typically installed by the NPM utility. These modules may be required for Moodle development. They are not needed to run a Moodle site and they can contain potentially dangerous code exposing your site to remote attacks.</p><p>It is strongly recommended to remove the directory if the site is available via a public URL, or at least prohibit web access to it.</p>';
$string['check_nodemodules_info'] = 'The node_modules directory should not be present on public sites.';
$string['check_nodemodules_name'] = 'Node.js modules directory';
$string['check_openprofiles_details'] = 'Open user profiles can be abused by spammers. It is recommended that either <code>Force users to log in for profiles</code> or <code>Force users to log in</code> are enabled.';
$string['check_openprofiles_error'] = 'Anyone can may view user profiles without logging in.';
$string['check_openprofiles_name'] = 'Open user profiles';
Expand Down Expand Up @@ -121,6 +124,9 @@
$string['check_unsecuredataroot_name'] = 'Insecure dataroot';
$string['check_unsecuredataroot_ok'] = 'Dataroot directory must not be accessible via the web.';
$string['check_unsecuredataroot_warning'] = 'Your dataroot directory <code>{$a}</code> is in the wrong location and might be exposed to the web.';
$string['check_vendordir_details'] = '<p>The vendor directory <em>{$a->path}</em> contains various third-party libraries and their dependencies, typically installed by the PHP Composer. It may be needed for local development, such as for installing the PHPUnit framework. But it can also contain potentially dangerous code exposing your site to remote attacks.</p><p>It is strongly recommended to remove the directory if the site is available via a public URL, or at least prohibit web access to it.</p>';
$string['check_vendordir_info'] = 'The vendor directory should not be present on public sites.';
$string['check_vendordir_name'] = 'Vendor directory';
$string['check_webcron_details'] = '<p>Running the cron from a web browser can expose privileged information to anonymous users. It is recommended to only run the cron from the command line or set a cron password for remote access.</p>';
$string['check_webcron_warning'] = 'Anonymous users can access cron.';
$string['check_webcron_name'] = 'Web cron';
Expand Down
64 changes: 64 additions & 0 deletions report/security/locallib.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ function report_security_get_issue_list() {
return array(
'report_security_check_unsecuredataroot',
'report_security_check_displayerrors',
'report_security_check_vendordir',
'report_security_check_nodemodules',
'report_security_check_noauth',
'report_security_check_embed',
'report_security_check_mediafilterswf',
Expand Down Expand Up @@ -897,3 +899,65 @@ function report_security_check_preventexecpath($detailed = false) {

return $result;
}

/**
* Check the presence of the vendor directory.
*
* @param bool $detailed Return detailed info.
* @return object Result data.
*/
function report_security_check_vendordir($detailed = false) {
global $CFG;

$result = (object)[
'issue' => 'report_security_check_vendordir',
'name' => get_string('check_vendordir_name', 'report_security'),
'info' => get_string('check_vendordir_info', 'report_security'),
'details' => null,
'status' => null,
'link' => null,
];

if (is_dir($CFG->dirroot.'/vendor')) {
$result->status = REPORT_SECURITY_WARNING;
} else {
$result->status = REPORT_SECURITY_OK;
}

if ($detailed) {
$result->details = get_string('check_vendordir_details', 'report_security', ['path' => $CFG->dirroot.'/vendor']);
}

return $result;
}

/**
* Check the presence of the node_modules directory.
*
* @param bool $detailed Return detailed info.
* @return object Result data.
*/
function report_security_check_nodemodules($detailed = false) {
global $CFG;

$result = (object)[
'issue' => 'report_security_check_nodemodules',
'name' => get_string('check_nodemodules_name', 'report_security'),
'info' => get_string('check_nodemodules_info', 'report_security'),
'details' => null,
'status' => null,
'link' => null,
];

if (is_dir($CFG->dirroot.'/node_modules')) {
$result->status = REPORT_SECURITY_WARNING;
} else {
$result->status = REPORT_SECURITY_OK;
}

if ($detailed) {
$result->details = get_string('check_nodemodules_details', 'report_security', ['path' => $CFG->dirroot.'/node_modules']);
}

return $result;
}

0 comments on commit 911fcae

Please sign in to comment.