Page MenuHomePhabricator

Incorrect parsing of IPs for global block
Closed, ResolvedPublic

Description

Today I mistakenly gave an IP in the form X.000.X.X to my steward bot and this apparently blocked all newly registered users (it was showed on the top of newbies' contribs). I was afk and Melos tried unblocking it via unblock form but it wasn't listed as being blocked, then he eventually managed to unblock it via api.
Here's the "IP" https://meta.wikimedia.org/w/index.php?title=Special%3ALog&type=&user=&page=User%3A141.000.11.253&year=&month=-1&tagfilter=&hide_patrol_log=1&hide_tag_log=1&hide_thanks_log=1


patches:

  • 1.24 - master:
  • 1.23:

CVE: CVE-2015-8627

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".May 2 2015, 2:35 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Vituzzu updated the task description. (Show Details)
Vituzzu added a project: acl*security.
Vituzzu changed Security from None to Software security bug.
Vituzzu edited subscribers, added: Vituzzu, Melos, hoo, Krenair; removed: Aklapper.

The root cause for this is that MediaWiki's IP class considers these IP addresses valid, but is not able to convert them to hex. I fixed the conversion to hex. See also T62035.

'/(?<=\.)0+(?=([1-9]|0\.))/'

Unnecessary parens inside the (?=).

Also, consider this address: 00192.0.2.000, the extraneous zeros won't be stripped. I'd recommend using '/(?:^|(?<=\.))0+(?=[1-9]|0\.|0$)/' instead.

dpatrick triaged this task as Medium priority.

I concur with @Anomie's regex correction. However, can we refactor out the preg_replace() into a separate function so that we can unit test that more effectively? Or, toward the same aim, can that preg_replace() be moved to the IPv4 handling portion of sanitizeIP()?

Attached for review/comments are two patches:

which is just the original patch, but with @Anomie's regex correction;

is a refactor which pulls out the preg_match() and adds additional test cases.

I think the latter is preferable for maintenance and code review reasons. What say others?

which is just the original patch, but with @Anomie's regex correction;

Looks ok. Haven't tested.

is a refactor which pulls out the preg_match() and adds additional test cases.

I'm not liking that we would now have IP::sanitizeIP()/IP::prettifyIP() that mostly ignores IPv4 and IP::standardizeIPv4() that does real cleanup for v4 but returns false for v6. I'd rather see the public behavior of your IP::standardizeIPv4() integrated into IP::sanitizeIP(). Then just use IP::sanitizeIP() inside the IP::toHex() case for IPv4 like it already does for IPv6. Or is there some sort of BC concern that prevents that?

Code-wise I don't care whether you put the logic from IP::standardizeIPv4() directly into IP::sanitizeIP() or split both the IPv4 and IPv6 logic into private functions with IP::sanitizeIP() just checking the type and calling the appropriate one. As for IP::toHex(), since we're touching it to this extent IMO either IP::IPv6ToRawHex() should be merged in or the IPv4 logic should be split out to a private IP::IPv4ToRawHex(), as it is now it's just weird.

is a refactor which pulls out the preg_match() and adds additional test cases.

I'm not liking that we would now have IP::sanitizeIP()/IP::prettifyIP() that mostly ignores IPv4 and IP::standardizeIPv4() that does real cleanup for v4 but returns false for v6. I'd rather see the public behavior of your IP::standardizeIPv4() integrated into IP::sanitizeIP(). Then just use IP::sanitizeIP() inside the IP::toHex() case for IPv4 like it already does for IPv6. Or is there some sort of BC concern that prevents that?

I do have backwards compatibility concerns, yes. That's why i didn't add the code to sanitizeIP() in this patch. Here are some examples:

I stopped examining grep results there, but there were other calls to IP::sanitizeIP() from the CheckUser, GlobalBlocking, ProofreadPage, and TorBlock extensions. In short, I hated to add another method, but I was concerned about breaking stuff by changing the behavior of IP::sanitizeIP().

Code-wise I don't care whether you put the logic from IP::standardizeIPv4() directly into IP::sanitizeIP() or split both the IPv4 and IPv6 logic into private functions with IP::sanitizeIP() just checking the type and calling the appropriate one. As for IP::toHex(), since we're touching it to this extent IMO either IP::IPv6ToRawHex() should be merged in or the IPv4 logic should be split out to a private IP::IPv4ToRawHex(), as it is now it's just weird.

We can't merge IP::IPv6ToRawHex() into IP::toHex() because it's called from IP::parseCIDR6(). I suppose it makes sense to pull the IPv4 functionality out into a separate IP::IPv4ToRawHex() function, but that function would still need to invoke a separate IP::standardizeIPv4()/IP::removeLeadingIPv4OctetZeros()/whatever in order to meet me original goal of improving testability of the preg_replace().

I'd rather actually look into those calls to see if anything really would break. XFF header parsing, for example, really should be normalizing the zeros to prevent spurious non-matches if some upstream is injecting IPs with the abnormal zeros.

This patch integrates zero trimming into sanitizeIP:

This patch integrates zero trimming into sanitizeIP:

Comment for sanitizeIP should be updated to reflect the change for ipv4. Otherwise I think that addresses @Anomie's comments.

@csteipp, thanks. Updated and rebased:

seems ok to me. Haven't tested.

I wasn't able to duplicate Vituzzu original issue in my testing, where all newbies were blocked. But I can confirm that after this patch, adding a block with 000 in it is normalized correctly to a 0, and the IP is actually blocked, where before the IP wasn't. Both via the Special page and API. Setting a local block exemption seems to work correctly with the patch as well.

Also, if a trusted proxy adds an IP with a 00 in it, the user is still correctly blocked, although there's a php warning,

Warning: inet_pton(): Unrecognized address 141.00.11.253 in <b>/var/www/wiki/en/vendor/wikimedia/ip-set/src/IPSet.php

I'll go ahead and deploy this in the next day or two.

Swell. Thanks Chris. Is that warning worth mentioning to the IPSet maintainer?

18:53 csteipp: deployed patches for T97897 & T115522

Here is an updated patch

which addresses T117471 and T117476. These were both caused by sanitizeIP() being invoked on login via User -> Title -> MediaWikiTitleCodec. The patch limits 0 removal to only strings which appear as valid IPv4 addresses.

22:11 csteipp: deployed patch for T97897

LGTM

Patch applies cleanly to 1.24 and beyond.

Patch for 1.23:

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 18 2015, 12:40 AM
demon changed the edit policy from "Custom Policy" to "All Users".
Restricted Application changed the visibility from "Public (No Login Required)" to "Custom Policy". · View Herald TranscriptDec 18 2015, 12:40 AM
Restricted Application changed the edit policy from "All Users" to "Custom Policy". · View Herald Transcript
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 18 2015, 6:41 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".
Reedy changed Security from Software security bug to None.