Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 1.9.8, 2.0
    • Component/s: MNet
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      32857

      Activity

      Hide
      Penny Leach added a comment -

      Petr can you summarise here? Feel free to add subtasks or whatever

      Show
      Penny Leach added a comment - Petr can you summarise here? Feel free to add subtasks or whatever
      Hide
      Petr Škoda added a comment -

      1/ we need to verify the mnet session hacks in 1.9 work fine with the
      $temp->add(new admin_setting_configcheckbox('regenloginsession', get_string('regenloginsession', 'admin'), get_string('configregenloginsession', 'admin'), 0));
      admin setting. It is forcing refresh of session id right after login. We need to make sure the new refreshed session id is stored the mnet_sessions table

      2/ the current mnet relies on keep alive server tricks, we have two option
      2a/ either update the 2.0dev code to set the last seen property in the new core session table
      2b/ implement networked session timeout test, the idea is to ask nicely the networked peers when you detect your own session expired. The new session code allows you to call PHP code right before your session expires and if needed the session can be refreshes (I suppose this would need a lot more work)

      Show
      Petr Škoda added a comment - 1/ we need to verify the mnet session hacks in 1.9 work fine with the $temp->add(new admin_setting_configcheckbox('regenloginsession', get_string('regenloginsession', 'admin'), get_string('configregenloginsession', 'admin'), 0)); admin setting. It is forcing refresh of session id right after login. We need to make sure the new refreshed session id is stored the mnet_sessions table 2/ the current mnet relies on keep alive server tricks, we have two option 2a/ either update the 2.0dev code to set the last seen property in the new core session table 2b/ implement networked session timeout test, the idea is to ask nicely the networked peers when you detect your own session expired. The new session code allows you to call PHP code right before your session expires and if needed the session can be refreshes (I suppose this would need a lot more work)
      Hide
      Penny Leach added a comment -

      Petr I just committed a bunch of code for this. It's reasonably well tested, and seems to work fine. Can you please review?

      I added a new hook in mnet's land.php to reset the session id after complete_user_login changes it, I'm going to look at backporting that to 1.9 and test it.

      Show
      Penny Leach added a comment - Petr I just committed a bunch of code for this. It's reasonably well tested, and seems to work fine. Can you please review? I added a new hook in mnet's land.php to reset the session id after complete_user_login changes it, I'm going to look at backporting that to 1.9 and test it.
      Hide
      Penny Leach added a comment -

      Here's the 1.9 patch. It works well for me.

      Show
      Penny Leach added a comment - Here's the 1.9 patch. It works well for me.
      Hide
      Petr Škoda added a comment -

      hmm, the patch does not seem right, the point of the session id regeneration is to prevent session fixation attempts.

      The auth/mnet/land.php is executed on the remove server, not the one where you are sending useranme/password, so in fact the land should regenerate the local session id and purge $SESSION instead.

      I went through the current 1.9. mnet code and I think that 1/ is ok because the start_jump_session() is executed right before the jump to remote server, I did not know previously how is that done.

      summary:

      • 1/ should work fine in 1.9
      • 3/ there is a potential session fixation security issue in land.php, it should regenerate the session id in both 1.9 (when option enabled) and 2.0 (always).
      Show
      Petr Škoda added a comment - hmm, the patch does not seem right, the point of the session id regeneration is to prevent session fixation attempts. The auth/mnet/land.php is executed on the remove server, not the one where you are sending useranme/password, so in fact the land should regenerate the local session id and purge $SESSION instead. I went through the current 1.9. mnet code and I think that 1/ is ok because the start_jump_session() is executed right before the jump to remote server, I did not know previously how is that done. summary: 1/ should work fine in 1.9 3/ there is a potential session fixation security issue in land.php, it should regenerate the session id in both 1.9 (when option enabled) and 2.0 (always).
      Hide
      Penny Leach added a comment -

      Hey Petr

      1.9 was not working without the patch. The issue was:

      • Land on remote server, a record is inserted into mnet_session, containing session_id
      • Complete user login function was called to log the user into remote moodle (this regenerates the session id)
      • Single sign OFF wasn't working - because the remote moodle's session didn't match anymore what their mnet_session.session_id record said

      I don't understand your point 3 above.

      Show
      Penny Leach added a comment - Hey Petr 1.9 was not working without the patch. The issue was: Land on remote server, a record is inserted into mnet_session, containing session_id Complete user login function was called to log the user into remote moodle (this regenerates the session id) Single sign OFF wasn't working - because the remote moodle's session didn't match anymore what their mnet_session.session_id record said I don't understand your point 3 above.
      Hide
      Petr Škoda added a comment -

      3/ http://en.wikipedia.org/wiki/Session_fixation - our protection is to regenerate session id right after login (new option regenloginsession in 1.9, mandatory in 2.0). The land.php was not fixed === is still vulnerable and needs to be fixed by regenerating session id in land.php.

      Show
      Petr Škoda added a comment - 3/ http://en.wikipedia.org/wiki/Session_fixation - our protection is to regenerate session id right after login (new option regenloginsession in 1.9, mandatory in 2.0). The land.php was not fixed === is still vulnerable and needs to be fixed by regenerating session id in land.php.
      Hide
      Penny Leach added a comment -

      I know what session fixation is. The session id is regenerated.

      land.php calls auth/mnet complete_user_login. The first thing complete_user_login does is regenerate the session id. Read my comment above for why this is causing problems.

      Show
      Penny Leach added a comment - I know what session fixation is. The session id is regenerated. land.php calls auth/mnet complete_user_login. The first thing complete_user_login does is regenerate the session id. Read my comment above for why this is causing problems.
      Hide
      Petr Škoda added a comment -

      sorrrryyyy!

      Show
      Petr Škoda added a comment - sorrrryyyy!
      Hide
      Penny Leach added a comment -

      heh, no problem

      ok so in summary:

      • mnet land is protected from session fixation by normal moodle code flow, nothing to do here
      • single sign off was broken because of this, but is fixed by above patch (already in 2.0, just needs committing to 1.9)

      solution:

      commit above patch to 1.9

      Regarding updates to 2.0 to work with new session code - did you review that stuff? It's mostly here: http://cvs.moodle.org/moodle/auth/mnet/auth.php?r1=1.69&r2=1.70

      Show
      Penny Leach added a comment - heh, no problem ok so in summary: mnet land is protected from session fixation by normal moodle code flow, nothing to do here single sign off was broken because of this, but is fixed by above patch (already in 2.0, just needs committing to 1.9) solution: commit above patch to 1.9 Regarding updates to 2.0 to work with new session code - did you review that stuff? It's mostly here: http://cvs.moodle.org/moodle/auth/mnet/auth.php?r1=1.69&r2=1.70
      Hide
      Petr Škoda added a comment -

      hmmm, maybe it would be better to just move the $mnet_session stuff from function confirm_mnet_session($token, $remotewwwroot) into a new method and call it after the complete_user_login()

      Show
      Petr Škoda added a comment - hmmm, maybe it would be better to just move the $mnet_session stuff from function confirm_mnet_session($token, $remotewwwroot) into a new method and call it after the complete_user_login()
      Hide
      Penny Leach added a comment - - edited

      You can't do it after complete_user_login, because the first time it happens, the user won't exist in the remote server, so auth/mnet has to create their account (using information it gets from their idp), before complete_user_login is called...

      I guess confirm_mnet_session could be split into two different methods though.. confirm_mnet_session and complete_mnet_session (or something)

      with complete_user_login called in between them.

      Show
      Penny Leach added a comment - - edited You can't do it after complete_user_login, because the first time it happens, the user won't exist in the remote server, so auth/mnet has to create their account (using information it gets from their idp), before complete_user_login is called... I guess confirm_mnet_session could be split into two different methods though.. confirm_mnet_session and complete_mnet_session (or something) with complete_user_login called in between them.
      Hide
      Petr Škoda added a comment -

      yes, reviewed it, but did not actually test it, it looks ok

      Show
      Petr Škoda added a comment - yes, reviewed it, but did not actually test it, it looks ok
      Hide
      Penny Leach added a comment -

      just committed (to head) - splitting up confirm_mnet_session.

      i still think the attached 1.9 patch is a lot less intrusive and better, even though head is different. if i hear no complaints i'll commit it to 1.9 next week.

      Show
      Penny Leach added a comment - just committed (to head) - splitting up confirm_mnet_session. i still think the attached 1.9 patch is a lot less intrusive and better, even though head is different. if i hear no complaints i'll commit it to 1.9 next week.
      Hide
      Penny Leach added a comment -

      fixed properly in 2.0, and temporarily in 1.9

      Show
      Penny Leach added a comment - fixed properly in 2.0, and temporarily in 1.9

        People

        • Votes:
          0 Vote for this issue
          Watchers:
          3 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved: