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

Redirect security checks should not introduce additional cURL requests

    XMLWordPrintable

    Details

    • Affected Branches:
      MOODLE_310_STABLE, MOODLE_311_STABLE, MOODLE_39_STABLE
    • Fixed Branches:
      MOODLE_310_STABLE, MOODLE_311_STABLE, MOODLE_39_STABLE
    • Pull 3.9 Branch:
    • Pull 3.10 Branch:
      MDL-72203-310
    • Pull 3.11 Branch:
      MDL-72203-311-2
    • Pull Master Branch:
      MDL-72203-master-2
    • Testing Instructions:
      Hide

      Test 1 - Security fix still handled

      Repeat the same test as in MDL-71916.

      NOTE: Please do not include those testing instructions here (or refer to their specifics within the comments on this issue), as this issue is public and not restricted as a security issue.Anything that may be considered security specific (such as referring to steps around reproducing/confirming that the vulnerability is still covered by this patch) should be limited to comments that are restricted to the moodle-security group, rather than Viewable by All Users.

      Test 2 - Additional request bug addressed (tested using ngrok)

      1. Add an image file to your web root (eg /var/www/html/test.jpg).
      2. Enable ngrok (ngrok http 80). Copy the ngrok forwarding http URL (and keep the ngrok terminal/console where you can see it).
      3. Log in as any user, navigate to the private file area.
      4. Open the URL downloader and paste the URL to the test.jpg image hosted through the ngrok address. Then press the "Download" button.
      5. Check the ngrok HTTP Requests list and CONFIRM there is only one GET request.
      Show
      Test 1 - Security fix still handled Repeat the same test as in MDL-71916. NOTE: Please do not include those testing instructions here (or refer to their specifics within the comments on this issue), as this issue is public and not restricted as a security issue.Anything that may be considered security specific (such as referring to steps around reproducing/confirming that the vulnerability is still covered by this patch) should be limited to comments that are restricted to the moodle-security group, rather than Viewable by All Users . Test 2 - Additional request bug addressed ( tested using ngrok ) Add an image file to your web root (eg /var/www/html/test.jpg ). Enable ngrok ( ngrok http 80 ). Copy the ngrok forwarding http URL (and keep the ngrok terminal/console where you can see it). Log in as any user, navigate to the private file area. Open the URL downloader and paste the URL to the test.jpg image hosted through the ngrok address. Then press the "Download" button. Check the ngrok HTTP Requests list and CONFIRM there is only one GET request.

      Description

      The original security issue MDL-71916 fix was released in 3.11.1 and it introduced an extra native cURL call inside curl_security_helper to check if the given URL triggers a redirect or not.

      Shortly after the release, a couple of regressions were reported as a result of the integrated solution, as it could unintentionally cause some requests to fail (such as those using one-time access URLs/tokens, as they do not support being requested more than once). It was agreed to revert the fix in MDL-71916 and progress with implementing the alternative approach as outlined by Brendan there:

      So I think we should tell curl to not follow redirects, and then if it comes back with a redirect header then we loop in php, call $this->check_securityhelper_blocklist again and then replace the curl again. This will mean we had to re-implement the logic around correctly handling a 301 / 304 307 differently. This way if there is no redirect then we don't waste a second call.

      This issue is created to provide this alternative fix.

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              mudrd8mz David Mudrák (@mudrd8mz)
              Reporter:
              mudrd8mz David Mudrák (@mudrd8mz)
              Peer reviewer:
              Michael Hawkins Michael Hawkins
              Integrator:
              Jun Pataleta Jun Pataleta
              Tester:
              Anna Carissa Sadia Anna Carissa Sadia
              Participants:
              Component watchers:
              Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias, Sujith Haridasan, Matteo Scaramuccia, Andrew Lyons, Dongsheng Cai, Huong Nguyen, Jun Pataleta, Michael Hawkins, Shamim Rezaie, Simey Lameze
              Votes:
              2 Vote for this issue
              Watchers:
              17 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Fix Release Date:
                29/Jul/21

                  Time Tracking

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