Moodle
  1. Moodle
  2. MDL-16872

fatal error when trying to clone $_SESSION

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.1
    • Fix Version/s: 1.9.7, 2.0
    • Component/s: MNet
    • Labels:
      None
    • Environment:
      php 5.2.6, Apache 2.0, WinXP Pro
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      30341

      Description

      Occurs when deleting a peer, and after the related bug on table mdl_mnet_rpc2host has been fixed.

      Fatal error: __clone method called on non-object in D:\wwwroot\WWW-PRF_VMOODLE-PHP\auth\mnet\auth.php on line 1230
      (this is a 1.9.1) ref.

      occurs when cloning forth and back $_SESSION for cache :

      function end_local_sessions(&$sessionArray) {
      global $CFG;
      if (is_array($sessionArray)) {
      $start = ob_start();

      $uc = ini_get('session.use_cookies');
      ini_set('session.use_cookies', false);
      print_object($_SESSION);
      $sesscache = clone($_SESSION); // THIS CLONE SEEMS BEING ILLEGAL $_SESSION is not a "created object"
      $sessidcache = session_id();
      session_write_close();
      unset($_SESSION);

      The subsequent back cloning provoques an error also.

      Fix : just don't try to clone the global hash here (just simply affect it). I don't know the exact consequences but seems work normally.

        Issue Links

          Activity

          Hide
          Valery Fremaux added a comment -

          MDL-16871 hides this bug.

          Show
          Valery Fremaux added a comment - MDL-16871 hides this bug.
          Hide
          Hubert Chathi added a comment -

          auth/mnet/auth.php tries to clone $_SESSION several times, which results in the same error, and also which breaks keepalive_server, kill_children, and kill_child. A symptom of this is that the remote Moodle calling that function will complain that: "1:Payload not encrypted".

          Show
          Hubert Chathi added a comment - auth/mnet/auth.php tries to clone $_SESSION several times, which results in the same error, and also which breaks keepalive_server, kill_children, and kill_child. A symptom of this is that the remote Moodle calling that function will complain that: "1:Payload not encrypted".
          Hide
          Petr Škoda added a comment -

          not really sure if cloning is needed here, but in any case this needs a detailed evaluation, please do not commit something that just "seems" to solve this, this could lead to critical security problems...

          Show
          Petr Škoda added a comment - not really sure if cloning is needed here, but in any case this needs a detailed evaluation, please do not commit something that just "seems" to solve this, this could lead to critical security problems...
          Hide
          Hubert Chathi added a comment -

          The problem is that the code tries to do a clone on an array, instead of just doing an assignment, which will copy the array. So I believe the correct solution is to replace $sesscache=clone($_SESSION) with $sesscache=$_SESSION (and similarly for the back-cloning). I believe that all the calls to clone in auth/mnet/auth.php are related to the $_SESSION variable.
          I'm not sure what security problems are possible here – the copying of $_SESSION is just to back up the current session, since the code needs to manipulate other sessions (e.g. to log users out, or to refresh a user's session to prevent a timeout). So the code just backs up $_SESSION, frobs some other session(s), and then restores the original $_SESSION.

          Show
          Hubert Chathi added a comment - The problem is that the code tries to do a clone on an array, instead of just doing an assignment, which will copy the array. So I believe the correct solution is to replace $sesscache=clone($_SESSION) with $sesscache=$_SESSION (and similarly for the back-cloning). I believe that all the calls to clone in auth/mnet/auth.php are related to the $_SESSION variable. I'm not sure what security problems are possible here – the copying of $_SESSION is just to back up the current session, since the code needs to manipulate other sessions (e.g. to log users out, or to refresh a user's session to prevent a timeout). So the code just backs up $_SESSION, frobs some other session(s), and then restores the original $_SESSION.
          Hide
          Nigel McNie added a comment -

          Attached is a patch that fixes this issue. I have tested this and it works with latest Moodle 1.9 (while signing out from Mahara 1.2).

          I don't see any security issue. The current code seems to work when being hosted on PHP4 (and thus the moodle clone() replacement is in effect). On PHP5, the code just crashes horribly, as you can't use the builtin clone to duplicate arrays. For arrays, duplication happens by default with = of course - see this script:

          <?php

          echo phpversion() . "\n";

          $a = array(1, 2, 3);
          $b = $a;

          print_r($a); // 1, 2, 3
          print_r($b); // 1, 2, 3

          $b[2] = 4;
          print_r($b); // 1, 2, 4

          unset($b);
          print_r($a); // 1, 2, 3

          ?>

          Would appreciate if this could be applied to 1.9 and 2.0. Let me know if there's any potential credible security issue and I'll investigate.

          Show
          Nigel McNie added a comment - Attached is a patch that fixes this issue. I have tested this and it works with latest Moodle 1.9 (while signing out from Mahara 1.2). I don't see any security issue. The current code seems to work when being hosted on PHP4 (and thus the moodle clone() replacement is in effect). On PHP5, the code just crashes horribly, as you can't use the builtin clone to duplicate arrays. For arrays, duplication happens by default with = of course - see this script: <?php echo phpversion() . "\n"; $a = array(1, 2, 3); $b = $a; print_r($a); // 1, 2, 3 print_r($b); // 1, 2, 3 $b [2] = 4; print_r($b); // 1, 2, 4 unset($b); print_r($a); // 1, 2, 3 ?> Would appreciate if this could be applied to 1.9 and 2.0. Let me know if there's any potential credible security issue and I'll investigate.
          Hide
          Petr Škoda added a comment -

          +1 for commit of the patch into 19 and HEAD, thanks Nigel

          Show
          Petr Škoda added a comment - +1 for commit of the patch into 19 and HEAD, thanks Nigel
          Hide
          Dan Poltawski added a comment -

          I will apply this after the tuesday code review

          Show
          Dan Poltawski added a comment - I will apply this after the tuesday code review
          Hide
          Petr Škoda added a comment -

          thanks

          Show
          Petr Škoda added a comment - thanks
          Hide
          Nigel McNie added a comment -

          After having read Hubert's comment again, I checked and saw that there were several places in auth/mnet/auth.php where clone() was being called on $_SESSION. I have attached another patch that fixes these, although I have not actually tested the code paths that would cause them to be run.

          They occur in:

          keepalive_server: This is meant to be called by a remote host with a list of usernames who are still roaming on it, so that their sessions can be kept alive. I do not know if this function is ever called in practice - I know that Mahara does not bother to call it (it would make sense to call it from cron, and Mahara has no such cron job). Maybe Moodle <-> Moodle SSO does call it though.

          kill_child: This is the counterpart to the kill_children method that was fixed by my first patch. This kills the session on this host, and is called by kill_children on some remote host, so you'd probably see this being called in Moodle <-> Moodle SSO, or the very uncommon case where Mahara was the IDP. For the record, Mahara doesn't implement kill_child.

          end_local_sessions: According to the PHPDoc, this is called before a host is deleted.

          I suspect that you can apply this patch without harm as it just looks like the previous developer (Donal?) made the same mistake in all places. But then I haven't tested it, so fully understand if it's too risky to be applied.

          Show
          Nigel McNie added a comment - After having read Hubert's comment again, I checked and saw that there were several places in auth/mnet/auth.php where clone() was being called on $_SESSION. I have attached another patch that fixes these, although I have not actually tested the code paths that would cause them to be run. They occur in: keepalive_server: This is meant to be called by a remote host with a list of usernames who are still roaming on it, so that their sessions can be kept alive. I do not know if this function is ever called in practice - I know that Mahara does not bother to call it (it would make sense to call it from cron, and Mahara has no such cron job). Maybe Moodle <-> Moodle SSO does call it though. kill_child: This is the counterpart to the kill_children method that was fixed by my first patch. This kills the session on this host, and is called by kill_children on some remote host, so you'd probably see this being called in Moodle <-> Moodle SSO, or the very uncommon case where Mahara was the IDP. For the record, Mahara doesn't implement kill_child. end_local_sessions: According to the PHPDoc, this is called before a host is deleted. I suspect that you can apply this patch without harm as it just looks like the previous developer (Donal?) made the same mistake in all places. But then I haven't tested it, so fully understand if it's too risky to be applied.
          Hide
          Dan Poltawski added a comment -

          I've fixed this in CVS

          Show
          Dan Poltawski added a comment - I've fixed this in CVS
          Hide
          Andrew Davis added a comment -

          $_SESSION is an array primitive and not an object so trying to clone it causes an error. Simply assigning it results in a copy being made. Closing.

          Show
          Andrew Davis added a comment - $_SESSION is an array primitive and not an object so trying to clone it causes an error. Simply assigning it results in a copy being made. Closing.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: