Moodle
  1. Moodle
  2. MDL-26260

Loophole in quiz 'secure mode' in some themes

    Details

    • Testing Instructions:
      Hide

      1. Create a quiz with Browser security set to "... popup ..."

      2. Attempt the quiz as a student. Try to defeat the security by clicking the the few pixels around the edge of the screen. In particular try to

      • get the context menu to appear
      • start to drag to select some text

      3. Ignore the fact that in Moodle 2.0, you get multiple alerts for each click. That seems to be a YUI bug. If it is causing you problems, you need to upgrade to Moodle 2.1 or later.

      Show
      1. Create a quiz with Browser security set to "... popup ..." 2. Attempt the quiz as a student. Try to defeat the security by clicking the the few pixels around the edge of the screen. In particular try to get the context menu to appear start to drag to select some text 3. Ignore the fact that in Moodle 2.0, you get multiple alerts for each click. That seems to be a YUI bug. If it is causing you problems, you need to upgrade to Moodle 2.1 or later.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      15873

      Description

      When attempting a quiz in secure mode as a student, it attempts to disable right click by using JavaScript. When a right click is detected, a popup box comes up, but then gets stuck in a loop which forces the user to have to close the page which cancels the quiz attempt. Added a detach call in to the function so it does not loop until another right click event is called. Also disabled the double alert boxes appearing by disabling the message from appearing when the context menu tries to appear.

      Whilst testing this, another loophole was detected where you could right click on the boundary of the page and the right click context menu would appear just fine. This was because it was only triggering the event on 'document.body'. Changing this to 'document' seems to have patched this.

        Issue Links

          Activity

          Show
          Jason Ilicic added a comment - My fix is available on GitHub: https://github.com/jasonilicic/moodle/commit/353f56bdfea40fb8aa1ffcb28d1a86842ff8df7b
          Hide
          Tim Hunt added a comment -

          This was reported and fixed a few weeks ago. Please search for existing bug reports before creating a new one.

          Show
          Tim Hunt added a comment - This was reported and fixed a few weeks ago. Please search for existing bug reports before creating a new one.
          Hide
          Tim Hunt added a comment -

          Oh, I see, the change from document to document.body, probably is still needed. Have you checked that still works in all browsers? (Perhaps a better way to phrase that is, which browsers have you tested so far?)

          Show
          Tim Hunt added a comment - Oh, I see, the change from document to document.body, probably is still needed. Have you checked that still works in all browsers? (Perhaps a better way to phrase that is, which browsers have you tested so far?)
          Hide
          Jason Ilicic added a comment -

          The browser I was testing with was IE8, but I'm not sure if this exists in FireFox or Chrome as I only found this as I was testing for compatibility.

          Show
          Jason Ilicic added a comment - The browser I was testing with was IE8, but I'm not sure if this exists in FireFox or Chrome as I only found this as I was testing for compatibility.
          Hide
          Tim Hunt added a comment -

          I just found time to look at your patch, and I don't really like it.

          To start with, including mod/quiz/module.js.orig in the commit is unhelpful.

          And I really don't like the handle.detatch(); lines. There is no handle variable in scope anywhere. This may work on IE, but is it really a defined, reliable API?

          I have worked out what is really going on. It is not an infinite loop. The problem is the Y.delegate calls. That effectively adds an event handler to all the elements in the document. Since any place you click is probably inside numerous nested divs, a single click seems to trigger numerous event handlers, and so you get numerous alerts (but not an infinite loop), and the e.halt() thing does not stop it.

          I tried changing those lines to Y.on('contextmenu', M.mod_quiz.secure_window.prevent, document); That worked in Safari, but did not work at all in Firefox.

          I am also not at all sure about changing document.body to document in the before/after print bit.

          So, I think we need a bit more thought, and a lot more testing in all browsers, before we can add this to Moodle. Sorry about that, and thanks for trying to help.

          Show
          Tim Hunt added a comment - I just found time to look at your patch, and I don't really like it. To start with, including mod/quiz/module.js.orig in the commit is unhelpful. And I really don't like the handle.detatch(); lines. There is no handle variable in scope anywhere. This may work on IE, but is it really a defined, reliable API? I have worked out what is really going on. It is not an infinite loop. The problem is the Y.delegate calls. That effectively adds an event handler to all the elements in the document. Since any place you click is probably inside numerous nested divs, a single click seems to trigger numerous event handlers, and so you get numerous alerts (but not an infinite loop), and the e.halt() thing does not stop it. I tried changing those lines to Y.on('contextmenu', M.mod_quiz.secure_window.prevent, document); That worked in Safari, but did not work at all in Firefox. I am also not at all sure about changing document.body to document in the before/after print bit. So, I think we need a bit more thought, and a lot more testing in all browsers, before we can add this to Moodle. Sorry about that, and thanks for trying to help.
          Hide
          Jason Ilicic added a comment -

          Tim, I've made a few changes since I posted this, obviously because we originally ran into problems with different browsers. However, our client ended up requesting for us to disable the alert box all together as they saw it as being unnecessary - it's probably a better idea that nothing happens rather than getting an alert message every time a user even accidentally clicks anywhere other than a field element.

          Show
          Jason Ilicic added a comment - Tim, I've made a few changes since I posted this, obviously because we originally ran into problems with different browsers. However, our client ended up requesting for us to disable the alert box all together as they saw it as being unnecessary - it's probably a better idea that nothing happens rather than getting an alert message every time a user even accidentally clicks anywhere other than a field element.
          Hide
          Tim Hunt added a comment -

          Yes, I was wondering about removing that alert completely.

          Actually, I don't think that just disabling all functionality with no explanation is a good idea. I think that the best solution is to have a message, but don't use an alert. Instead just some sort of non-modal notification, like a div that appears across the bottom or top of the browser window for a few seconds, and then fades out. Of course, that is more work to implement, and I can't be bothered, but it is a better solution.

          Show
          Tim Hunt added a comment - Yes, I was wondering about removing that alert completely. Actually, I don't think that just disabling all functionality with no explanation is a good idea. I think that the best solution is to have a message, but don't use an alert. Instead just some sort of non-modal notification, like a div that appears across the bottom or top of the browser window for a few seconds, and then fades out. Of course, that is more work to implement, and I can't be bothered, but it is a better solution.
          Hide
          Michael Blake added a comment -

          This issue is causing problems for a MP client. Please give it priority.

          Show
          Michael Blake added a comment - This issue is causing problems for a MP client. Please give it priority.
          Hide
          Tim Hunt added a comment -

          Note that the above github link is broken. I'll do my own fix.

          Show
          Tim Hunt added a comment - Note that the above github link is broken. I'll do my own fix.
          Hide
          Tim Hunt added a comment -

          This fix seems to work, however I have only tested this on IE9.

          Mike, since this has a partner label, can I assume that someone will peer-review and test it soon?

          Show
          Tim Hunt added a comment - This fix seems to work, however I have only tested this on IE9. Mike, since this has a partner label, can I assume that someone will peer-review and test it soon?
          Hide
          Michael Blake added a comment -

          yes Tim, one of the devs at HQ will have a look at it later today

          Show
          Michael Blake added a comment - yes Tim, one of the devs at HQ will have a look at it later today
          Hide
          Sam Hemelryk added a comment -

          Hi Tim,

          Looks good to me testing in Firefox and Chrome as well both fixed.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, Looks good to me testing in Firefox and Chrome as well both fixed. Cheers Sam
          Hide
          Tim Hunt added a comment -

          Thanks Sam.

          So, what remains to be done is for someone who cares to test this in IE9, IE8, ...

          Show
          Tim Hunt added a comment - Thanks Sam. So, what remains to be done is for someone who cares to test this in IE9, IE8, ...
          Hide
          Sam Hemelryk added a comment -

          Hi Tim,
          I'm working from HQ presently and don't have access to IE, I've removed myself from the peer-review position so that someone else at HQ spots it.
          If it's not gone in the next couple of days I'll force someone towards it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, I'm working from HQ presently and don't have access to IE, I've removed myself from the peer-review position so that someone else at HQ spots it. If it's not gone in the next couple of days I'll force someone towards it. Cheers Sam
          Hide
          Tim Hunt added a comment -

          Thanks Sam.

          Show
          Tim Hunt added a comment - Thanks Sam.
          Hide
          Charles Fulton added a comment - - edited

          I can't officially review this, but here's what I encountered. The behavior in IE9 is still inconsistent in 2.0. If I right-click in page-footer, I get the box five times. In page-content, seven times. In region-main-box, eleven times. In region-post-box, just the one time. Page-header, four times etc. The right boundary issue was fine though.

          On upgrading the site to 2.1 I did not experience this behavior: everything worked as expected. Ditto 2.2 and 2.3. I'm going to do a fresh checkout of 2.0 to see if I missed something.

          EDIT: Still the same in 2.0 on a fresh checkout.

          Show
          Charles Fulton added a comment - - edited I can't officially review this, but here's what I encountered. The behavior in IE9 is still inconsistent in 2.0. If I right-click in page-footer, I get the box five times. In page-content, seven times. In region-main-box, eleven times. In region-post-box, just the one time. Page-header, four times etc. The right boundary issue was fine though. On upgrading the site to 2.1 I did not experience this behavior: everything worked as expected. Ditto 2.2 and 2.3. I'm going to do a fresh checkout of 2.0 to see if I missed something. EDIT: Still the same in 2.0 on a fresh checkout.
          Hide
          Tim Hunt added a comment -

          The problem with the alert appearing several times in IE is a separate bug, that I guess only got fixed in 2.1 and later.

          So, I think that is an OK result for IE9, I think we still need to test IE8, and probably IE7.

          Show
          Tim Hunt added a comment - The problem with the alert appearing several times in IE is a separate bug, that I guess only got fixed in 2.1 and later. So, I think that is an OK result for IE9, I think we still need to test IE8, and probably IE7.
          Hide
          Rajesh Taneja added a comment -

          Code looks good Tim,
          Although, the issue seems to persist. With or without the patch, recursive pop-up is only visible in 2.0 branch and not in 2.1+
          FYI:
          Tested on IE7, 8 and 9.

          Show
          Rajesh Taneja added a comment - Code looks good Tim, Although, the issue seems to persist. With or without the patch, recursive pop-up is only visible in 2.0 branch and not in 2.1+ FYI: Tested on IE7, 8 and 9.
          Hide
          Tim Hunt added a comment -

          Rajesh, I am confused. You say both:

          "Code looks good"

          and

          "the issue seems to persist"

          So, is this fix good or not? Also, When you say 'the issue', which issue is that you are referring to?

          Show
          Tim Hunt added a comment - Rajesh, I am confused. You say both: "Code looks good" and "the issue seems to persist" So, is this fix good or not? Also, When you say 'the issue', which issue is that you are referring to?
          Hide
          Rajesh Taneja added a comment -

          Hello Tim,
          Changing document.body to document make sense. But recursive pop-up issue (original issue, described in description) still persist and can be reproducible on ie 7, 8 and 9, for Moodle 2.0

          Sorry for not being so clear earlier.

          Show
          Rajesh Taneja added a comment - Hello Tim, Changing document.body to document make sense. But recursive pop-up issue (original issue, described in description) still persist and can be reproducible on ie 7, 8 and 9, for Moodle 2.0 Sorry for not being so clear earlier.
          Hide
          Tim Hunt added a comment -

          Thank for the clarification Rajesh.

          The weird thing is, if you diff mod/quiz/module.js between 2.0.x and 2.1.x, there are no differences that could explain the recursive pop-up issues. At least none that I can see.

          How confusing.

          Show
          Tim Hunt added a comment - Thank for the clarification Rajesh. The weird thing is, if you diff mod/quiz/module.js between 2.0.x and 2.1.x, there are no differences that could explain the recursive pop-up issues. At least none that I can see. How confusing.
          Hide
          Tim Hunt added a comment -

          So, presumably the difference is between YUI 3.2 in Moodle 2.0 and YUI 3.4.1 in Moodle 2.1

          Sorry, I cannot fix that. And 2.0 is now in 'security fix only mode'.

          Therefore, I am going to submit the changes here, which are a good improvement, for integration.

          If anyone else wants to work on the 2.0 issue, please be my guest. Create a new MDL, submit a patch, and I will review it and submit it for integration.

          Thanks everyone who tested this.

          Show
          Tim Hunt added a comment - So, presumably the difference is between YUI 3.2 in Moodle 2.0 and YUI 3.4.1 in Moodle 2.1 Sorry, I cannot fix that. And 2.0 is now in 'security fix only mode'. Therefore, I am going to submit the changes here, which are a good improvement, for integration. If anyone else wants to work on the 2.0 issue, please be my guest. Create a new MDL, submit a patch, and I will review it and submit it for integration. Thanks everyone who tested this.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Aparup Banerjee added a comment -

          Hi all,
          Thanks for all work here, the patch has been integrated now into 20, 21, 22 and master.

          ps: about the other 2.0 issue (yui issue), perhaps a separate issue for 2.0 could be created (or linked if existing) for others to work on.

          anyway up for testing, (even though it seems pretty well tested).

          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - Hi all, Thanks for all work here, the patch has been integrated now into 20, 21, 22 and master. ps: about the other 2.0 issue (yui issue), perhaps a separate issue for 2.0 could be created (or linked if existing) for others to work on. anyway up for testing, (even though it seems pretty well tested). cheers, Aparup
          Hide
          Andrew Davis added a comment -

          I've come across some issues that may or may not be related to this bug. I'm not even sure they're things we can do anything about.

          I did not come across any problems like an alert being generated in a loop like is described in the original issue description.

          The issue title mentions a theme but I cant see any other mention of a particular theme. Im using "Standard".

          The issues I noticed:

          1) On the full screen popup in both FF 9.0.1 and Chrome 16.0.912.77 it was possible for the student to grab the edge of the popup window and drag the window smaller to get to the window behind. The window appears to be set to the height and width of the screen but not actually maximised.

          2) If you right click the secure window both browsers display a popup that says "This functionality is currently disabled". After 3 or 4 right clicks both browsers give the user a check box labelled "Prevent this page from creating additional dialogs".

          If you tick it in Chrome subsequent right clicks do nothing.

          If you tick it in Firefox subsequent right clicks show the browsers default right click menu.

          Show
          Andrew Davis added a comment - I've come across some issues that may or may not be related to this bug. I'm not even sure they're things we can do anything about. I did not come across any problems like an alert being generated in a loop like is described in the original issue description. The issue title mentions a theme but I cant see any other mention of a particular theme. Im using "Standard". The issues I noticed: 1) On the full screen popup in both FF 9.0.1 and Chrome 16.0.912.77 it was possible for the student to grab the edge of the popup window and drag the window smaller to get to the window behind. The window appears to be set to the height and width of the screen but not actually maximised. 2) If you right click the secure window both browsers display a popup that says "This functionality is currently disabled". After 3 or 4 right clicks both browsers give the user a check box labelled "Prevent this page from creating additional dialogs". If you tick it in Chrome subsequent right clicks do nothing. If you tick it in Firefox subsequent right clicks show the browsers default right click menu.
          Hide
          Andrew Davis added a comment -

          I'm logging off for a while so Im putting this back in the waiting for testing pool so someone can pick this up if there is any change of circumstance while Im away.

          Show
          Andrew Davis added a comment - I'm logging off for a while so Im putting this back in the waiting for testing pool so someone can pick this up if there is any change of circumstance while Im away.
          Hide
          Aparup Banerjee added a comment -

          looks good to pass for me

          Show
          Aparup Banerjee added a comment - looks good to pass for me
          Hide
          Jason Fowler added a comment -

          Test passed

          Show
          Jason Fowler added a comment - Test passed
          Hide
          Jason Fowler added a comment -

          I couldn't get the popup working on my installs for some reason, assuming browser issues, but tested it on Aparup's machine and it worked fine

          Show
          Jason Fowler added a comment - I couldn't get the popup working on my installs for some reason, assuming browser issues, but tested it on Aparup's machine and it worked fine
          Hide
          Tim Hunt added a comment -

          Thanks for the detailed testing. Just to add some more explanation. What secure window is doing is using really all the obnoxious things that JavaScript can do, to make life difficult for the students. These are exactly the types of things that advertisers like to abuse (forced full-screen pop-ups, lots of alerts, block context menu, ...) so over the years browser makers have made it harder and harder to keep secure mode working.

          Speaking as someone who uses the web, I applaud what browser makers have done.

          I only try to maintain 'secure' mode because a lot of quiz user think it is necessary. The OU does not use it at all.

          Show
          Tim Hunt added a comment - Thanks for the detailed testing. Just to add some more explanation. What secure window is doing is using really all the obnoxious things that JavaScript can do, to make life difficult for the students. These are exactly the types of things that advertisers like to abuse (forced full-screen pop-ups, lots of alerts, block context menu, ...) so over the years browser makers have made it harder and harder to keep secure mode working. Speaking as someone who uses the web, I applaud what browser makers have done. I only try to maintain 'secure' mode because a lot of quiz user think it is necessary. The OU does not use it at all.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: