Very weak access control

[mudrd8mz]

Scripts like visible.php, favorite.php sort.php and others have very weak access control.

The only check you do is isloggedin(). Since then, you blindly rely on the passed parameters like $userid. As a result, any logged in user can change the setting of the block of any other user.

There is no protection against CSRF via sesskey.

As I understand it, these scripts are actually server side handlers for AJAX requests. Note the documentation on the recommended way to implement such scripts - e.g. https://docs.moodle.org/dev/AJAX_pre_2.9

[mudrd8mz]

Your AJAX handlers should make use of $USER and require_sesskey() at least.

[rachel-smith]

Added checks for sesskey and $USER in toggle, visible, favorite, sort files as follows:
if( !isloggedin() || !confirm_sesskey(){ die();} else if($USER->sesskey == false){ die();}

[mudrd8mz]

I do not understand what is the point of if($USER->sesskey == false) part. What I meant was that the script should simply call require_sesskey() and that $USER->id could be used instead of $userid = required_param('userid', PARAM_INT); as long as these scripts are supposed to alter the current user's environment only.

[rachel-smith]

$USER->id is now being used instead of $userid=requried_param('userid', PARAM_INT) on master branch. Currently cannot get require_sesskey() to work, as it causes the following issue: Post: 404 (not found) on line 58 of jquery-1.9.1.min.js when put in the sort.php file, or any file attempting to make database changes to the block.

[rachel-smith]

Currently confirm_sesskey() and require_sesskey() with no parameters passed to the functions cause 404 errors. However it seems to work when confirm_sesskey($USER->sesskey) is used.

[mudrd8mz]

404? that's weird is not it?

[rachel-smith]

Yes it is weird. I have attached a photo of the error message. I get this message when I try to re-order the courses, which calls sort.php (the file containing confrim_sesskey() with no parameters). This error is shown and then the courses are not saved in the correct order.

[upats]

Hi David,

I made a post in the moodle dev forum a while back asking about this. I included a guess as to why it might be giving a 404 error: https://moodle.org/mod/forum/discuss.php?d=325690

Do you think this might be a bug?

Thanks!

[rachel-smith]

I think this is starting to make more sense. I've updated both visible.php and sort.php on the devel branch, still working on implementing it in favorite.php. These files now make use of confirm_sesskey() with the sesskey passed in the courses.js file.

[rachel-smith]

The sesskey check is now working in visible, sort and favorite php files, code is up to date on devel branch.

[upats]

javascript handlers should now be secure passing sesskey() over HTTPS

[mudrd8mz]

I don't think this is a correct fix. The code like

if (!isloggedin() || confirm_sesskey(sessid)) {
    die();
}

if wrong for several reasons:

• If something like that, then it should be !confirm_sesskey() (negate operator)
• If you really want to pass an explicit seskey value, then it should be a variable. If you had debugging enabled and actually watched the error log, you must have seen it reported.
• Ideally should be controlled by a capability.

As said above, I believe you should simply use require_sesskey() as all other Moodle code does.

[upats]

HI David,
Thanks for looking at the new code. We will try to use the info you gave us in your post on the Moodle forums here https://moodle.org/mod/forum/discuss.php?d=325690 from a while back.

[rachel-smith]

Ok, I've changed it to require_sesskey() as you suggested. I've uploaded it to the devel branch, and will merge with master shortly.

[rachel-smith]

Master branch now up to date, closing issue.

[upats]

This issue does not appear to be fixed per @mudrd8mz . We may need to reevaluate the permissions system for this plugin to figure out if there's an alternative solution that will work properly with Ajax.

@Syxton --do you know the best practice for incorporating Moodle permissions checks with ajax calls?

[Syxton]

I am looking at the latest version of sort.php and comparing it to lib/ajax/blocks.php. The only real difference I see is that sort.php calls !isloggedin() and the other example says require_login(). Using the $USER variable was the most important change along with sending the sessionkey via ajax param to confirm on the backend. The only other thing we could possibly do is have a permission check, to be sure the user making the request has permission to perform each action. However, this should probably be done before the user sees an (edit, move, hide, collapse, favorite) icon / link in the first place. But the user is already limited to only their own content, and each user has been given full control over their own list. So while these permissions could be added to allow for certain users to be prevented from using certain features, I don't see it as necessary for security reasons.

After looking at the code and the comments, I'm unsure exactly what else needs to be done. It seems that the recommendations have been met.

[upats]

Thanks for taking a look. We thought it looked fine as well but @mudrd8mz sent me a PM on the moodle forums saying that after his last look at the code, it still didn't look like it had been fixed. I'll try to get him to read your message to see if he can offer any other feedback or if maybe he thinks it is actually okay to go. It'd be really nice to finally get this dang thing in the plugin database.

[upats]

I believe we used !isloggedin() because the require_login() kept giving us 404 errors when sending post data to the ajax handlers.

[mudrd8mz]

Thanks guys for working on these improvements. I agree it looks much better than it was before. I did not manage to find the relevant messages regarding this, but I believe it was related to those parts that @Syxton refers to in

to be sure the user making the request has permission to perform each action.

That is essential part of Moodle security and is even highlighted at https://docs.moodle.org/dev/Plugin_contribution_checklist#Security

Always check that the user has appropriate capabilities before displaying the widgets and before taking the actual action.

As far as I can see, there is no way in your code to control who can actually use it. Most Moodle features can be granularly configured via capabilities and roles. So that was the part I was referring to.

Anyway, in this particular case I can understand it may be seen as a missing feature that would be added in the future. The essential problems reported initially here seem to be fixed.