Moodle
  1. Moodle
  2. MDL-31248

Change in rc4encrypt key is causing cookies encrypted before the change to produce garbage text

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.16, 2.0.7, 2.1.4, 2.2.1
    • Fix Version/s: 1.9.17, 2.0.8, 2.1.5, 2.2.2
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      1) Start with a fresh install of an old version (eg 1.9.14).
      2) Log out and log in a few times.
      3) Upgrade to the current version (pre-patch) logout and observe the garbled characters. Log back in.
      4) Install the patch.

      [TEST] When you log into the site the username field should be blank, or it may possibly have your login details (depends on the browser). Make sure that there is no garbled text.

      Show
      1) Start with a fresh install of an old version (eg 1.9.14). 2) Log out and log in a few times. 3) Upgrade to the current version (pre-patch) logout and observe the garbled characters. Log back in. 4) Install the patch. [TEST] When you log into the site the username field should be blank, or it may possibly have your login details (depends on the browser). Make sure that there is no garbled text.
    • Workaround:
      Hide

      Removing the proposed workaround. Sorry, I thought this was a newly introduced setting, but it was not.

      Show
      Removing the proposed workaround. Sorry, I thought this was a newly introduced setting, but it was not.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-31248-master-v3
    • Rank:
      37709

      Description

      The fix for MDL-28948 (changing the rc4encrypt key for moodle cookies) is causing the prepopulated username field in the login form to display garbage when a moodle cookie exists from a previous moodle version. So this will occur for the first visit to the site on any browser with the cookie saved after the last moodle upgrade.

      To prevent the previous cookie being misread (as the encryption key will have now changed), I suggest appending the cookie name with "_V2" (hat tip to Matt Clarkson for the suggestion).

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          For an existing install, the change should not be introducing a new key.

          I'll refer this back to Adrian to see what is going on.

          Show
          Michael de Raadt added a comment - For an existing install, the change should not be introducing a new key. I'll refer this back to Adrian to see what is going on.
          Hide
          Simon Coggins added a comment -

          The workaround posted above will override the site's existing siteidentifier. This might cause some issues with backups, since the siteidentifier is used in backup_is_same_site() to identify if the backup was made on the current site.

          I think a better workaround would be to add some text to the "Cookie prefix" field in Site Admin > Server > Session Handling.

          Simon

          Show
          Simon Coggins added a comment - The workaround posted above will override the site's existing siteidentifier. This might cause some issues with backups, since the siteidentifier is used in backup_is_same_site() to identify if the backup was made on the current site. I think a better workaround would be to add some text to the "Cookie prefix" field in Site Admin > Server > Session Handling. Simon
          Hide
          Adrian Greeve added a comment -

          I'm not really happy with the patch that I've provided. I think that there is a more elegant solution available.

          What I have done is created a settings switch which allows the site administrator to switch between the old password key and the random generated key. If the site is being installed then a line will be inserted into the config.php file which turns on the switch effectively denying the site administrator from using the old password key.

          There is a small addition to the session cookie value which will force a renewal of cookies and should avoid the garbage text that is generated when changing between keys.

          I'd love some suggestions about other ways that I might tackle this problem.

          Thanks!

          Show
          Adrian Greeve added a comment - I'm not really happy with the patch that I've provided. I think that there is a more elegant solution available. What I have done is created a settings switch which allows the site administrator to switch between the old password key and the random generated key. If the site is being installed then a line will be inserted into the config.php file which turns on the switch effectively denying the site administrator from using the old password key. There is a small addition to the session cookie value which will force a renewal of cookies and should avoid the garbage text that is generated when changing between keys. I'd love some suggestions about other ways that I might tackle this problem. Thanks!
          Hide
          Ankit Agarwal added a comment -

          Hi Adrian,
          If we are changing cookie names than in my opinion there is no need to define the key via a config setting. Since it will wipe out the old cookies anyway.
          The patch looks spot on to me as per your approach. The only issue that I noticed in the patch is:-

          • $temp->add should be done before $ADMIN->add

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Adrian, If we are changing cookie names than in my opinion there is no need to define the key via a config setting. Since it will wipe out the old cookies anyway. The patch looks spot on to me as per your approach. The only issue that I noticed in the patch is:- $temp->add should be done before $ADMIN->add Thanks
          Hide
          Adrian Greeve added a comment -

          Thanks for the input Ankit. I moved the $temp->add section in front of the $ADMIN->add section.
          I've set a config setting switch because the rc4encryption function is used in other areas besides the session cookies.
          The current behaviour now is that if you do an install of Moodle, it will automatically use the randomly generated key. If you are upgrading (2.2.1 to 2.2.1 or 2.0 to 2.2) It will use the old hard coded key until the setting is changed in [Site administration->Security->Site policies]
          Submitting for peer review.

          Show
          Adrian Greeve added a comment - Thanks for the input Ankit. I moved the $temp->add section in front of the $ADMIN->add section. I've set a config setting switch because the rc4encryption function is used in other areas besides the session cookies. The current behaviour now is that if you do an install of Moodle, it will automatically use the randomly generated key. If you are upgrading (2.2.1 to 2.2.1 or 2.0 to 2.2) It will use the old hard coded key until the setting is changed in [Site administration->Security->Site policies] Submitting for peer review.
          Hide
          Ankit Agarwal added a comment -

          Hi Adrian,
          Patch looks good to me. one little thing:-
          IMHO the the version in the upgrade code can point to latest 2.2.1+ release instead of 2012021300.01

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Adrian, Patch looks good to me. one little thing:- IMHO the the version in the upgrade code can point to latest 2.2.1+ release instead of 2012021300.01 Thanks
          Hide
          Adrian Greeve added a comment -

          I've made a couple of changes to the help string for the setting to make it clear that changing this setting may not be a wise move. When installing this will use the new randomly generated key. If upgrading then it will use the old key. Hopefully this will keep other plugins that use the rc4encrypt function from breaking. I also made a slight change to the session cookie so that if upgrading and the key is changed it won't produce mangled text.
          Version numbers have been changed. I had a talk with Apu and he said that master and 2.2 couldn't be the same.
          Thanks everyone.

          Show
          Adrian Greeve added a comment - I've made a couple of changes to the help string for the setting to make it clear that changing this setting may not be a wise move. When installing this will use the new randomly generated key. If upgrading then it will use the old key. Hopefully this will keep other plugins that use the rc4encrypt function from breaking. I also made a slight change to the session cookie so that if upgrading and the key is changed it won't produce mangled text. Version numbers have been changed. I had a talk with Apu and he said that master and 2.2 couldn't be the same. Thanks everyone.
          Hide
          Ankit Agarwal added a comment -

          Hi Adrian,
          Looks good to me.
          Just a small thing "the username login to display strange characters" shouldn't that be something more like "the username in the login form to display strange characters"?
          Rest looks on spot.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Adrian, Looks good to me. Just a small thing "the username login to display strange characters" shouldn't that be something more like "the username in the login form to display strange characters"? Rest looks on spot. Thanks
          Hide
          Adrian Greeve added a comment -

          Thanks for that Ankit.
          I think that the string is much clearer now. Submitting for integration review.

          Show
          Adrian Greeve added a comment - Thanks for that Ankit. I think that the string is much clearer now. Submitting for integration review.
          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
          Sam Hemelryk added a comment -

          Hi guys,

          I've been having a think about this and how people have been affected by this patch.
          To be truthful I think that the introduction of a new setting is not needed, more yet perhaps introduces more problems than it is worth.
          There are presently three situations a site can be in before this patch:

          1. Fresh install: Everything is working fine.
          2. Upgraded site: Things are broken but may have been potentially fixed
          3. Not upgraded yet: Things are fine but will break when upgrade next happens

          After the cookie name is changed:

          1. Fresh install: Still works fine
          2. Upgraded sites: Fixed and those who fixed themselves work fine
          3. Not upgraded yet: Things will work fine when they upgrade

          Changing the cookie name has the ideal outcome, all sites will work and all sites will be using the marginally more secure site_identifier rather than the fixed string. We avoid having to add settings and mess with defaults.
          I feel the setting is really just adding confusion and certainly isn't necessary... really the key to that is that we are adding it to solve an upgrade problem we created

          Eloy did have one good idea he mentioned during an integration chat, we could try detecting both encodings and checking for valid UTF8 and then migrating when required.
          Something worth exploring, anyway for the time being I am sending this back for more thought/investigation.

          I'll also add Eloy here as he has obviously been thinking about it as well.

          For the time being my -1 to adding a setting to control this, and a +1 to looking at just changing the cookie name or other ways we could make it smooth such as detection.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've been having a think about this and how people have been affected by this patch. To be truthful I think that the introduction of a new setting is not needed, more yet perhaps introduces more problems than it is worth. There are presently three situations a site can be in before this patch: Fresh install: Everything is working fine. Upgraded site: Things are broken but may have been potentially fixed Not upgraded yet: Things are fine but will break when upgrade next happens After the cookie name is changed: Fresh install: Still works fine Upgraded sites: Fixed and those who fixed themselves work fine Not upgraded yet: Things will work fine when they upgrade Changing the cookie name has the ideal outcome, all sites will work and all sites will be using the marginally more secure site_identifier rather than the fixed string. We avoid having to add settings and mess with defaults. I feel the setting is really just adding confusion and certainly isn't necessary... really the key to that is that we are adding it to solve an upgrade problem we created Eloy did have one good idea he mentioned during an integration chat, we could try detecting both encodings and checking for valid UTF8 and then migrating when required. Something worth exploring, anyway for the time being I am sending this back for more thought/investigation. I'll also add Eloy here as he has obviously been thinking about it as well. For the time being my -1 to adding a setting to control this, and a +1 to looking at just changing the cookie name or other ways we could make it smooth such as detection. Cheers Sam
          Hide
          Michael de Raadt added a comment -

          Hi, all.

          I agree that an administer-able setting is not necessary for this, but I don't think we should use the site_identifier value (which I thought was a new value for this purpose, but I was wrong). If we can introduce a new setting without adding it to global settings interface, that would be good. It doesn't need to be visible, but it does need to be unique.

          Show
          Michael de Raadt added a comment - Hi, all. I agree that an administer-able setting is not necessary for this, but I don't think we should use the site_identifier value (which I thought was a new value for this purpose, but I was wrong). If we can introduce a new setting without adding it to global settings interface, that would be good. It doesn't need to be visible, but it does need to be unique.
          Hide
          Adrian Greeve added a comment -

          I've made changes as per conversations had with Eloy and Sam. I altered the rc4encrypt and rc4decrypt functions to allow for people still using authorize.net in version 1.9 and a check to see which password is being used in the cookies.

          Submitting for peer review.

          Show
          Adrian Greeve added a comment - I've made changes as per conversations had with Eloy and Sam. I altered the rc4encrypt and rc4decrypt functions to allow for people still using authorize.net in version 1.9 and a check to see which password is being used in the cookies. Submitting for peer review.
          Hide
          Ankit Agarwal added a comment -

          Looks Good!
          Thanks

          Show
          Ankit Agarwal added a comment - Looks Good! Thanks
          Hide
          Adrian Greeve added a comment -

          Thank you for peer reviewing this again Ankit. Much appreciated.

          Show
          Adrian Greeve added a comment - Thank you for peer reviewing this again Ankit. Much appreciated.
          Hide
          Adrian Greeve added a comment -

          I made a change to the code of 1.9. 1.9 Doesn't have PARAM_USERNAME. Instead I've done a search on the user table. I'd be happy to hear of other suggestions as to how to get around making a query of the database.

          Show
          Adrian Greeve added a comment - I made a change to the code of 1.9. 1.9 Doesn't have PARAM_USERNAME. Instead I've done a search on the user table. I'd be happy to hear of other suggestions as to how to get around making a query of the database.
          Hide
          Ankit Agarwal added a comment -

          Well you can do a regex match instead of DB check, I would suggest you get a second opinion from Sam or Eloy on this.
          Thanks

          Show
          Ankit Agarwal added a comment - Well you can do a regex match instead of DB check, I would suggest you get a second opinion from Sam or Eloy on this. Thanks
          Hide
          Adrian Greeve added a comment -

          Regular expression seems to be the popular consensus. I've created a new patch using that.

          Show
          Adrian Greeve added a comment - Regular expression seems to be the popular consensus. I've created a new patch using that.
          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
          Sam Hemelryk added a comment -

          Making Eloy the integrator, probably best he looks at this.

          Show
          Sam Hemelryk added a comment - Making Eloy the integrator, probably best he looks at this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Awaiting some minor tweaks to arrive before pushing this...

          Show
          Eloy Lafuente (stronk7) added a comment - Awaiting some minor tweaks to arrive before pushing this...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Summary of chat with Adrian, some little things to fix:

          1) In 1.9, we need to moodle_strtolower() before the regexp because can be a lot of cookies out there having uppers (and we don't modify them, just perform the regexp againt the lowercase username)

          2) In master, the calls to rc4decrypt() require fixing, so the first is true and the second is false (now both are true)

          3) Missing returns in get_moodle_cookie() when old-password is tried

          4) Missing guest/nobody checks when old-password is tried

          Commenting with SamH about it, Adrian will ping him once this is ready for review if I'm not online.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Summary of chat with Adrian, some little things to fix: 1) In 1.9, we need to moodle_strtolower() before the regexp because can be a lot of cookies out there having uppers (and we don't modify them, just perform the regexp againt the lowercase username) 2) In master, the calls to rc4decrypt() require fixing, so the first is true and the second is false (now both are true) 3) Missing returns in get_moodle_cookie() when old-password is tried 4) Missing guest/nobody checks when old-password is tried Commenting with SamH about it, Adrian will ping him once this is ready for review if I'm not online. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Adrian, all perfect but the point 4) above.

          As far as you've changed the inequalities to equalities, the "or" within them should be changed to "and". In any case, for better readability.... what if you also take these lines:

          if ($username === 'guest' or $username === 'nobody') {
              // backwards compatibility - we do not set these cookies any more
              $username = '';
          }
          

          and put them exactly before the final "return $username". IMO with them there, you will be ensure that "guest" or "nobody" won't be ever being returned, no matter of the encrypt used. So you can safely delete any other use of those conditions in the method (yes the cookie will be set with those values sometimes - when converting to new key, but who cares, it won't be ever returned).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Adrian, all perfect but the point 4) above. As far as you've changed the inequalities to equalities, the "or" within them should be changed to "and". In any case, for better readability.... what if you also take these lines: if ($username === 'guest' or $username === 'nobody') { // backwards compatibility - we do not set these cookies any more $username = ''; } and put them exactly before the final "return $username". IMO with them there, you will be ensure that "guest" or "nobody" won't be ever being returned, no matter of the encrypt used. So you can safely delete any other use of those conditions in the method (yes the cookie will be set with those values sometimes - when converting to new key, but who cares, it won't be ever returned). Ciao
          Hide
          Adrian Greeve added a comment -

          I've moved the quest / nobody check to just before the final return, and changed the if statements a bit. It just needs a final inspection and it should be ready to go.
          Thanks Eloy.

          Show
          Adrian Greeve added a comment - I've moved the quest / nobody check to just before the final return, and changed the if statements a bit. It just needs a final inspection and it should be ready to go. Thanks Eloy.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been integrated, thanks! (19, 20, 21, 22 & master). Let's see how testing goes.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been integrated, thanks! (19, 20, 21, 22 & master). Let's see how testing goes. Ciao
          Hide
          Rajesh Taneja added a comment -

          Sorry Adrain, it still fails for me. It do clean garbage but still showing username which I never entered. Only tested on 19, so didn't proceed further.
          Steps to reproduce:

          1. Clear browser cookie/data (I used Chrome, use any browser which u normally don't use.)
          2. I have been using a test site before, (with garbage username in cookie), so I deleted the site and installed STABLE_19. (Leaving garbage username cookie in FF)
          3. All worked fine on chrome as no cookies were present and I could see garbage username on FF
          4. Updated STABLE_19 with INTEGRATION_19 and all went fine on chrome, but on FF I could see username which I never entered. (It cleaned garbage, but left charaters and numbers)

          Summary:
          If my site is broken and user have already got a garbage in the cookie, then some random username turns up without garbage chars (garbage chars are stripped leaving chars and numbers)

          I am not sure, but I feel the best and easy way to solve this issue is to change $cookiename prefix in get_moodle_cookie and set_moodle_cookie. If you feel it can break stable branch then we should introduce a duplicate cookie and update both in stable branches.

          Show
          Rajesh Taneja added a comment - Sorry Adrain, it still fails for me. It do clean garbage but still showing username which I never entered. Only tested on 19, so didn't proceed further. Steps to reproduce: Clear browser cookie/data (I used Chrome, use any browser which u normally don't use.) I have been using a test site before, (with garbage username in cookie), so I deleted the site and installed STABLE_19. (Leaving garbage username cookie in FF) All worked fine on chrome as no cookies were present and I could see garbage username on FF Updated STABLE_19 with INTEGRATION_19 and all went fine on chrome, but on FF I could see username which I never entered. (It cleaned garbage, but left charaters and numbers) Summary: If my site is broken and user have already got a garbage in the cookie, then some random username turns up without garbage chars (garbage chars are stripped leaving chars and numbers) I am not sure, but I feel the best and easy way to solve this issue is to change $cookiename prefix in get_moodle_cookie and set_moodle_cookie. If you feel it can break stable branch then we should introduce a duplicate cookie and update both in stable branches.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Yeah, false-positive usernames can be returned (decoded), I just hoped they did not happen usually, as you initially get.

          In fact, if your cookie already had garbage, I cannot understand why it was converted to a non-garbaged alternative... grrr, perhaps the moodle_strtolower() is the culprit, leading to "cleaned" string without notice.

          I'd suggest to change, in 19_STABLE:

          $username = moodle_strtolower($username);
          $userdata = preg_replace('/[^-\.@_a-z0-9]/', '', $username);
          

          by

          $userdata = moodle_strtolower($username);
          $userdata = preg_replace('/[^-\.@_a-z0-9]/', '', $userdata);
          

          so username is NOT modified silently by moodle_strtolower(). I think that will lead to you getting one empty username in the 4) point of your steps instead of the false-positive silently introduced by moodle_strtolower().

          Ciao

          PS: I'm adding one commit to 19_STABLE with that change. If finally it does not work... we'll have to revert this, although previous situation is clearly worse, with all existing cookies breaking badly.

          PS2: Note that 19_STABLE is the only branch affected by the moodle_strtolower() silently behavior. All 2.x branches should go ok without any modification.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Yeah, false-positive usernames can be returned (decoded), I just hoped they did not happen usually, as you initially get. In fact, if your cookie already had garbage, I cannot understand why it was converted to a non-garbaged alternative... grrr, perhaps the moodle_strtolower() is the culprit, leading to "cleaned" string without notice. I'd suggest to change, in 19_STABLE: $username = moodle_strtolower($username); $userdata = preg_replace('/[^-\.@_a-z0-9]/', '', $username); by $userdata = moodle_strtolower($username); $userdata = preg_replace('/[^-\.@_a-z0-9]/', '', $userdata); so username is NOT modified silently by moodle_strtolower(). I think that will lead to you getting one empty username in the 4) point of your steps instead of the false-positive silently introduced by moodle_strtolower(). Ciao PS: I'm adding one commit to 19_STABLE with that change. If finally it does not work... we'll have to revert this, although previous situation is clearly worse, with all existing cookies breaking badly. PS2: Note that 19_STABLE is the only branch affected by the moodle_strtolower() silently behavior. All 2.x branches should go ok without any modification.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Commit sent to integration.git. Now I'm doing (all under 19_STABLE branch):

          1) Clean all the cookies in my browser

          2) Go to a old point in the branch history: git reset --hard v1.9.14
          3) Go to the login page. Username is empty (because of 1).
          3) Access to the site, and immediately logout.
          4) Go to the login page. The username is display properly.

          5) Go to last-week point in branch history: git reset --hard 2c39e251b420383518f769d9de865e1c619d2fb6
          6) Go to the login page. Garbled username is shown. Correct. That's $this bug.
          7) Don't try to login!

          8) Go to current code @ integration.git: git reset --hard c67bc65cbec7e9f0f3d3502f5383fd9193251d0e

          9) Go to the login page. You will get one of this: Or the original username accessed in 3), or a blank username.

          But not one "invented, never used" username. Note that there are possible coincidences where those "invented, never used" usernames can happen, but then you would be seeing them also in 6) No way one garbled (non-asccii chars) username in 6) can lead to one "invented" username in 9). As said it only can lead to the original or to blank.

          So these steps demo that, for a given username (and cookie) in old 1.9, the change performed some weeks ago leaded to garbled usernames. And people that still have not logged (aka, having old cookies), will get the correct usernames back (or blank) once they update to current integration.git. Of course, people that have already logged is using the new cookies so they won't get any problem updating to current integration.git.

          Still, as said, there are chances to get some "invented, never used" valid usernames, if decoding with the new password lead, by coincidence, to one valid username. But chances of that are really way below 10%, because we only accept like 30-40 chars (lowercase ascii, numbers...) and the decode returns 0-255 bytes. So the chance of getting all bytes being "valid" ones is really small.

          Of course, the 1-10 steps above, should be also performed in 20_STABLE, 21_STABLE, 22_STABLE and master, by picking correct commits to run them.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Commit sent to integration.git. Now I'm doing (all under 19_STABLE branch): 1) Clean all the cookies in my browser 2) Go to a old point in the branch history: git reset --hard v1.9.14 3) Go to the login page. Username is empty (because of 1). 3) Access to the site, and immediately logout. 4) Go to the login page. The username is display properly. 5) Go to last-week point in branch history: git reset --hard 2c39e251b420383518f769d9de865e1c619d2fb6 6) Go to the login page. Garbled username is shown. Correct. That's $this bug. 7) Don't try to login! 8) Go to current code @ integration.git: git reset --hard c67bc65cbec7e9f0f3d3502f5383fd9193251d0e 9) Go to the login page. You will get one of this: Or the original username accessed in 3), or a blank username. But not one "invented, never used" username. Note that there are possible coincidences where those "invented, never used" usernames can happen, but then you would be seeing them also in 6) No way one garbled (non-asccii chars) username in 6) can lead to one "invented" username in 9). As said it only can lead to the original or to blank. So these steps demo that, for a given username (and cookie) in old 1.9, the change performed some weeks ago leaded to garbled usernames. And people that still have not logged (aka, having old cookies), will get the correct usernames back (or blank) once they update to current integration.git. Of course, people that have already logged is using the new cookies so they won't get any problem updating to current integration.git. Still, as said, there are chances to get some "invented, never used" valid usernames, if decoding with the new password lead, by coincidence, to one valid username. But chances of that are really way below 10%, because we only accept like 30-40 chars (lowercase ascii, numbers...) and the decode returns 0-255 bytes. So the chance of getting all bytes being "valid" ones is really small. Of course, the 1-10 steps above, should be also performed in 20_STABLE, 21_STABLE, 22_STABLE and master, by picking correct commits to run them. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Gah, one more thing. I also have found one incorrect condition in the 19_STABLE that was causing the match to happen always! Adding one more commit!

          With that fixed and the (silent cleanup of) moodle_strtolower() not leading to an excess of valid usernames, this should be ok now. I'm doing a big test to try to infere the % where "invented" valid usernames can happen agains one big dictionary.

          Note: 2.x branches are not affected by all these changes at all. They use clean_param() and correct condition.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Gah, one more thing. I also have found one incorrect condition in the 19_STABLE that was causing the match to happen always! Adding one more commit! With that fixed and the (silent cleanup of) moodle_strtolower() not leading to an excess of valid usernames, this should be ok now. I'm doing a big test to try to infere the % where "invented" valid usernames can happen agains one big dictionary. Note: 2.x branches are not affected by all these changes at all. They use clean_param() and correct condition. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oki, I've executed this little script against 19_STABLE:

          <?php
          
              require_once ('config.php');
          
              $errors = 0;
              $processed = 0;
          
              $fh = fopen('all.txt', 'r');
          
              if ($fh) {
                  while (($line = fgets($fh, 4096)) !== false) {
                      if (substr($line, 0, 1) === '#') {
                          continue;
                      }
                      if (leads_to_invented(trim($line))) {
                          $errors++;
                      }
                      $processed++;
                      if ($processed % 5000 == 0) {
                          echo 'Progress: ' . $processed . ', correct "invented": ' . $errors . PHP_EOL;
                      }
                  }
                  fclose($fh);
              }
          
              echo 'Processed: ' . $processed . ', correct "invented": ' . $errors . PHP_EOL;
          
              /**
               * Tell us if a given word leads to invented username or no
               */
              function leads_to_invented($word) {
                  global $CFG;
          
                  // Encrypt (old pass) one lower word from the dictionary
                  $oldencrypted = rc4encrypt(moodle_strtolower($word), false);
                  // Decrypt (new pass) the word
                  $username = rc4decrypt($oldencrypted, true);
          
                  // Clean the decrypted username (as Moodle 1.9 does, 2.x clean more)
                  $userdata = trim(moodle_strtolower($username));
                  if (empty($CFG->extendedusernamechars)) {
                      $userdata = preg_replace('/[^-\.@_a-z0-9]/', '', $userdata);
                  }
          
                  if ($username == $userdata) {
                      // BAD: We have obtained one correct "invented" username
                      echo 'Hell, this leads to correct "invented" username: ' . $word . ' => ' . $username . PHP_EOL;
                      return true;
                  }
                  // GOOD: All right, the decoded username was garbled and we detected it
                  return false;
              }
          

          Using one big dictionary (near 4 million words) against it:

          • With $CFG->extendedusernamechars disabled => 312 "invented" usernames => 0.0078% false positives => 99.9922% OK.
          • With $CFG->extendedusernamechars enabled => 42830 "invented" usernames => 1.0708% false positives => 98.9292% OK.

          (note, results extrapolated after processing 2.5 millions of usernames, no time to complete the 4 millions).

          So, in the worst case ($CFG->extendedusernamechars), 1% of the people will get one incorrect (invented) username the first time they try to login. Once they set their correct username manually once, it won't be ever a problem.

          In the other side, if now we change to the 2-cookies alternative, all the people that has upgraded (and logged in) to Moodle already (since January 31th)... will have new passwords into the "old" cookies and they will have to, manually introduce their name the first time the access, because validation won't pass and will lead to empty (or invented). I'm not audacious enough to bet if that is more than the 1% happening in the worst case above or no.

          While originally the 2-cookies alternative sounded better... the more time we spend without fixing this... the worse that alternative becomes. With the "valid username" (worst) alternative we'll get, always, one 1% of people having to reintroduce their usernames and near perfection (0.0078%) if they don't have $CFG->extendedusernamechars enabled.

          And that's all I can say. Any comment will be welcome aiming to decide this ASAP.

          Ciao

          PS: Note that I've one extra commit here, that has not been pushed to integration yet, about to avoid performing the grep if $CFG->extendedusernamechars is enabled (our worst 1% scenario). If finally we go the "valid username" route, I'll add it to 19_STABLE (2.x don't need it as far as PARAM_USENAME already controls it).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oki, I've executed this little script against 19_STABLE: <?php require_once ('config.php'); $errors = 0; $processed = 0; $fh = fopen('all.txt', 'r'); if ($fh) { while (($line = fgets($fh, 4096)) !== false ) { if (substr($line, 0, 1) === '#') { continue ; } if (leads_to_invented(trim($line))) { $errors++; } $processed++; if ($processed % 5000 == 0) { echo 'Progress: ' . $processed . ', correct "invented" : ' . $errors . PHP_EOL; } } fclose($fh); } echo 'Processed: ' . $processed . ', correct "invented" : ' . $errors . PHP_EOL; /** * Tell us if a given word leads to invented username or no */ function leads_to_invented($word) { global $CFG; // Encrypt (old pass) one lower word from the dictionary $oldencrypted = rc4encrypt(moodle_strtolower($word), false ); // Decrypt ( new pass) the word $username = rc4decrypt($oldencrypted, true ); // Clean the decrypted username (as Moodle 1.9 does, 2.x clean more) $userdata = trim(moodle_strtolower($username)); if (empty($CFG->extendedusernamechars)) { $userdata = preg_replace('/[^-\.@_a-z0-9]/', '', $userdata); } if ($username == $userdata) { // BAD: We have obtained one correct "invented" username echo 'Hell, this leads to correct "invented" username: ' . $word . ' => ' . $username . PHP_EOL; return true ; } // GOOD: All right, the decoded username was garbled and we detected it return false ; } Using one big dictionary (near 4 million words) against it: Dictionary: http://ftp.sunet.se/pub/security/tools/net/Openwall/wordlists/all.gz With $CFG->extendedusernamechars disabled => 312 "invented" usernames => 0.0078% false positives => 99.9922% OK. With $CFG->extendedusernamechars enabled => 42830 "invented" usernames => 1.0708% false positives => 98.9292% OK. (note, results extrapolated after processing 2.5 millions of usernames, no time to complete the 4 millions). So, in the worst case ($CFG->extendedusernamechars), 1% of the people will get one incorrect (invented) username the first time they try to login. Once they set their correct username manually once, it won't be ever a problem. In the other side, if now we change to the 2-cookies alternative, all the people that has upgraded (and logged in) to Moodle already (since January 31th)... will have new passwords into the "old" cookies and they will have to, manually introduce their name the first time the access, because validation won't pass and will lead to empty (or invented). I'm not audacious enough to bet if that is more than the 1% happening in the worst case above or no. While originally the 2-cookies alternative sounded better... the more time we spend without fixing this... the worse that alternative becomes. With the "valid username" (worst) alternative we'll get, always, one 1% of people having to reintroduce their usernames and near perfection (0.0078%) if they don't have $CFG->extendedusernamechars enabled. And that's all I can say. Any comment will be welcome aiming to decide this ASAP. Ciao PS: Note that I've one extra commit here, that has not been pushed to integration yet, about to avoid performing the grep if $CFG->extendedusernamechars is enabled (our worst 1% scenario). If finally we go the "valid username" route, I'll add it to 19_STABLE (2.x don't need it as far as PARAM_USENAME already controls it).
          Hide
          Simon Coggins added a comment -

          Hi Eloy,

          We fixed this for Totara (1.9) shortly after reporting it here by changing the cookiename prefix from 'MOODLEID' to 'MOODLEID1' in set_moodle_cookie and get_moodle_cookie.

          Although this meant users had to re-enter their username the first time they visited after an upgrade it keeps the code simple and we haven't had any further issues reported since.

          Simon

          Show
          Simon Coggins added a comment - Hi Eloy, We fixed this for Totara (1.9) shortly after reporting it here by changing the cookiename prefix from 'MOODLEID' to 'MOODLEID1' in set_moodle_cookie and get_moodle_cookie. Although this meant users had to re-enter their username the first time they visited after an upgrade it keeps the code simple and we haven't had any further issues reported since. Simon
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yup, that's one, 3rd alternative.

          Switch to different cookie and done. Everybody will have to re-introduce his username once and done.

          Let's see. ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yup, that's one, 3rd alternative. Switch to different cookie and done. Everybody will have to re-introduce his username once and done. Let's see. ciao
          Hide
          Adrian Greeve added a comment -

          Wow! It looks like you've been extremely busy. Thanks Eloy for all of your hard work. I can't thank you enough.

          Show
          Adrian Greeve added a comment - Wow! It looks like you've been extremely busy. Thanks Eloy for all of your hard work. I can't thank you enough.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Was fun to crunch millions of usernames, yup. My computer wasn't so happy, hehe.

          Show
          Eloy Lafuente (stronk7) added a comment - Was fun to crunch millions of usernames, yup. My computer wasn't so happy, hehe.
          Hide
          Aparup Banerjee added a comment -

          for the record and for summary so far (from dev chat):

          (09:35:05) Cindereloy: 3 alternatives:

          • 2 cookie alternative. Will work ok for all but for those that have upgraded since January.
          • current code @ integration.git => 99% succes based in my tests. 1% can see "fake" usernames the 1st time they go to login.
          • create one 100% new cookie, and force everybody to re-introduce the username the first time they go to login. Warn @ docs.
          Show
          Aparup Banerjee added a comment - for the record and for summary so far (from dev chat): (09:35:05) Cindereloy: 3 alternatives: 2 cookie alternative. Will work ok for all but for those that have upgraded since January. current code @ integration.git => 99% succes based in my tests. 1% can see "fake" usernames the 1st time they go to login. create one 100% new cookie, and force everybody to re-introduce the username the first time they go to login. Warn @ docs.
          Hide
          Martin Dougiamas added a comment -

          +1 for switching to a new cookie name like MOODLELOGIN (and deleting the old MOODLEID cookie).

          Since we've changed the way it's calculated for a security reason, then I think it's justifiable. Also, I guess a lot of people are using browser form memory to fill in the field anyway and so won't be affected.

          Show
          Martin Dougiamas added a comment - +1 for switching to a new cookie name like MOODLELOGIN (and deleting the old MOODLEID cookie). Since we've changed the way it's calculated for a security reason, then I think it's justifiable. Also, I guess a lot of people are using browser form memory to fill in the field anyway and so won't be affected.
          Hide
          Adrian Greeve added a comment -

          I just updated the patch. This one changes the cookie prefix to MOODLEID1. Next step: testing.

          Show
          Adrian Greeve added a comment - I just updated the patch. This one changes the cookie prefix to MOODLEID1. Next step: testing.
          Hide
          Adrian Greeve added a comment -

          Testing instructions updated. Rajesh and I have been testing the patch. I haven't had any problems.

          Show
          Adrian Greeve added a comment - Testing instructions updated. Rajesh and I have been testing the patch. I haven't had any problems.
          Hide
          Rajesh Taneja added a comment -

          All works well, Thanks for all the hard work Adrian

          Findings:
          I don't see any repercussion of losing my saved password/username for the site, with change in cookie name.
          The only difference is it comes up blank, but then it gets populated with mouse click or keystroke.

          Show
          Rajesh Taneja added a comment - All works well, Thanks for all the hard work Adrian Findings: I don't see any repercussion of losing my saved password/username for the site, with change in cookie name. The only difference is it comes up blank, but then it gets populated with mouse click or keystroke.
          Hide
          Adrian Greeve added a comment -

          Oh one thing. Sam suggested that the rc4encrpytion function in master should have a default of true for using the randomly generated password key instead of false. This could cause problems with upgrading from 1.9 to 2.3 if the old site has something like authorize.net installed.

          Show
          Adrian Greeve added a comment - Oh one thing. Sam suggested that the rc4encrpytion function in master should have a default of true for using the randomly generated password key instead of false. This could cause problems with upgrading from 1.9 to 2.3 if the old site has something like authorize.net installed.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Reverted old commits and re-integrated, thanks!

          Side note: Perhaps we should warn about the adopted behavior so EVERYBODY (not having username stored in browser) will need to introduce it once after the updates? That can be achieved both in release notes and in security advisories.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Reverted old commits and re-integrated, thanks! Side note: Perhaps we should warn about the adopted behavior so EVERYBODY (not having username stored in browser) will need to introduce it once after the updates? That can be achieved both in release notes and in security advisories.
          Hide
          Rajesh Taneja added a comment -

          All well,

          Thanks everyone.

          @Adrian: AFAIK 1.9 to 2.3 is not possible. user has to go from 1.9 to 2.2 then 2.2 to 2.3 and yes, this should be included in 2.3 release notes.

          Show
          Rajesh Taneja added a comment - All well, Thanks everyone. @Adrian: AFAIK 1.9 to 2.3 is not possible. user has to go from 1.9 to 2.2 then 2.2 to 2.3 and yes, this should be included in 2.3 release notes.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

          icao_reverse('arreis olik rebemevon afla letoh ognat');
          

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao
          Hide
          Michael de Raadt added a comment -

          I've noted the effect of this in the release notes.

          Show
          Michael de Raadt added a comment - I've noted the effect of this in the release notes.
          Hide
          Helen Foster added a comment -

          I notice this issue is mentioned in http://docs.moodle.org/dev/Moodle_2.2.2_release_notes Does it also need to be mentioned in http://docs.moodle.org/dev/Moodle_2.3_release_notes or in the user docs?

          Show
          Helen Foster added a comment - I notice this issue is mentioned in http://docs.moodle.org/dev/Moodle_2.2.2_release_notes Does it also need to be mentioned in http://docs.moodle.org/dev/Moodle_2.3_release_notes or in the user docs?
          Hide
          Adrian Greeve added a comment -

          Hello Helen,

          Garbled text was only a problem for people that upgraded early from a previous patch. This patch doesn't have those issues, and people upgrading from any version will no longer encounter that problem. So there is no need to mention it in the release notes or the user docs.

          Show
          Adrian Greeve added a comment - Hello Helen, Garbled text was only a problem for people that upgraded early from a previous patch. This patch doesn't have those issues, and people upgrading from any version will no longer encounter that problem. So there is no need to mention it in the release notes or the user docs.
          Hide
          Mary Cooch added a comment -

          (Housekeeping) Removing docs_required label as apparently it doesn't need to be mentioned in user docs.

          Show
          Mary Cooch added a comment - (Housekeeping) Removing docs_required label as apparently it doesn't need to be mentioned in user docs.

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: