Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43918

Denial of service and possible SQL injection in get_coursemodule_from_id() and get_coursemodule_from_instance()




      get_coursemodule_from_id() and get_coursemodule_from_instance() both include the following SQL snippet:

       FROM {course_modules} cm
                         JOIN {modules} md ON md.id = cm.module
                         JOIN {".$modulename."} m ON m.id = cm.instance

      since there is no cleaning of $modulename this is potentially vulnerable to SQL injection, although I have not yet been able to find a way for a user to pass in a module name in order to exploit this.

      However, the same issue does allow for a kind of denial of service attack by a less privileged user. To reproduce:

      1. Create a test user
      2. Edit the role permissions to allow 'moodle/calendar:manageentries' for authenticated users. This is just for testing purposes, this issue would only be exploitable by users with this capability.
      3. Login as test user in a different browser
      4. Click Calendar > Add event
      5. Fill in details for a new 'site' event on today's date
      6. Via the browser inspector, modify the "value" of the "modulename" hidden input form field. Set a value of '1234'. (this could also be done via CURL etc)
      7. Create the event

      What happens

      The test user gets an SQL error after submitting the form, but the event is created so now all other users will get an SQL error when visiting a page that displays the calendar (including the site homepage):

      Debug info: ERROR: syntax error at or near "{"
      LINE 4: JOIN {1234} m ON m.id = cm.instance
      SELECT cm.*, m.name, md.name AS modname 
      FROM mdl_course_modules cm
      JOIN mdl_modules md ON md.id = cm.module
      JOIN {1234} m ON m.id = cm.instance
      WHERE m.id = $1 AND md.name = $2
      [array (
      0 => '0',
      1 => '1234',
      Error code: dmlreadexception
      Stack trace:
      line 441 of /lib/dml/moodle_database.php: dml_read_exception thrown
      line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      line 742 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
      line 1463 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
      line 1387 of /lib/datalib.php: call to moodle_database->get_record_sql()
      line 267 of /calendar/lib.php: call to get_coursemodule_from_instance()
      line 91 of /blocks/calendar_month/block_calendar_month.php: call to calendar_get_mini()
      line 296 of /blocks/moodleblock.class.php: call to block_calendar_month->get_content()
      line 238 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
      line 956 of /lib/blocklib.php: call to block_base->get_content_for_output()
      line 1008 of /lib/blocklib.php: call to block_manager->create_block_contents()
      line 353 of /lib/blocklib.php: call to block_manager->ensure_content_created()
      line 4 of /theme/base/layout/frontpage.php: call to block_manager->region_has_content()
      line 877 of /lib/outputrenderers.php: call to include()
      line 807 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
      line 101 of /index.php: call to core_renderer->header()

      The only way to fix is to manually remove the event from the database.

      The reason this is happening is that $modulename is inserted into the query as a table "{1234}". The regular expression in fix_table_names() that would normally substitute this to add the table prefix fails to match because of the lack of a leading letter. The unmodified SQL statement is not valid so it fails.

      Suggested fix

      Add a check into the functions above to ensure $modulename matches the syntax required by fix_table_names(). For example:

      if (!preg_match('/^[a-z][a-z0-9_]*$/', $modulename)) {
          return false;


          Issue Links



              skodak Petr Skoda
              simoncoggins Simon Coggins
              Peer reviewer:
              Andrew Davis Andrew Davis
              Damyon Wiese Damyon Wiese
              Mark Nelson Mark Nelson
              Component watchers:
              Andrew Lyons, Dongsheng Cai, Huong Nguyen, Jun Pataleta, Michael Hawkins, Shamim Rezaie, Simey Lameze
              0 Vote for this issue
              10 Start watching this issue


                Fix Release Date: