Details
-
Type:
Improvement
-
Status:
Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 1.6.7, 1.7.5, 1.8.6, 1.9.2
-
Fix Version/s: 1.9.3
-
Component/s: Administration, Installation, Security Alert
-
Labels:None
-
Affected Branches:MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE
-
Fixed Branches:MOODLE_19_STABLE
Description
The attached patch adds additional checks for the moodledata directory during early installation phases (much earlier than the current check inside admin/index.php), uses stronger warning messages, lets the user click on a link to actually check whether the moodle data directory is accessible from the web or not (which is far easier than trying to check it from the server itself, as it is quite complicated and depend on a lot of configuration factors) and refuses to continue the installation if the moodledata directory appears to be accesible, unless the user explicitly confirms that s/he has verified the directory is not accesible. See the attached image (called install.png) to view all the details mentionned above.
In addition to it, the current check inside admin/index.php is extended to visually notify the admin about the potential problem with moodledata, as it may happen that moodle has been installed with some automated installer that completely hides the installation process, including the warnings and the confirmation checkbox (fantastico is an example of this). So the patchs adds a visual notification about the potential problem to the administration block notifications area (see attached image called admin_block.png). When you click on it, it displays a stronger warning than the current one, lets the administration click on a link to check the moodledata directory accessibility from the web and offers a button to remove the warning (both from the administration block, and the admin notifications page).
Given that there are thousands of Moodle install all over the world with their moodledata directory open to anyone, and that having access to moodledata basically means you can do whatever you want with that moodle install (you can steal the admin user session, for example), I think it's really important to add this check & visual notification to help those people configure their sites correctly.
I'm attaching patches for 1.5, 1.6, 1.7. 1.8, 1.9 and HEAD current as of today.
Saludos. Iñaki.
Attachments
Activity
- All
- Comments
- History
- Activity
- Source
- Test Sessions
hmm, I do not think the code in is_dataroot_insecure() is 100% accurate for all setups, clicking on link may not always validate that moodledata is not accessible
I am not sure I like the option to remove the warning, many people either do not understand English enough (not all lang packs will have it translated) or click anything. If implemented it should be IMO user preference, not site setting. We have already discussed some ideas related to general notification framework, unfortunately there was not enough time to do that during 1.9dev last year 
I agree that this security warning should be more annoying and visible - different CSS (red color?), bigger font (bold?), different wording.
The changes in installer is a good idea. The target="_new" is not strict and the <u> tag is not nice. Maybe the wording could be improved a bit.
assigning to Martin for final decision 
I agree that this security warning should be more annoying and visible - different CSS (red color?), bigger font (bold?), different wording.
The changes in installer is a good idea. The target="_new" is not strict and the <u> tag is not nice. Maybe the wording could be improved a bit.
assigning to Martin for final decision I know the code in is_data_root_insecure() is not 100% proof. There are lots of ways to configure your web server to deliver your content even if the tests in is_data_root_insecure() fail to noice it, with tricks like location aliasing, filesystem symlinking, url rewriting and so on.
But I posit that those savvy enough to do all those tricks don't need this check and the associated warnings. It's the non-technical people that use a shared hosting setup who need this. Specially if they use the-easy-to-setup but brain-damaged (wrt the final configuration) Fantastico installer.
Whether you make the option to remove the warning a site setting or an (admin) user preference, I don't really mind. As to the lang packs not translating the text, it'll be no worse than the current situation, where a user can be vulnerable and still not know it. So this can only be an improvement 
Regarding the '_new', <u> tag and the improved wording, feel free to change it as much as you want. All I really want is the general functionality of the patch to be added to the core as fast as possible, so vulnerable people can get a warning of their current situation as soon as possible.
Saludos. Iñaki.
Regarding the '_new', <u> tag and the improved wording, feel free to change it as much as you want. All I really want is the general functionality of the patch to be added to the core as fast as possible, so vulnerable people can get a warning of their current situation as soon as possible.
Saludos. Iñaki. Maybe I am missing something here, let me sum up the dataroot problem:
1/ July 2006 - Steve Lord reported that misconfigured sites are vulnerable - Google search confirmed this is a real problem
2/ August 2006 - I have proposed code that changed default (often not safe) dataroot location in installer and also added warning on admin/index.php, please note the admin/index.php is displayed right after installation and after EACH upgrade; Several people including Iñaki, TIm, Howard really helped to improve my solution.
3/ I have also done some more Google research at that time - it was really scary We have notified several large sites before the release.
4/ September 2006 - Moodle 1.6.2 released - the first version with warning included; after this many ppl asked in Moodle.org forums "how do I fix that new warning on admin page?"
I really like this patch, but:
- it helps only for sites that do upgrades - if I search for allinurl:moodledata/sessions I get around 1k of results and many of them are repeated hits, many of them show dates from 2006 - conclusion: warnings did not help to fix abandoned or not maintained sites
- not many people are upgrading to latest cvs nightlies or weeklies - conclusion: we do not need to rush the commits here, it should be enough to do it before next releases
- we must finalise the language strings before commit and coordinate this with translators - conclusion: we have to wait for ppl to get from vacations
We have notified several large sites before the release.
4/ September 2006 - Moodle 1.6.2 released - the first version with warning included; after this many ppl asked in Moodle.org forums "how do I fix that new warning on admin page?"
I really like this patch, but:
- it helps only for sites that do upgrades - if I search for allinurl:moodledata/sessions I get around 1k of results and many of them are repeated hits, many of them show dates from 2006 - conclusion: warnings did not help to fix abandoned or not maintained sites
- not many people are upgrading to latest cvs nightlies or weeklies - conclusion: we do not need to rush the commits here, it should be enough to do it before next releases
- we must finalise the language strings before commit and coordinate this with translators - conclusion: we have to wait for ppl to get from vacations
Lowering the security level now that Petr has removed the problematic section from the patch
Saludos. Iñaki.
Saludos. Iñaki. Please test and reopen if any problems found,
please file a new issue if you volunteer to backport this improvement into older branches
Petr
Petr I think that my development environment confuses the new code.
In /var/www I have a moodledata folder writable by the web server, and a symbolic link from moodle -> /home/tim/Workspace/moodle_head/. So in config.php I have
$CFG->wwwroot = 'http://tim.moodle.com/moodle';
$CFG->dirroot = '/home/tim/Workspace/moodle_head';
$CFG->dataroot = '/var/www/moodledata';
I believe this is insecure, but I don't see the admin warning.
(If I try changing $CFG->dirroot to "/var/www/1.9"; then I get told to change it back, which I think is good.)
Looking at the code in is_dataroot_insecure it is hard to see how to generalise it to cover cases like this (I know, why don't we try to find and parse the Apache config file ). Anyway, I just thought I should report what I am seeing.
). Anyway, I just thought I should report what I am seeing. 
I said I was attaching patches for 1.5 too, but I was wrong. 1.6 is the oldest version I have patches for. Sorry for the confusion.
Saludos. Iñaki.