Page MenuHomePhabricator

IP::isInRange() returns true on nonsense input
Closed, ResolvedPublic

Description

In AbuseFilter

is_in_range(ip, range)

will give true, if ip is not an ip and range is not a range.

The reason seems to be that

IP::isInRange($ip, $range)

in mediawiki-core(?), see https://doc.wikimedia.org/mediawiki-core/master/php/IP_8php_source.html#l00637, uses

strcmp(. , .) >= 0 && strcmp(. , .) <= 0,

which is always true, if the params are not defined, because strcmp results in 0.

This can lead to big problems, if somebody makes a minor error in using the AbuseFilter extension, e.g.

is_in_range(user_name, "127.0.1/16")
is_in_range("127.0.1/16", user_name)
is_in_range(user_name, "wtf")

Details

Related Gerrit Patches:

Event Timeline

seth created this task.Jan 19 2016, 11:33 PM
seth raised the priority of this task from to Medium.
seth updated the task description. (Show Details)
seth added a subscriber: seth.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 19 2016, 11:33 PM

We could try to check the IP range at validation level and do IP::isIPAddress( $IP ) && IP::isInRange( $ip, $range ) during evaluation instead of just checking isInRange?

Change 266595 had a related patch set uploaded (by MtDu):
Fix IP::isInRange() returning true on nonsensical outputs

https://gerrit.wikimedia.org/r/266595

MtDu added a subscriber: MtDu.Jan 26 2016, 9:31 PM

Not sure if this works. (letting result = null) Let me know your opinions.
Thanks,
MtDu

Change 275544 had a related patch set uploaded (by Glaisher):
Don't allow invalid IP ranges to be entered in ip_in_range()

https://gerrit.wikimedia.org/r/275544

Change 275564 had a related patch set uploaded (by Glaisher):
Add note that IP::isInRange() can return unexpected results for invalid args

https://gerrit.wikimedia.org/r/275564

Change 275564 merged by jenkins-bot:
Add note that IP::isInRange() can return unexpected results for invalid args

https://gerrit.wikimedia.org/r/275564

Change 266595 abandoned by MtDu:
Check whether IP is valid in ip_in_range()

Reason:
Per @TTOs comment.

https://gerrit.wikimedia.org/r/266595

Change 275544 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Don't allow invalid IP ranges to be entered in ip_in_range()

https://gerrit.wikimedia.org/r/275544

Huji added a subscriber: Huji.

@matej_suchanek are we all set here?

matej_suchanek closed this task as Resolved.Jun 10 2018, 8:21 AM
matej_suchanek moved this task from Backlog to Filtering features on the AbuseFilter board.

Yes! \o/