Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31938

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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.
      2. 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment - https://bugzilla.redhat.com/show_bug.cgi?id=801343 https://bugzilla.redhat.com/show_bug.cgi?id=801347
            Hide
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            stronk7 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
            stronk7 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
            iarenaza 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
            iarenaza 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
            thijskh 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
            thijskh 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
            iarenaza 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
            iarenaza 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
            thijskh 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
            thijskh 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
            iarenaza 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
            iarenaza 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
            thijskh Thijs Kinkhorst added a comment -

            So, anything against merging this into master?

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

            No, i'm sending this for peer review.

            Show
            poltawski Dan Poltawski added a comment - No, i'm sending this for peer review.
            Hide
            poltawski 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
            poltawski 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
            thijskh 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
            thijskh 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - I have sorted out MDL-36818 so that gets sent for integration and applied ASAP.
            Hide
            poltawski 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
            poltawski 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
            iarenaza 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
            iarenaza 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
            poltawski Dan Poltawski added a comment -

            Thanks Iñaki,

            It looks fine now - sending for integration

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

            Oh, testing instructions!

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

            Pushed the rebased branch.

            Ciao.

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

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

            Show
            marina Marina Glancy added a comment - it's badly missing testing instructions... at least to understand why we need this change
            Hide
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - I just created a new branch with the change to thirdparty.xml squashed into Iñakis commit
            Hide
            marina 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 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 Damyon Wiese added a comment -

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

            Show
            damyon Damyon Wiese added a comment - Passing this test - seems to work the same as before.
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  18/Nov/13