Moodle
  1. Moodle
  2. MDL-37625

Included PEAR Crypt_CHAP library uses obsolete mhash() code which breaks RADIUS auth support in some PHP 5.3 installs

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Requirements

      1. a RADIUS server
      2. the PHP RADIUS extension installed and available on the web server of your Moodle testing site

      Testing steps

      1. log into your Moodle test site as a system administrator
      2. configure the RADIUS auth plugin in Moodle by going to the following path in the Settings block – Site administration -> Plugins -> Authentication -> Manage authentication
      3. enable and configure the RADIUS server plugin so that it communicates with your RADIUS server
      4. log out of the Moodle site
      5. attempt to log into the Moodle site using the credentials for a test user from the RADIUS server
      6. you should be able to successfully log in and have a new account created for you if you have never logged in with that user before
      Show
      Requirements a RADIUS server if you don't have one available you can setup one using FreeRADIUS – http://freeradius.org/ the PHP RADIUS extension installed and available on the web server of your Moodle testing site Testing steps log into your Moodle test site as a system administrator configure the RADIUS auth plugin in Moodle by going to the following path in the Settings block – Site administration -> Plugins -> Authentication -> Manage authentication enable and configure the RADIUS server plugin so that it communicates with your RADIUS server log out of the Moodle site attempt to log into the Moodle site using the credentials for a test user from the RADIUS server you should be able to successfully log in and have a new account created for you if you have never logged in with that user before
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-37625-m24
    • Pull Master Branch:
      MDL-37625-master
    • Rank:
      47316

      Description

      The PHP mhash module is obsoleted by the hash module (the latter of which is a required extension for current Moodle installs).

      The Crypt_CHAP PEAR library that is currently included with Moodle uses mhash() calls which breaks on some PHP 5.3 installs due to the mhash extension not being available. This was a bug reported, fixed and released for this PEAR library in 2010.

      Note: It looks like Red Hat Linux does not even have the mhash module available for install but Debian-based distros do include it (even with PHP 5.4). This is output from my Debian workstation:

      • $ php --version
        PHP 5.4.4-12 (cli) (built: Jan 21 2013 10:32:30) 
        Copyright (c) 1997-2012 The PHP Group
        Zend Engine v2.4.0, Copyright (c) 1998-2012 Zend Technologies
            with Xdebug v2.2.1, Copyright (c) 2002-2012, by Derick Rethans
        
        $ php -m | grep hash
        hash
        mhash
        

        Issue Links

          Activity

          Hide
          Justin Filip added a comment -

          My MacPorts-based PHP 5.4.11 install also has the mhash module available.

          Show
          Justin Filip added a comment - My MacPorts-based PHP 5.4.11 install also has the mhash module available.
          Hide
          Justin Filip added a comment -

          Adding patch information for a commit with the updated library version.

          Show
          Justin Filip added a comment - Adding patch information for a commit with the updated library version.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting and patching that, Justin. Feel free to request a peer review.

          Petr: perhaps you could peer review this.

          Show
          Michael de Raadt added a comment - Thanks for reporting and patching that, Justin. Feel free to request a peer review. Petr: perhaps you could peer review this.
          Hide
          Petr Škoda added a comment -

          +1, there is not much we can review, the code is from upstream, I think testing is enough

          Show
          Petr Škoda added a comment - +1, there is not much we can review, the code is from upstream, I think testing is enough
          Hide
          Justin Filip added a comment -

          Testing is the tough one. It's what has prevented me from sending this for review so far. We have a client who is currently suffering from this problem. We can check that the problem goes away and their RADIUS auth starts working correctly for them. I checked the QA tests from 2.4 and there were no tests for this auth plug-in. Is it worth while to come up with some instructions for how to setup a test RADIUS server to validate that this plug-in works?

          Show
          Justin Filip added a comment - Testing is the tough one. It's what has prevented me from sending this for review so far. We have a client who is currently suffering from this problem. We can check that the problem goes away and their RADIUS auth starts working correctly for them. I checked the QA tests from 2.4 and there were no tests for this auth plug-in. Is it worth while to come up with some instructions for how to setup a test RADIUS server to validate that this plug-in works?
          Hide
          Petr Škoda added a comment -

          I am planning to set up some read only test VMs with all these external services necessary for testing of auth plugins. Hopefully it will be available for HQ CI servers before 2.5 release.

          Show
          Petr Škoda added a comment - I am planning to set up some read only test VMs with all these external services necessary for testing of auth plugins. Hopefully it will be available for HQ CI servers before 2.5 release.
          Hide
          Justin Filip added a comment -

          Sounds good to me. We'll get my branch tested on our clent's site that is broken right now to verify if it works and then I'll push this through the rest of the process.

          Show
          Justin Filip added a comment - Sounds good to me. We'll get my branch tested on our clent's site that is broken right now to verify if it works and then I'll push this through the rest of the process.
          Hide
          Justin Filip added a comment -

          I'm requesting a peer review on this one as we can now confirm that this fix has allowed the RADIUS auth plugin to work with our affected client installation.

          Show
          Justin Filip added a comment - I'm requesting a peer review on this one as we can now confirm that this fix has allowed the RADIUS auth plugin to work with our affected client installation.
          Hide
          Dan Poltawski added a comment -

          Hi Justin,

          It looks good to me, can't really argue with the upstream changes.

          We do need some testing instructions for this though, even if its difficult to setup the test, could you give us the requirements to setup a test of it?

          Also, I assume you have tested it, so it at least gives us some confidence in applying this fix

          [-] Syntax
          [-] Output
          [-] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Dan Poltawski added a comment - Hi Justin, It looks good to me, can't really argue with the upstream changes. We do need some testing instructions for this though, even if its difficult to setup the test, could you give us the requirements to setup a test of it? Also, I assume you have tested it, so it at least gives us some confidence in applying this fix [-] Syntax [-] Output [-] Whitespace [-] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check
          Hide
          Justin Filip added a comment -

          Thanks, Dan.

          I put in rough instructions. In our case, the client we host who had the problem was connecting to their own RADIUS server that was external from our network. We do not maintain any setups internally so I can't really provide any further detailed instructions on how to do the RADIUS setup. =(

          We did test this, though, and it resolved the issue and allowed users to successfully authenticated into the client's Moodle site again.

          Show
          Justin Filip added a comment - Thanks, Dan. I put in rough instructions. In our case, the client we host who had the problem was connecting to their own RADIUS server that was external from our network. We do not maintain any setups internally so I can't really provide any further detailed instructions on how to do the RADIUS setup. =( We did test this, though, and it resolved the issue and allowed users to successfully authenticated into the client's Moodle site again.
          Hide
          Sam Hemelryk added a comment -

          Thanks Justin, this has been integrated now on 23, 24, and master.

          Show
          Sam Hemelryk added a comment - Thanks Justin, this has been integrated now on 23, 24, and master.
          Hide
          David Monllaó added a comment - - edited

          It passes, tested in 22, 23, 24 and master with a RADIUS VM using Microsoft CHAP 1 authentication and freeradius.

          I've received a strict standards error in 23 and 24, but IIRC we are not compliant with strict standards until 24. I wait for integrators feedback to pass it or fail it.

          Strict Standards: Non-static method PEAR::loadExtension() should not be called statically, assuming $this from incompatible context in /var/www/integration/MOODLE_24_STABLE/lib/pear/Auth/RADIUS.php on line 49 Strict Standards: Non-static method PEAR::isError() should not be called statically, assuming $this from incompatible context in /var/www/integration/MOODLE_24_STABLE/auth/radius/auth.php on line 108
          

          Edit: I've just noticed that it hasn't been integrated in 22; anyway I also have mhash, that's why it hasn't failed.

          Show
          David Monllaó added a comment - - edited It passes, tested in 22, 23, 24 and master with a RADIUS VM using Microsoft CHAP 1 authentication and freeradius. I've received a strict standards error in 23 and 24, but IIRC we are not compliant with strict standards until 24. I wait for integrators feedback to pass it or fail it. Strict Standards: Non- static method PEAR::loadExtension() should not be called statically, assuming $ this from incompatible context in / var /www/integration/MOODLE_24_STABLE/lib/pear/Auth/RADIUS.php on line 49 Strict Standards: Non- static method PEAR::isError() should not be called statically, assuming $ this from incompatible context in / var /www/integration/MOODLE_24_STABLE/auth/radius/auth.php on line 108 Edit: I've just noticed that it hasn't been integrated in 22; anyway I also have mhash, that's why it hasn't failed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          it seems that the class has not been changed as trunk continue showing that static behavior:

          http://svn.php.net/viewvc/pear/packages/Auth_RADIUS/trunk/RADIUS.php?revision=257341&view=markup

          So, or we fix that in the PEAR wrapper or we consider switching to another (pure or wrapper) lib. If this had to be voted, I'd say "pure" is better (less dependencies). Like, for example:

          http://www.phpclasses.org/package/4326-PHP-Authenticate-users-with-a-RADIUS-server.html

          Although I don't know much really. Adding Jonathan Harker here coz he did the initial (and current) implementation.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - it seems that the class has not been changed as trunk continue showing that static behavior: http://svn.php.net/viewvc/pear/packages/Auth_RADIUS/trunk/RADIUS.php?revision=257341&view=markup So, or we fix that in the PEAR wrapper or we consider switching to another (pure or wrapper) lib. If this had to be voted, I'd say "pure" is better (less dependencies). Like, for example: http://www.phpclasses.org/package/4326-PHP-Authenticate-users-with-a-RADIUS-server.html Although I don't know much really. Adding Jonathan Harker here coz he did the initial (and current) implementation. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've created MDL-38373 and provided a trivial solution there for the stricts standard thingy. So I think this can be passed safely.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've created MDL-38373 and provided a trivial solution there for the stricts standard thingy. So I think this can be passed safely. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

          Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

          Thanks, closing as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!
          Hide
          David Monllaó added a comment -

          For the record, there is a VM in the HQ servers to use in further RADIUS-related tests

          Show
          David Monllaó added a comment - For the record, there is a VM in the HQ servers to use in further RADIUS-related tests

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: