[MDL-37625] Included PEAR Crypt_CHAP library uses obsolete mhash() code which breaks RADIUS auth support in some PHP 5.3 installs - Moodle Tracker

Details

Type: Bug
Status: Closed
Priority: Major
Resolution: Fixed
Affects Version/s: 2.2.5, 2.3.4, 2.4.1
Fix Version/s: 2.3.5, 2.4.2
Component/s: Authentication, Libraries
Labels:
- partner
- patch
- remote-learner
- triaged

Database:

Any
Testing Instructions:
Hide

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
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
URL:
https://github.com/jfilip/moodle
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 2.4 Diff URL:
https://github.com/jfilip/moodle/compare/MOODLE_24_STABLE...MDL-37625-m24
Pull Master Branch:
~~MDL-37625~~-master
Pull Master Diff URL:
https://github.com/jfilip/moodle/compare/master...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).

http://php.net/manual/en/intro.mhash.php

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

testing discovered

MDL-38373 Strict standards error with auth_radius

Closed

will help resolve

MDL-32000 RADIUS authentification

Closed

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Justin Filip added a comment - 22/Jan/13 3:23 PM

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

Show

Justin Filip added a comment - 22/Jan/13 3:23 PM My MacPorts-based PHP 5.4.11 install also has the mhash module available.

Hide

Permalink

Justin Filip added a comment - 22/Jan/13 3:47 PM

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

Show

Justin Filip added a comment - 22/Jan/13 3:47 PM Adding patch information for a commit with the updated library version.

Hide

Permalink

Michael de Raadt added a comment - 30/Jan/13 6:53 AM

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 - 30/Jan/13 6:53 AM Thanks for reporting and patching that, Justin. Feel free to request a peer review. Petr: perhaps you could peer review this.

Hide

Permalink

Petr Škoda added a comment - 30/Jan/13 10:24 AM

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

Show

Petr Škoda added a comment - 30/Jan/13 10:24 AM +1, there is not much we can review, the code is from upstream, I think testing is enough

Hide

Permalink

Justin Filip added a comment - 30/Jan/13 2:04 PM

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 - 30/Jan/13 2:04 PM 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

Permalink

Petr Škoda added a comment - 30/Jan/13 2:08 PM

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 - 30/Jan/13 2:08 PM 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

Permalink

Justin Filip added a comment - 30/Jan/13 2:12 PM

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 - 30/Jan/13 2:12 PM 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

Permalink

Justin Filip added a comment - 20/Feb/13 4:09 PM

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 - 20/Feb/13 4:09 PM 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

Permalink

Dan Poltawski added a comment - 01/Mar/13 9:17 AM

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 - 01/Mar/13 9:17 AM 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

Permalink

Justin Filip added a comment - 01/Mar/13 6:09 PM

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 - 01/Mar/13 6:09 PM 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

Permalink

Sam Hemelryk added a comment - 04/Mar/13 1:26 AM

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

Show

Sam Hemelryk added a comment - 04/Mar/13 1:26 AM Thanks Justin, this has been integrated now on 23, 24, and master.

Hide

Permalink

David Monllaó added a comment - 07/Mar/13 9:32 AM - 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 - 07/Mar/13 9:32 AM - 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

Permalink

Eloy Lafuente (stronk7) added a comment - 07/Mar/13 11:12 AM

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 - 07/Mar/13 11:12 AM 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

Permalink

Eloy Lafuente (stronk7) added a comment - 07/Mar/13 2:48 PM

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 - 07/Mar/13 2:48 PM 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

Permalink

Eloy Lafuente (stronk7) added a comment - 07/Mar/13 4:20 PM

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 - 07/Mar/13 4:20 PM 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

Permalink

David Monllaó added a comment - 08/Mar/13 2:42 AM

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

Show

David Monllaó added a comment - 08/Mar/13 2:42 AM For the record, there is a VM in the HQ servers to use in further RADIUS-related tests

People

Assignee:

Justin Filip

Reporter:

Justin Filip

Peer reviewer:

Dan Poltawski

Integrator:

Sam Hemelryk

Tester:

David Monllaó

Participants:

Dan Poltawski, David Monllaó, Eloy Lafuente (stronk7), Justin Filip, Michael de Raadt, Petr Škoda, Sam Hemelryk

Votes:

5 Vote for this issue

Watchers:

9 Start watching this issue

Dates

Created:

22/Jan/13 3:14 PM

Updated:

08/Mar/13 2:42 AM

Resolved:

07/Mar/13 4:20 PM

Agile

View on Board

Details

Requirements

Testing steps

Description

Issue Links

Activity

People

Dates

Agile