Moodle
  1. Moodle
  2. MDL-31938

Upgrade phpCAS library - fixing CVE-2012-1104 and CVE-2012-1105 and various problems

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.16, 2.0.7, 2.1, 2.2
    • Fix Version/s: 2.6
    • Component/s: Authentication
    • Database:
      Any
    • Testing Instructions:
      Hide

      Prerequisites: You need a CAS SSO server setup.

      For the purposes of this issue, the ldap connection parts have not changed, so it is probably sufficient to simply test the php to CAS SSO. (e.g. the default CAS setup of authenticating with any credentials).

      1. Enable the CAS authentication plugin and configure it to talk to your CAS server
        # Ensure that users can login correctly through CAS SSO.
      Show
      Prerequisites: You need a CAS SSO server setup. For the purposes of this issue, the ldap connection parts have not changed, so it is probably sufficient to simply test the php to CAS SSO. (e.g. the default CAS setup of authenticating with any credentials). Enable the CAS authentication plugin and configure it to talk to your CAS server # Ensure that users can login correctly through CAS SSO.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-31938-master
    • Rank:
      38589

      Description

      Two security issues were discovered in phpCAS that Moodle embeds: CVE-2012-1104 and CVE-2012-1105. See http://seclists.org/oss-sec/2012/q1/551 for more details.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Reassigning to Iñaky: I personally do not know much about cas and I doubt any other HQ developer does, could you please have a look at this if you find some time? thanks

          Tomasz: Thanks a lot for the report, if you know how to patch our libs feel free to create a patch.

          Petr

          Show
          Petr Škoda added a comment - Reassigning to Iñaky: I personally do not know much about cas and I doubt any other HQ developer does, could you please have a look at this if you find some time? thanks Tomasz: Thanks a lot for the report, if you know how to patch our libs feel free to create a patch. Petr
          Hide
          Dan Poltawski added a comment -

          I've got a working CAS test install now. Our phpcas seems really old (not compliant with E_STRICT), I think its worth a library update on master.

          Show
          Dan Poltawski added a comment - I've got a working CAS test install now. Our phpcas seems really old (not compliant with E_STRICT), I think its worth a library update on master.
          Show
          Dan Poltawski added a comment - https://bugzilla.redhat.com/show_bug.cgi?id=801343 https://bugzilla.redhat.com/show_bug.cgi?id=801347
          Hide
          Dan Poltawski added a comment -

          By the way Tomasz - I just noticed this: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=495542

          Looks liked redhat are using their stripped pear package.

          Show
          Dan Poltawski added a comment - By the way Tomasz - I just noticed this: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=495542 Looks liked redhat are using their stripped pear package.
          Hide
          Dan Poltawski added a comment - - edited

          Here is a backported patch for CVE-2012-1105.

          But CVE-2012-1104 is a huge change and I can't really tell if we are vulnerable.

          Show
          Dan Poltawski added a comment - - edited Here is a backported patch for CVE-2012-1105. But CVE-2012-1104 is a huge change and I can't really tell if we are vulnerable.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Based on some chatting about this I think that, in order to get everything updated we could try this alternative:

          1) upgrade to modern phpcas library.
          2) ask for testing (seems 1) is working ok but surely some real testing will help).

          That, instead of patching the old phpcas we are using. If works ok, I think it would be easy to backport it to all the 2.x series.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Based on some chatting about this I think that, in order to get everything updated we could try this alternative: 1) upgrade to modern phpcas library. 2) ask for testing (seems 1) is working ok but surely some real testing will help). That, instead of patching the old phpcas we are using. If works ok, I think it would be easy to backport it to all the 2.x series. Ciao
          Hide
          Iñaki Arenaza added a comment -

          Hi,

          as mentionned in MDL-36138 the thing is upgrading to the latest phpCAS library version (1.3.1 as of today) includes a non-compatible change in the proxy mode.

          I've been reading a lot about the CAS proxy mode, and the more I read about it the more I'm convinced this should not affect standar Moodle installations. But, if we upgrade to 1.3.1, we should loudly warn people about the possible breakage.

          Other than that, I already have a working patched version with phpCAS 1.3.1 (master only). You can have a look at the changes at https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-36138_fix_cas_auth

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi, as mentionned in MDL-36138 the thing is upgrading to the latest phpCAS library version (1.3.1 as of today) includes a non-compatible change in the proxy mode. I've been reading a lot about the CAS proxy mode, and the more I read about it the more I'm convinced this should not affect standar Moodle installations. But , if we upgrade to 1.3.1, we should loudly warn people about the possible breakage. Other than that, I already have a working patched version with phpCAS 1.3.1 (master only). You can have a look at the changes at https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-36138_fix_cas_auth Saludos. Iñaki.
          Hide
          Thijs Kinkhorst added a comment -

          Hi Iñaki,

          I'd gladly test drive your patch, but the link doesn't work for me and I've got trouble locating the branch. Can you point me to it?

          thanks,
          Thijs

          Show
          Thijs Kinkhorst added a comment - Hi Iñaki, I'd gladly test drive your patch, but the link doesn't work for me and I've got trouble locating the branch. Can you point me to it? thanks, Thijs
          Hide
          Iñaki Arenaza added a comment - - edited

          Hi Thijs,

          sorry about that. It seems I linked to the wrong branch comparison in github. This one should do the trick: https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-31938_fix_cas_auth

          I have rebased the patch on top of current master, and upgraded to phpCAS 1.3.2 (my initial patch used 1.3.1). I haven't tested this new version at all! (but changes from 1.3.1 to 1.3.2 shouldn't be an issue).

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - - edited Hi Thijs, sorry about that. It seems I linked to the wrong branch comparison in github. This one should do the trick: https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-31938_fix_cas_auth I have rebased the patch on top of current master, and upgraded to phpCAS 1.3.2 (my initial patch used 1.3.1). I haven't tested this new version at all! (but changes from 1.3.1 to 1.3.2 shouldn't be an issue). Saludos. Iñaki.
          Hide
          Thijs Kinkhorst added a comment -

          Thanks! I did need to fix some details which I have put in pull request https://github.com/iarenaza/moodle/pull/1. With those it works great for me!

          Indeed I do not use the proxyCAS but in fact I think that's rather a more rare usecase, so indeed should probably just be documented well (but where?).

          Show
          Thijs Kinkhorst added a comment - Thanks! I did need to fix some details which I have put in pull request https://github.com/iarenaza/moodle/pull/1 . With those it works great for me! Indeed I do not use the proxyCAS but in fact I think that's rather a more rare usecase, so indeed should probably just be documented well (but where?).
          Hide
          Iñaki Arenaza added a comment -

          Hi Thijs,

          thanks for the fixes! I merged your pull request and have pushed the changes back to github.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Thijs, thanks for the fixes! I merged your pull request and have pushed the changes back to github. Saludos. Iñaki.
          Hide
          Thijs Kinkhorst added a comment -

          So, anything against merging this into master?

          Show
          Thijs Kinkhorst added a comment - So, anything against merging this into master?
          Hide
          Dan Poltawski added a comment -

          No, i'm sending this for peer review.

          Show
          Dan Poltawski added a comment - No, i'm sending this for peer review.
          Hide
          Dan Poltawski added a comment -

          Can you clarify which branches are the ones we are talking about here - is it wip_master_mdl-31938_fix_cas_auth?

          (If you could update the pull fields in the tracker here it woudl help).

          For the record: I think that we should integrate these straight away despite being security issues (and usually waiting till point releases) because they are already widely publicly disclosed.

          Show
          Dan Poltawski added a comment - Can you clarify which branches are the ones we are talking about here - is it wip_master_mdl-31938_fix_cas_auth? (If you could update the pull fields in the tracker here it woudl help). For the record: I think that we should integrate these straight away despite being security issues (and usually waiting till point releases) because they are already widely publicly disclosed.
          Hide
          Thijs Kinkhorst added a comment -

          This should be the correct branch: https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-31938_fix_cas_auth

          Re last sentence: yes, but as discussed elsewhere, in the current release branches the chirurgical fix MDL-36818 should be the one that's applied straight away.

          Show
          Thijs Kinkhorst added a comment - This should be the correct branch: https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-31938_fix_cas_auth Re last sentence: yes, but as discussed elsewhere, in the current release branches the chirurgical fix MDL-36818 should be the one that's applied straight away.
          Hide
          Dan Poltawski added a comment -

          I have sorted out MDL-36818 so that gets sent for integration and applied ASAP.

          Show
          Dan Poltawski added a comment - I have sorted out MDL-36818 so that gets sent for integration and applied ASAP.
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          Sorry about the delay with action on this..

          The fix looks good for integration in general, but there is something weird about the branch - when I try and pull it into master, I get unstated changes (I have never ever seen git manage to do this before!):

          $ git reset --hard f8eff10319e1bd67e10634e79002b850702946c6
          $ git pull https://github.com/iarenaza/moodle.git wip_master_mdl-31938_fix_cas_auth
          $ git status
          # On branch MDL-31938-master
          # Your branch is ahead of 'origin/master' by 5 commits.
          #   (use "git push" to publish your local commits)
          #
          # Changes not staged for commit:
          #   (use "git add/rm <file>..." to update what will be committed)
          #   (use "git checkout -- <file>..." to discard changes in working directory)
          #
          #	deleted:    auth/cas/CAS/CAS/Client.php
          #	deleted:    auth/cas/CAS/CAS/Languages/Catalan.php
          #	deleted:    auth/cas/CAS/CAS/Languages/English.php
          #	deleted:    auth/cas/CAS/CAS/Languages/French.php
          #	deleted:    auth/cas/CAS/CAS/Languages/German.php
          #	deleted:    auth/cas/CAS/CAS/Languages/Greek.php
          #	deleted:    auth/cas/CAS/CAS/Languages/Japanese.php
          #	deleted:    auth/cas/CAS/CAS/Languages/Spanish.php
          #
          no changes added to commit (use "git add" and/or "git commit -a")
          

          It would be better if that branch was without the merge commit, and all commits had the tracker issue ID in there:
          http://docs.moodle.org/dev/Coding_style#Git_commits

          Ideally it would be squashed into a single commit (or at two commits to ensure that Thijs gets credited as well as Iñaki).

          We also need to decide what to do about testing instructions for this. I think that a basic regression test - and then relying on you guys using it in production is probably best.

          [Y] Syntax
          [Y] Whitespace
          [-] Output
          [-] Language
          [-] Databases
          [N] Testing (instructions and automated tests)
          [Y] Security
          [-] Documentation
          [N] Git
          [Y] Sanity check

          Show
          Dan Poltawski added a comment - Hi Guys, Sorry about the delay with action on this.. The fix looks good for integration in general, but there is something weird about the branch - when I try and pull it into master, I get unstated changes (I have never ever seen git manage to do this before!): $ git reset --hard f8eff10319e1bd67e10634e79002b850702946c6 $ git pull https: //github.com/iarenaza/moodle.git wip_master_mdl-31938_fix_cas_auth $ git status # On branch MDL-31938-master # Your branch is ahead of 'origin/master' by 5 commits. # (use "git push" to publish your local commits) # # Changes not staged for commit: # (use "git add/rm <file>..." to update what will be committed) # (use "git checkout -- <file>..." to discard changes in working directory) # # deleted: auth/cas/CAS/CAS/Client.php # deleted: auth/cas/CAS/CAS/Languages/Catalan.php # deleted: auth/cas/CAS/CAS/Languages/English.php # deleted: auth/cas/CAS/CAS/Languages/French.php # deleted: auth/cas/CAS/CAS/Languages/German.php # deleted: auth/cas/CAS/CAS/Languages/Greek.php # deleted: auth/cas/CAS/CAS/Languages/Japanese.php # deleted: auth/cas/CAS/CAS/Languages/Spanish.php # no changes added to commit (use "git add" and/or "git commit -a" ) It would be better if that branch was without the merge commit, and all commits had the tracker issue ID in there: http://docs.moodle.org/dev/Coding_style#Git_commits Ideally it would be squashed into a single commit (or at two commits to ensure that Thijs gets credited as well as Iñaki). We also need to decide what to do about testing instructions for this. I think that a basic regression test - and then relying on you guys using it in production is probably best. [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [N] Testing (instructions and automated tests) [Y] Security [-] Documentation [N] Git [Y] Sanity check
          Hide
          Iñaki Arenaza added a comment -

          Hi Dan,

          probably my fault. I looks like I didn't rebase the patch on the latest master branch. I've just done it and also reworked the commits as you suggested (squash Thijs's commits into one, add the tracker issue ID to all of them, and removed the merge commit). I've pushed the branch to github right now.

          If I repeat the exact same steps you did, it merges cleanly and leaves no unstaged changes at all.

          Hope that helps.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Dan, probably my fault. I looks like I didn't rebase the patch on the latest master branch. I've just done it and also reworked the commits as you suggested (squash Thijs's commits into one, add the tracker issue ID to all of them, and removed the merge commit). I've pushed the branch to github right now. If I repeat the exact same steps you did, it merges cleanly and leaves no unstaged changes at all. Hope that helps. Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          Thanks Iñaki,

          It looks fine now - sending for integration

          Show
          Dan Poltawski added a comment - Thanks Iñaki, It looks fine now - sending for integration
          Hide
          Dan Poltawski added a comment -

          Oh, testing instructions!

          Show
          Dan Poltawski added a comment - Oh, testing instructions!
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Iñaki Arenaza added a comment -

          Pushed the rebased branch.

          Ciao.

          Show
          Iñaki Arenaza added a comment - Pushed the rebased branch. Ciao.
          Hide
          Marina Glancy added a comment -

          it's badly missing testing instructions... at least to understand why we need this change

          Show
          Marina Glancy added a comment - it's badly missing testing instructions... at least to understand why we need this change
          Hide
          Dan Poltawski added a comment -

          Marina: the main point of upgrading is to get us off a badly outdated version of the library (currently on 1.1.2, released 3 years ago) which has multiple security vulnerabilities reported in it (making it increasingly more difficult to patch).

          In terms of testing - we need to verify its still working . I will add some basic testing instructions, but I think that we really need to rely a bit on the CAS using universities as I doubt out basic testing will be as through as some uni setups.

          Of course, i'm more confident about that because of Iñaki and Thijs using it and worked on it.

          Show
          Dan Poltawski added a comment - Marina: the main point of upgrading is to get us off a badly outdated version of the library (currently on 1.1.2, released 3 years ago) which has multiple security vulnerabilities reported in it (making it increasingly more difficult to patch). In terms of testing - we need to verify its still working . I will add some basic testing instructions, but I think that we really need to rely a bit on the CAS using universities as I doubt out basic testing will be as through as some uni setups. Of course, i'm more confident about that because of Iñaki and Thijs using it and worked on it.
          Hide
          Dan Poltawski added a comment -

          I just created a new branch with the change to thirdparty.xml squashed into Iñakis commit

          Show
          Dan Poltawski added a comment - I just created a new branch with the change to thirdparty.xml squashed into Iñakis commit
          Hide
          Marina Glancy added a comment -

          Thanks a lot Inaki and everybody participating in this issue. Integrated in master

          I removed security level because it's a known 3rd party bug (confirmed with Dan)

          Show
          Marina Glancy added a comment - Thanks a lot Inaki and everybody participating in this issue. Integrated in master I removed security level because it's a known 3rd party bug (confirmed with Dan)
          Hide
          Damyon Wiese added a comment -

          Passing this test - seems to work the same as before.

          Show
          Damyon Wiese added a comment - Passing this test - seems to work the same as before.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          "Aequam memento rebus in arduis servare mentem"

          Many thanks for your hard work, this is now part of "Moodle, the LMS". Closing!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - "Aequam memento rebus in arduis servare mentem" Many thanks for your hard work, this is now part of "Moodle, the LMS". Closing! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: