SQL injection from unsanitized user input

[2shediac]

Hi - when doing a security review of your plugin we noticed several instances where you are saving data directly from POST variables.

For example in gadd.php starting at line 32: $record->mootyper = $_POST['rpSityperId']; $record->userid = $_POST['rpUser']; $record->grade = 0; $record->mistakes = $_POST['rpMistakesInput']; $record->timeinseconds = $_POST['rpTimeInput']; $record->hitsperminute = $_POST['rpSpeedInput']; $record->fullhits = $_POST['rpFullHits'];

etc.

This looks like it leaves the plugin open to PHP Object Injection and this would need to be corrected prior to installation on our system.

There are other examples of this through the plugin.

Also in settings.php there is the following line:

file_put_contents($CFG->dirroot.'/mod/mootyper/styles.css', $data);

Writing to webroot is also not allowed since it can present a security risk.

Let me know if you need more information.

[drachels]

I have been working for some time on eliminating the _POST instances. The Moodle documentation does not seem to have any examples that I have been able to find, but I think I have figured out a way to fix them. I still have at least two more instances to fix, before I try a new release.

As for the file_put_contents($CFG->dirroot.'/mod/mootyper/styles.css', $data); - Do you have a suggestion to do it another way? Or do you know of another way to be able to use a color setting for the keyboard background and keytops? Quite frankly, your admin account should be the only one able to get to that page due to the defined('MOODLE_INTERNAL') || die; code about line 27, and if they exploit it, then you have a much bigger security risk to your whole site.

[2shediac]

Hi - in terms of sanitizing user input there are a couple of methods that I know of to allow you to do it easily - see: https://coderwall.com/p/ccti9q/quick-and-easy-way-to-sanitize-post-data-in-php, and here https://security.stackexchange.com/questions/42498/which-is-the-best-way-to-sanitize-user-input-in-php among others.

Regarding the file_put_contents, security policies on servers preclude any writing to the dirroot folder. You could either store the value in a database table, or in config variable. Some themes have examples or I can see if I can find something specific for you if you want..

[drachels]

Hi Derek - Thanks for the links. After days of digging around in Moodle core and other plugins, I think I have fixed all the instances of _POST in gadd.php and gcnext.php, and have pushed them to the master branch. Still have a few more to look at in eedit.php, eins.php, and mod_setup.php.

Regarding the file_put_contents, I have never be able to get storing the value in a database table or as a config variable to work, as in this case, the purpose is to change a background color in the css file for the plugin. No one in the themes forum was able to help when I asked for alternatives. If you are able to show me examples or another way to do the color changes, I would appreciate it.

[2shediac]

Hi - if you look at settings.php?section=themesettings, then look at custom menu items, or user menu items, or settings.php?section=additionalhtml when body is opened - there should be examples there on how to store things.

Does that help?

Thanks

[drachels]

The problem I ran in to when trying to do it like themes do, is that it "IS" a theme setting then, and only works with the one I set it to use. Let's say I set it up for Boost and you want to use Clean? The settings no longer work/show for you. What I came up with works for ANY theme by altering the styles.php file that gets read by Moodle when admins click Save changes while viewing the MooTyper settings page...you just have to trust your admin user. Unfortunately, Moodle HQ has not implemented any way that I know of adding a universal css setting that can be easily changed. We are stuck with using a styles.php file for a plugin, but there is no way to easily change it "on the fly" to reflect what users "want" things to look like on their site, no matter which theme they use.

The alternative would be to tell admins how to go in and make changes to the styles.css file by hand...but then we are right back to having to trust an admin to manually do what I have automated, and then upload the changed file. If you are using Boost for the whole site, and you make one simple typo error in styles.css, you then break formatting for the WHOLE site. I know this from personal experience...just one misspelled word broke Boost for days.

I will keep looking for an alternative

[2shediac]

Can you post what would be in a typical file? Wondering if they are small enough they could be stored in a db...

[drachels]

A typical styles.css file for any given plugin contains the additional css information to make the plugin "look" the way it is supposed to. The one for MooTyper controls the placement of the text to enter, the text the user types, links to view the resulting grades, as well as all the mistakes, hits per minute, precision, etc. The styles.php file for MooTyper is in the mootyper root directory.

Saving style settings in the "db" is not the problem. The problem is to make it so that Moodle applies the style settings no matter which user opens MooTyper and also no matter which theme that user is currently using.

[drachels]

Changes are currently in master branch. Need to do a little more testing before new release.

[2shediac]

Let me know when a release is available for Moodle 3.1

[drachels]

Unfortunately, my wife is scheduled for heart test/surgery tomorrow. That and other commitments means it might take me a while to get to it. You can get 90% of the changes by simply grabbing the gadd.php and gcnext.php files from the new MooTyper331 branch and use them to replace the ones in your MooTYper 3.1.

[2shediac]

My best to you and your wife. I have actually had heart surgery, so know the "fun" involved..

Stay healthy..

[2shediac]

I had a look at the updated code. There are still some un-sanitized GET variables atchk.php, and erm.php etc.

Let us know when you have a release ready.

[drachels]

I am in the process of backporting all the code changes to MOODLE_321_STABLE. Will try to finish tomorrow.

[2shediac]

Thanks - when this is all ready let me know and I am happy to test. Glad you liked the fix I pushed up

[drachels]

Except for 4 _POST's, all the _GET and _POST changes are now in the master branch.

[drachels]

Down to only one _POST left to fix and it is in mod_setup.php in line 62. So far, after two days and lots of hours, every idea I've tried does not give the same "performance" for the three checkboxes on the page.

[2shediac]

How about some thing like this: $val =mysql_real_escape_string($_POST);

if (empty($val)) {

I would think that would do it... It would satisfy our security issues anyway

[drachels]

If I try to use mysql, then doesn't that break MooTyper for everyone who uses mysqli or mssql or postgresql? The dev site I am working on uses mysqli, so I had to change it there, at least. Plus, to actually use it you have to also use a DB connection such as, $con=mysqli_connect("localhost","my_user","my_password","my_db"); Doing so then bypasses Moodle's DB handling, doesn't it? Ever since I started using this plugin, even before I adopted it to maintain it, I never really cared for the fact it had a non-standard settings setup where you made some settings when creating the activity, and then had an additional settings page, that you had to access after the initial activity creation. My current thought is just to move everything on the mod_setup.php page, over to the mod_form.php page.

[drachels]

Well, it took a while longer than I had hoped, but I think I have a working fix. I made a duplicate of the mod_setup.php file, stripped everything out except the code for showkeyboard, and figured out how to eliminate the, onchange="this.form.submit()", that was using the if, (empty($_POST)). I then figured out how to save the setting to the DB using onclick="this.form.submit();. Once I had it working like the _POST method, I started adding the rest of the settings back in, making sure they were saving each setting only when the Confirm button was clicked, instead of onchange for each individual setting.

Just need to clean out all my 'debugging' code and verify it still works correctly, and then I will push a change to github. Hopefully before the end of my workday.

[drachels]

I just pushed the changes to the master branch. Now to try and get all those new changes into a branch for Moodle 3.1 and lower.

[drachels]

Hi Derek, The changes are in master and new MooTyper 3.4.0. So I have tested this new version on Moodle 2.7 (php 5.6) and on Moodle 3.0, 3.2.5+, 3.3.2+ and 3.4rc1 with these four using php 7.1. Seems to work okay on each. I will be interested to know if it works okay for you on Moodle 3.1. I have to run tests on Moodle 2.7 thru 3.4 as this version should work on all of them. It has charts that only show for Moodle 3.2 and higher, while hidden on 3.1 and lower.

[2shediac]

Hi this all looks good to me. Its been approved and will be available on our sites (subject to my partner validating my checks) within a few days.

Let me know if I can do anything else, and hope your wife is doing good.

[drachels]

Thanks for the feedback and your help, Derek. My wife and I are both doing better, which is why I was able to spend the time needed to get the last pesky _POST eliminated.