-
Bug
-
Resolution: Fixed
-
Major
-
2.6.1, 2.7
-
MOODLE_26_STABLE, MOODLE_27_STABLE
-
MOODLE_26_STABLE, MOODLE_27_STABLE
-
wip_
MDL-43918_m28_cm -
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:
- Create a test user
- 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.
- Login as test user in a different browser
- Click Calendar > Add event
- Fill in details for a new 'site' event on today's date
- 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)
- 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;
|
}
|
- has a non-specific relationship to
-
-
- Closed
-
-
-
- Closed
-
- has been marked as being related by
-
-
- Closed
-