Page MenuHomePhabricator

NetworkAuth: Security: Bad IP range recognition
Closed, ResolvedPublic

Description

Author: olaf

Description:
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] ))

{

  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.


Version: unspecified
Severity: critical
URL: http://www.mediawiki.org/wiki/Extension:NetworkAuth

Details

Reference
bz38117

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 12:52 AM
bzimport set Reference to bz38117.
bzimport added a subscriber: Unknown Object (MLST).

olaf wrote:

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] )))

{

  1. authenticate user

}

olaf wrote:

Patch that fixes the problem.

attachment fix.patch ignored as obsolete

olaf wrote:

Patch that really fixes the problem.

Attached:

Tim: ping - do you still maintain this?

olaf wrote:

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... :-/

t.laqua wrote:

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.

Uh, thanks a lot for elaborating! Closing as FIXED then.