Note:

If you want to create a new page for developers, you should create it on the Moodle Developer Resource site.

Hardening new Roles system

From MoodleDocs

New roles add great freedom when assigning rights to students. The problem might arise when students are assigned permission that allows adding of content that is not cleaned before display - such as editting Resources, adding activities, etc. They could then use any type of XSS attack to gain full administrative access without any restrictions.

Proposed solution (updated with feedback and ideas from the rest)

Risk bitmap in capabilities

Add risk bitmap field to each capability. Each bit indicates presence of different risk associated with given capability.

Basic risks

  • access to private personal information - ex: backups with user details, non public information in profile (hidden email), etc.
  • missing XSS protection - entering of uncleaned content such as HTML with javascript or unprotected uploaded files
  • dangerous global configuration changes - various settings that might render whole site unoperable, changing trust bitmaps
  • some more if needed (can be added later)

Implementation

  • add new LONGINT column riskbitmap to table capabilities
  • define risks and assign them to capabilities in mod/xxx/db/access.php
  • link wiki pages with explanation to each risk from capabilities page
  • allow risk based filtering of capabilities admin/roles/manage.php (optional)

The user interface would be minimal, icons and maybe colors indicating each risk together with description links which we need anyway. Developers would be deciding about the risks, the risk assignment would be hardcoded in access description file, no GUI needed.

Benefits

  1. proper documentation of risks associated with capabilities, easy to explain
  2. solid foundation for regular code audits (mainly XSS prevention and personal information disclosure)


User trust bitmap

Indicate what kind of trust each user has. Match the risk bitmap of capability and user trust bitmap in both has_capability() and require_capability()

Implementation

  • add new LONGINT column trustbitmap to user table
  • add capability moodle/site:managetrustbitmaps with dangerous global configuration risk
  • add trust checks to has_capability() and require_capability()
  • add GUI
    • preset trust bitmap for new users
    • changing of trust bitmaps
    • add field to user/edit.php
    • request trust level change form - something like new course request (optional)
  • fix upgrade to assign trust bitmaps based on original teacher or administrator rights
  • patch user import script and synchronizations (optional)

Benefits

  1. This part is optional and can be implemented later.
  2. Trust manager or admin has full control over potentially dangerous capabilities - it is necessary for large sites (or connected sites in the future).
  3. Trust bitmap mechanism can be turned off by single configuration switch (both GUI and checks) - needed for small insecure workshop sites.
  4. General protection against future bugs in role and capability management code.


Trusted text

working on it now...

Proposed solution 1a

This proposal is pretty good, in that it is simple, and will work. I just worry that it is just tring to preseve the existing 7 roles:

minimal
guest
standard
student, non-editing teacher
high
editing teacher, course creator
absolute
admin, oringinal admin

Of course our current security model is build round these concepts, so changing it is a risk. Also, it is clearly not future proof to have 4 hard-coded levels.

As I understand it: the new roles system is secure with the default settings, and it remains secure if the roles and permissions settings at a site are only editing by people who fully understand all the consequences of their actions. It is just the the new system is so powerful that it makes it too easy for people to screw up. Therefore, we need to protect them from themselves. Do I understand the problem correctly?

However, the new roles system is meant to make things more flexible, and at a large installation you might want to trust some people with some of the things you classify as high rist, but not others. For example, you might want to trust them to edit or upload uncleaned content, but not to handle backups nor create courses. Under your model, you would have to give these people high trust level, and then it would be possible give them capabilities that we would rather they did not have by mistake.

So can I suggest a slightly more flexible (and hence slightly more complicated solution). Instead of trust levels, have risks. For example:

  • Access to sensitive information
  • Add uncleaned content
  • Destroy the site

Then for each user, list which risks you trust them not to exploit maliciously. For each cabability, list which risks it opens up.

All the checks you want to perform are still possible under my system. Explaining it is still easy, if not easier, because you can explain the possible consequences of trusting someone one risk at a time. Basically, instead of using integer trust levels and >= operator, we are using sets of risks and the superset operator. Very similar to implement, but much more flexible when we discover new classes of risk in the future.


Proposed solution 1b

I like the idea of the risk system but I wonder if adding it to all users right now will cause a new interface overload. The interface for assigning roles could also be a bit scary to think about.

As a first step, a simpler idea might be just be to add a column to the capabilities table called "risk" which defaults to "0" but could be set to higher numbers that are a mask (list of flags) with the kinds of risk this capability includes.

On the capability screens (eg role definition and override screens) any capabilities with a higher-than-zero entry could flags or colours or warnings next to it to help the user know that these capabilities had possible security or privacy issues that they should be aware of (linking off to the docs wiki for more details).


This is exactly what I am proposing in 1a. Just you call them flags, I call them risks. Your proposed capability screen UI is exactly what I had in mind.

Oh, I see, the bit that 1b is missing is Petr's bit where you give each user a trust rating, the the system double-checks and prevents the user being allowed access to a capability that exceeds the user's trust rating.

Proposed solution 2 (independent of above)

  1. Have one new capability called "moodle/site:trusttext".
  2. Certain roles who you trust to edit text and allow to have Javascript, EMBED etc can have permission for this capability set to "allow" (these people are generally teachers).
  3. When saving a text from a user, modules can call a function on the text and insert a special tag (eg ####TRUST#####) if the current user is trusted (and actively REMOVE all such tags if the user is NOT trusted).
  4. When displaying the text with format_text(), a new parameter needs to be passed to enable the trust system ($options->usetrust = true).
  5. If $options->usetrust is present, each text is checked for ####TRUST#### in the text and output is cleaned appropriately, depending on whether the tag was found. If this new parameter is not present (eg in old modules or 3rd party modules) then all such tags are removed and the text is always fully cleaned.
  6. For caching, the text is stored in the final form as it is now.

Example

Storing the text

Here is the original text from the user:

  Elephant says <script>alert('hello')</script>

Here it's converted before storage:

  $text = apply_trust_to_text($text);

For a trusted user, this will return this text for storage:

  ####TRUST####Elephant says <script>alert('hello')</script>

For an untrusted user, this will return:

  Elephant says <script>alert('hello')</script>

Showing the text

If the tag is found AND the code is trust-enabled then cleaning is NOT done, and only that special tag is removed before output.

 Elephant says <script>alert('hello')</script>

If not found then cleaning is done fully (as it is now in Moodle):

 Elephant says