Last modified: 2013-12-23 14:30:11 UTC
This is a security-related bug in the Extension:NetworkAuth. The problem is that the extension does not to match IP ranges correctly and thus authenticates IP addresses that do not belong to the specified IP range. Both the IP address that the extension gets via wfGetIP() and the IP range that is specified in LocalSettings.php are transformed to hex numbers via IP::toHex(). Afterwards the obtained IP adress is compared to the range to determine whether the address is in the range. Here is the somewhat simplified code. parsedRange is an array containing the lower and upper limits of the range. $ip = wfGetIP(); $hex = IP::toHex( $ip ); if ( $hex >= IP::toHex( $parsedRange[0] ) && $hex <= IP::toHex( $parsedRange[1] )) { # authenticate user } Unfortunately, the function IP::toHex() does *not* return a hex number, but a string containing the hex digits (e.g. IP::toHex("46.115.22.119") -> "2E771673") *without* the leading "0x". This works fine in most cases, as the string is implicitly typecast to a number and compared afterwards. However, in the case that the string of the IP range contains only decimal digits (e.g. IP::toHex("129.69.120.0") -> "81457800"), this fails spectacularly, as in the one case, it interprets the string as a hex number, and in the other case as a dec number. In the above case, this means that IP::toHex("46.115.22.119") > IP::toHex("129.69.120.0") == true This bug report is made problematic by the fact that at the moment I cannot even find the current code of the extension, as all "Download" links on http://www.mediawiki.org/wiki/Extension:NetworkAuth seem to be broken. I am willing to provide a patch when anyone can tell me where to find the repo containing the current code of the extension.
The proposed fix to this bug is to explicitly cast the transformed IP address to a number by using hexdec() like: $ip = wfGetIP(); $hex = hexdec(IP::toHex( $ip )); if ( $hex >= hexdec(IP::toHex( $parsedRange[0] )) && $hex <= hexdec(IP::toHex( $parsedRange[1] ))) { # authenticate user }
CC'ing the maintainer.
Created attachment 11184 [details] Patch that fixes the problem.
Created attachment 11185 [details] Patch that really fixes the problem.
Tim: ping - do you still maintain this?
The problem currently still exists in the code. However, I am about to take over maintenance of the whole extension, as Tim Laqua is busy. Still I think until I have actually taken over, the patch should be applied.
(In reply to comment #6) > I am about to take over maintenance of the whole extension. That's great to hear! > the patch should be applied. Not sure if somebody else feels comfortable enough to review that code... :-/
Patch looks good ;-) And no, I'm not maintaining it anymore - Olaf, feel free to take it over. I also can't apply the patch, I don't have a working copy checked out any more.
This patch is applied in the latest release of NetworkAuth. Does the problem still exist ?
> This patch is applied in the latest release of NetworkAuth. Direct link to the change on git.wikimedia.org or gerrit.wikimedia.org is highly welcome for such statements.
@Andre : The change was merged by id: d71a9e648c7cf9dd7196cc626e1fe057b42b9e4d in https://git.wikimedia.org/commit/mediawiki%2Fextensions%2FNetworkAuth/d71a9e648c7cf9dd7196cc626e1fe057b42b9e4d Now its splitted into a new NetworkAuthclass.php https://git.wikimedia.org/commit/mediawiki%2Fextensions%2FNetworkAuth/fefe8d2c38869c92d37e9f78fceee19899d6a154
Uh, thanks a lot for elaborating! Closing as FIXED then.
