Page MenuHomePhabricator

CheckUser should use methods from the IP class to validate IPs and CIDR ranges
Closed, ResolvedPublic

Description

Marking as security for now since it contains an IP range.

I was able to block the range 2600:7400:0:0:0:0:0:0/28 (or as I had entered it in the block form, 2600:7400::/28), but when I tried to run a check on that same range, I got the "invalid IP" error (I tried both forms and got the same result). It is a valid /28 range of IPv6 addresses, so I presume there is something wrong with the CheckUser code. I will investigate and propose a patch myself soon.

Event Timeline

Huji created this task.Jul 26 2017, 2:34 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 26 2017, 2:34 AM
Huji claimed this task.
dpatrick triaged this task as Normal priority.Jul 26 2017, 4:55 PM

Turns out the reason for this is that $wgBlockCIDRLimit by default allows blocking IPv6 in up to /19, but $wgCheckUserCIDRLimit will not allow checking ranges wider than /32. Not knowing what the reason behind this is, I will leave it as is for now.

However, as I was investigating this I noted the CheckUser uses a regex-based method to identify IPs and ranges, which is bad because we already have methods to do this in MediaWiki core. So I will submit a patch to fix that.

Restricted Application added a project: Security. · View Herald TranscriptJul 27 2017, 11:28 PM
Huji renamed this task from Certain IPv6 ranges cannot be checked but can be blocked to CheckUser should use methods from the IP class to validate IPs and CIDR ranges.
Huji edited subscribers, added: gerritbot; removed: GerritBot.
Huji added a comment.Jul 27 2017, 11:42 PM

So apparently Gerritbot cannot send notifications here due to the Custom Policy (which by the way can be removed). So I am adding a link to the patch manually: https://gerrit.wikimedia.org/r/#/c/368327/

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 16 2017, 10:52 PM
Huji moved this task from Backlog to Patches in review on the CheckUser board.Aug 28 2017, 12:29 PM

Change 368327 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Use methods from the IP class to validate IPs and CIDR ranges

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

Huji closed this task as Resolved.Sep 6 2017, 1:09 AM
Huji removed a project: Patch-For-Review.
Huji reopened this task as Open.Oct 10 2017, 3:55 PM

Re-opening it as the problem persists despite the patch. A range like 2A01:4F8:0:0:0:0:0:0/29 works when you block it, but gives an "Invalid IP address" error when you try to check it with CU. I think the :4F8: part is what is throwing it off (it is only three characters long, not 4).

Melos added a subscriber: Melos.Oct 10 2017, 4:25 PM

I think it don't pass the part ( IP::isIPv6( $ip ) && $range < $wgCheckUserCIDRLimit['IPv6'] ) ) since 29 < 32 (range is too wide). Is the bug in the block stuffs instead?

Huji closed this task as Resolved.EditedOct 10 2017, 4:32 PM

Actually, it is not a bug; the IPv6 default for maximum range size is different for blocks and CheckUser. I don't know why (and I wish they were not the same) but there is no issue with the code.

CheckUser's config: $wgCheckUserCIDRLimit['IPv6'] = 32
Block config: $wgBlockCIDRLimit['IPv6'] = 19