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

Admin config changes report SQL ORDER BY not limited to ASC/DESC strings

    XMLWordPrintable

    Details

    • Testing Instructions:
      Hide
      1. Log in as admin on a site where some config options have previously been changed.
      2. Visit the following:

        <wwwroot>/report/configlog/index.php?sort=name&dir=DESC&page=0&perpage=30

      3. CONFIRM the rows are sorted by the Setting column in reverse alphabetical order (ie z-a, so something like 'youtube' would be towards the top of the table).
      4. CONFIRM the icon next to the Setting heading is an up arrow.
      5. Click on the "Setting" column heading.
      6. CONFIRM the rows are still sorted by the Setting column, but are now sorted in alphabetical order (ie a-z, so something earlier alphabetically will appear at the top)
      7. CONFIRM the icon next to the Setting heading is a down arrow.
      8. In the address bar of your browser, change "dir=ASC" to "dir=ASCCCCCC" (ie an invalid value) and press enter.
      9. CONFIRM the rows are now sorted in reverse alphabetical order again, with the up arrow icon next to the Setting heading.
      Show
      Log in as admin on a site where some config options have previously been changed. Visit the following: <wwwroot>/report/configlog/index.php?sort=name&dir=DESC&page=0&perpage=30 CONFIRM the rows are sorted by the Setting column in reverse alphabetical order (ie z-a, so something like 'youtube' would be towards the top of the table). CONFIRM the icon next to the Setting heading is an up arrow. Click on the "Setting" column heading. CONFIRM the rows are still sorted by the Setting column, but are now sorted in alphabetical order (ie a-z, so something earlier alphabetically will appear at the top) CONFIRM the icon next to the Setting heading is a down arrow. In the address bar of your browser, change "dir=ASC" to "dir=ASCCCCCC" (ie an invalid value) and press enter. CONFIRM the rows are now sorted in reverse alphabetical order again, with the up arrow icon next to the Setting heading.
    • Affected Branches:
      MOODLE_35_STABLE, MOODLE_36_STABLE, MOODLE_37_STABLE, MOODLE_38_STABLE
    • Fixed Branches:
      MOODLE_38_STABLE
    • Sprint:
      International 4.0 - Sprint 1

      Description

      The admin config changes report (Site admin > Reports > Config changes) accepts a "dir" GET parameter to determine whether to order the report by ASC or DESC. The problem with its implementation is that it injects an alpha filtered version of that value directly into the SQL, instead of using logical operators to insert a predefined string of ASC or DESC.

      Since it does only allow alphabet characers (along with its position in the query), I don't think its susceptible to malicious SQL injection, the result will just be that the query will be rendered invalid and cause an error on the page. I have, however, set this to "could be a security issue" for the time being, pending whoever takes the issue doing some further investigation.

      Note: The "dir" parameter has been removed from Moodle 3.9, so that version and later are not affected.

       

      (Prepared by Michael Hawkins based on report by  Spyridon Chatzimichail, SF case 00076304)

        Attachments

        1. MDL-69129.jpg
          MDL-69129.jpg
          47 kB
        2. MDL-69129-35.mdk.patch
          0.8 kB
        3. MDL-69129-37.mdk.patch
          0.8 kB
        4. MDL-69129-38.mdk.patch
          0.8 kB

          Activity

            People

            Assignee:
            michaelh Michael Hawkins
            Reporter:
            schatzim Spyridon Chatzimichail
            Peer reviewer:
            Mathew May
            Integrator:
            Andrew Nicols
            Tester:
            Anna Carissa Sadia
            Participants:
            Component watchers:
            Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              Fix Release Date:
              13/Jul/20

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 5 hours, 30 minutes
                5h 30m