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

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".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 2 2015, 2:35 PM
Vituzzu changed the title from "" to "Incorrect parsing of IPs for global block".May 2 2015, 2:35 PM
Vituzzu edited the task description. (Show Details)
Vituzzu added a project: Security.
Vituzzu changed Security from None to Software security bug.
Vituzzu edited subscribers, added: Vituzzu, Melos, hoo, Krenair; removed: Aklapper.
Krenair merged a task: Restricted Task.May 2 2015, 4:27 PM
hoo added a comment.May 2 2015, 4:53 PM

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.

Anomie added a subscriber: Anomie.May 4 2015, 2:08 PM

'/(?<=\.)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.

Vituzzu added a subscriber: saper.May 5 2015, 8:19 PM
dpatrick triaged this task as "Normal" priority.May 21 2015, 9:49 PM
dpatrick claimed this task.
dpatrick moved this task from Backlog to In Progress on the Security-Team board.

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()?

dpatrick moved this task from In Progress to Waiting on the Security-Team board.Jun 4 2015, 10:12 PM
dpatrick moved this task from Waiting to In Progress on the Security-Team board.

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?

dpatrick moved this task from In Progress to Waiting on the Security-Team board.Jun 9 2015, 6:13 PM

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.

dpatrick moved this task from Waiting to In Progress on the Security-Team board.Jun 10 2015, 5:34 PM

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().

dpatrick moved this task from In Progress to Waiting on the Security-Team board.Jun 12 2015, 11:39 PM

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.

dpatrick moved this task from Waiting to In Progress on the Security-Team board.Jul 7 2015, 6:59 PM

This patch integrates zero trimming into sanitizeIP:

dpatrick moved this task from In Progress to Waiting on the Security-Team board.Jul 13 2015, 6:07 PM

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:

@Anomie, any more comments?

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?

csteipp closed this task as "Resolved".

18:53 csteipp: deployed patches for T97897 & T115522

Legoktm reopened this task as "Open".Nov 2 2015, 8:40 PM
Legoktm added a subscriber: Legoktm.
dpatrick moved this task from Waiting to In Progress on the Security-Team board.Nov 9 2015, 9:17 PM

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.

dpatrick moved this task from In Progress to Waiting on the Security-Team board.Nov 14 2015, 4:22 AM

LGTM

csteipp closed this task as "Resolved".Nov 19 2015, 10:27 PM

22:11 csteipp: deployed patch for T97897

demon added a subscriber: demon.Dec 15 2015, 7:39 PM
{F2961639}

LGTM

Patch applies cleanly to 1.24 and beyond.

Patch for 1.23:

demon added a subscriber: Grunny.Dec 17 2015, 1:19 AM
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.
Reedy added a project: MediaWiki-User-blocking.
csteipp edited the task description. (Show Details)Dec 24 2015, 12:12 AM
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptDec 24 2015, 12:12 AM
Restricted Application added a subscriber: JEumerus. · View Herald TranscriptJan 24 2016, 2:41 PM