I guess this is a combination of XFF-blocking plus T55008: MediaWiki:Autoblocker etc. reveals to third party that a (blocked) user has used the same IP address. cc @Skizzerz since he originally wrote this feature in https://static-bugzilla.wikimedia.org/23343 / d99bda6ee6b78345b8beaccc894a443a217d23cd.

I think we would want autoblocks to apply normally to trusted XFF headers, not doing that would render autoblocks useless on wiki setups that proxy behind a caching layer. I also think the reasoning for XFF blocks to operate on all XFF headers and not just trusted ones as laid out in T25343 still applies.

Would fixing the privacy leak in T55008 be sufficient to resolve this issue as well? This would require a fair amount of engineering effort and would also require removing a lot of details from the autoblock interface message (such as time of block, time of expiration, who performed the block, who was blocked, and the block reason; as any one of those could be used to examine the block log and discover the matching user or at least significantly reduce the number of possible users)

This may be overengineered, but I believe we could generate a stateless identifier that ties the message to the autoblock. For example E("autoblock" || autoblock id padded to a fixed length || random) where E represents symmetric encryption using a secret key specified in LocalSettings.php and random is just some random bytes so that multiple reports can't be correlated to guess at the autoblock. The fixed string "autoblock" is there so that we can check that the ciphertext passed to us is valid. Decrypting this must be done via a special page, which we can control access to via some user right (so the ability to turn that identifier into autoblock details can be controlled).

A faster fix would be to apply autoblocks only to trusted XFF headers, but we would lose out on some measure of abuse prevention there. It may be worth pursuing this in the meantime anyway just so production doesn't remain vulnerable in the meantime.

I did a bit of digging and found this was actually reported back in 2013(!), ugh as {T53842}. I'll dupe that here. The contents of that task are:

Copied from email to security@wm.o:

Hello,

I would like to report a privacy issue in MediaWiki, with the configuration used on Wikimedia projects. This is not technically a security issue, but following some advice on the MediaWiki support desk, I prefer not to make a public bugzilla report.

On Wikimedia projects, MediaWiki partially trusts the X-Forwarded-For header to apply blocks: $wgApplyIpBlocksToXff is true, as requested in T25343.

A side effect is that anyone can check if a given IP w.x.y.z is (auto)blocked on a given wiki by trying to edit a page on this wiki, with a browser configured to send a fake HTTP header "X-Forwarded-For: w.x.y.z". To state the problem differently: if you can restrict the search to a small enough IP range, you can find the IP of a blocked user, which is not supposed to be available (except to checkusers). Same problem as in T48457, but you don't need sysop rights. Also, the name of the blocked user corresponding to the IP can easily be checked, as it is displayed in the block message.

By sending requests with multiple XFF headers, a script could check multiple IPs at once and scan ranges efficiently (I did not try to find the limit of XFF headers per request, so I have no idea of how fast it would be).

Perhaps the X-Forwarded-For header should not be trusted for autoblocks? Unfortunately this would be good news for vandals... At least, I think that the name of the blocked user should not be visible in this case and there should be a small limit on the number of XFF headers taken into account.

Kind regards,

Orlodrim

---

Version: unspecified
Severity: normal

In T53842#570193, @Catrope wrote:

(In reply to comment #0)

On Wikimedia projects, MediaWiki partially trusts the X-Forwarded-For header
to
apply blocks: $wgApplyIpBlocksToXff is true, as requested in bug 23343.

It appears the core issue is that this setting ignores our other configuration which is set up to only trust XFF from specific whitelisted proxies.

From DefaultSettings.php:

/**

• Whether to look at the X-Forwarded-For header's list of (potentially spoofed)
• IPs and apply IP blocks to them. This allows for IP blocks to work with correctly-configured
• (transparent) proxies without needing to block the proxies themselves. */

$wgApplyIpBlocksToXff = false;

(note the "potentially spoofed" bit)

In T53842#570199, @csteipp wrote:

Orlodrim, sorry for the delayed response on this. It does make me uncomfortable, so we do want to get this addressed.

Either not checking autoblocks, or not showing the autoblocked user when this is an xff hit are both easy solutions to implement. Let me check with some Stewwards and Ombudsmen to get their opinions.

Roan the issue with that is we have (relatively) few proxies on the trusted list, so it vastly reduces the IPs we can look at, even though a good many of them are accurate. I think that would pretty much negate what the Stewards are trying to accomplish with the feature.

In my opinion, just ignoring autoblocks for non-trusted headers won't be hugely harmful. If the vandal was using a proxy in the first place, then it's the proxy that will get hit by the autoblock, not the real IP. So the "abuse" scenario is:

1. Vance connects with their own IP
2. Vance is blocked, triggering the autoblock on their own IP
3. Before the autoblock expires, Vance switches to a non-anonymizing proxy

So we're dealing with someone:

1. Not clever enough to connect through a proxy in the first place
2. Clever enough to start using a proxy once they are blocked
3. But not clever enough to use a proxy that doesn't add XFF headers
4. And impatient enough to not just wait for the autoblock to expire

My guess it that's a very small number of users, though I don't have any data of course.

But not clever enough to use a proxy that doesn't add XFF headers

Since Wikipedia is now https-only, I think the proxy can only add an XFF header if it acts as a man-in-the-middle, asking the user to accept its certificate, right? I don't know how proxies used by vandals typically look like, but it must be less common than in 2013 (at least, the default configuration for Squid is to just pass https traffic through so no header is added).

Patch against master to not apply autoblocks against untrusted XFF headers (generated via git format-patch and can be applied via git am). The backport to 1.36 is unfortunately not as trivial as cherry-picking, so if this patch passes code review I'll work on that backport as well as the 1.35 backport.

Edit since Phab is derpy, here's the contents of the above .patch file in a code block for easier review:

From 335a9e12ef424c7f4893c8238d9fc9116eb0d73f Mon Sep 17 00:00:00 2001
From: Ryan Schmidt <skizzerz@skizzerz.net>
Date: Mon, 21 Jun 2021 19:09:22 -0700
Subject: [PATCH] Do not apply autoblocks to untrusted XFF headers

X-Forwarded-For is not necessarily trustworthy and can specify multiple
IP addresses in a single header, all of which are checked for blocks.
When a user is autoblocked, the wiki will create an IP block
behind-the-scenes for that user without exposing the user's IP on-wiki.
However, spoofing XFF would let an attacker guess at the IPs of users
who have active autoblocks, since the block message includes the
username of the original block target.

We still want to apply autoblocks to XFF when it comes from a trusted
proxy server so that autoblocks still work on wikis with reverse proxies
in front of them. However, don't allow potentially-spoofed XFF through
to close up this privacy loophole.

Bug: T285159
Change-Id: I7f4d9b078d6f7692a8d399dcaf07b0ddf812a333
---
 includes/ServiceWiring.php      |  1 +
 includes/block/BlockManager.php | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 1638096941..3fe0654338 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -232,6 +232,7 @@ return [
 			),
 			$services->getPermissionManager(),
 			$services->getUserFactory(),
+			$services->getProxyLookup(),
 			LoggerFactory::getInstance( 'BlockManager' ),
 			$services->getHookContainer()
 		);
diff --git a/includes/block/BlockManager.php b/includes/block/BlockManager.php
index 119a704cae..b3360287ec 100644
--- a/includes/block/BlockManager.php
+++ b/includes/block/BlockManager.php
@@ -31,6 +31,7 @@ use MediaWiki\User\UserFactory;
 use MediaWiki\User\UserIdentity;
 use Message;
 use MWCryptHash;
+use ProxyLookup;
 use Psr\Log\LoggerInterface;
 use User;
 use WebRequest;
@@ -51,6 +52,9 @@ class BlockManager {
 	/** @var UserFactory */
 	private $userFactory;
 
+	/** @var ProxyLookup */
+	private $proxyLookup;
+
 	/** @var ServiceOptions */
 	private $options;
 
@@ -79,6 +83,7 @@ class BlockManager {
 	 * @param ServiceOptions $options
 	 * @param PermissionManager $permissionManager
 	 * @param UserFactory $userFactory
+	 * @param ProxyLookup $proxyLookup
 	 * @param LoggerInterface $logger
 	 * @param HookContainer $hookContainer
 	 */
@@ -86,6 +91,7 @@ class BlockManager {
 		ServiceOptions $options,
 		PermissionManager $permissionManager,
 		UserFactory $userFactory,
+		ProxyLookup $proxyLookup,
 		LoggerInterface $logger,
 		HookContainer $hookContainer
 	) {
@@ -93,6 +99,7 @@ class BlockManager {
 		$this->options = $options;
 		$this->permissionManager = $permissionManager;
 		$this->userFactory = $userFactory;
+		$this->proxyLookup = $proxyLookup;
 		$this->logger = $logger;
 		$this->hookRunner = new HookRunner( $hookContainer );
 	}
@@ -255,6 +262,14 @@ class BlockManager {
 			$xff = array_diff( $xff, [ $ip ] );
 			// TODO: remove dependency on DatabaseBlock (T221075)
 			$xffblocks = DatabaseBlock::getBlocksForIPList( $xff, $isAnon, $fromPrimary );
+
+			// Exclude autoblocks from non-trusted XFF headers (T285159)
+			if ( !$this->proxyLookup->isTrustedProxy( $ip ) ) {
+				$xffblocks = array_filter( $xffblocks, static function ( $block ) {
+					return $block->getType() !== Block::TYPE_AUTO;
+				} );
+			}
+
 			$blocks = array_merge( $blocks, $xffblocks );
 		}
 	}
-- 
2.31.1.windows.1

-1 to the patch above. BlockManager is also constructed in BlockManagerTest and that also needs to be updated with this new parameter, or when this patch is published it won't pass tests. BlockManagerTest::getBlockManagerConstructorArgs() can just be updated to include a ProxyLookup configured correctly.

+1 to the approach used, -1 to the patch. In addition to what Danny said, please prefix the commit message with "[SECURITY]".

+			// Exclude autoblocks from non-trusted XFF headers (T285159)
+			if ( !$this->proxyLookup->isTrustedProxy( $ip ) ) {
+				$xffblocks = array_filter( $xffblocks, static function ( $block ) {
+					return $block->getType() !== Block::TYPE_AUTO;
+				} );
+			}

Can you expand this comment to explain the why? "...to avoid allowing people using spoofed XFF headers to leak autoblocks, revealing users IPs".

Do all the "trusted" proxies strip out existing XFF headers? I thought they usually just append another address.

Let Bob's IP be 1.2.3.4
Let the trusted proxy's IP be 42.42.42.42

Bob sends a request through the the proxy with:

X-Forwarded-For: 5.6.7.8, 5.6.7.9, 5.6.7.10

Now the proxy will change that to:

X-Forwdarded-For: 5.6.7.8, 5.6.7.9, 5.6.7.10, 1.2.3.4

$ip is 42.42.42.42, a trusted proxy. So it's fine to check 1.2.3.4, autoblock or not. But not the other addresses; those were spoofed by Bob.

New set of patches attached, including backports to 1.36, 1.35, and 1.31. For the 1.31 backport I changed the static anonymous function into a regular one to match how every other anonymous function in 1.31 is presented. 1.31 supports PHP 7.0.0+ and I believe static anonymous functions have existed long before then, but I felt it better to be safe and do what everything else does on that branch.

This incorporates @suffusion_of_yellow's feedback as well, as even if the XFF came from a trusted proxy, it does not necessarily mean the XFF header itself is 100% trustworthy. Other code in MediaWiki does not assume complete trustworthiness of XFF (https://gerrit.wikimedia.org/g/mediawiki/core/+/b1b6afd53f4e3c8e6d96f5a417f2b6baa9677941/includes/WebRequest.php#1281) so this shouldn't either. The resolution I went for in the security patch is to ignore XFF entirely for autoblocks rather than attempt to replicate the logic found in WebRequest.php. A later patch could re-introduce autoblock checking for the trusted portion of XFF headers if desired.

Is this resolved? Can we close? Either way, removing Traffic as I don't think this has much to do with Traffic-layer handing of XFF.

In T285159#7413304, @BBlack wrote:

Is this resolved? Can we close? Either way, removing Traffic as I don't think this has much to do with Traffic-layer handing of XFF.

The Security-Team still needs to review the patches from T285159#7171522 and deploy them, if necessary. But no, I don't believe we need any further input from Traffic unless they'd like to assist with CR of the aforementioned patches.

Can we get a review of the patches in T285159#7171522 by Traffic so the patch can be deployed by the security team?

Hi, asking again if the patches that are in this ticket can be reviewed? @Skizzerz perhaps you can recommend someone to take a second look?

Probably AHT is the "ideal" team to review this, but I already had looked over this earlier so +2 to the approach and patch in T285159#7171522 (just master, didn't check the backports).

@Legoktm - thanks for the +2. The master patch didn't apply to wmf/1.38.0-wmf.9 for me due to a line number issue. So I did a -3 and fixed the conflicts in this new patch. I assume the +2 is still valid re: the actual functionality of the patch.

In T285159#7553567, @sbassett wrote:

@Legoktm - thanks for the +2. The master patch didn't apply to wmf/1.38.0-wmf.9 for me due to a line number issue. So I did a -3 and fixed the conflicts in this new patch. I assume the +2 is still valid re: the actual functionality of the patch.

01-T285159.patch1 KBDownload

Yep, lgtm. And I just double checked that Block::getType() and Block::TYPE_AUTO still exist in master and haven't been refactored away yet :)

The patch above still applied to current master, so I tried to deploy it and it failed scap's canary checks - see below. It's off of deployment and removed from /srv/patches and /srv/mediawiki-staging/php-1.39.0-wmf.4/ for now.

21:21:59 Check 'Logstash Error rate for mw1414.eqiad.wmnet' failed: ERROR: 94% OVER_THRESHOLD (Avg. Error rate: Before: 0.28, After: 54.00, Threshold: 2.80)

21:21:59 Check 'Logstash Error rate for mw1417.eqiad.wmnet' failed: ERROR: 98% OVER_THRESHOLD (Avg. Error rate: Before: 0.00, After: 66.50, Threshold: 1.00)

21:21:59 Check 'Logstash Error rate for mw1449.eqiad.wmnet' failed: ERROR: 99% OVER_THRESHOLD (Avg. Error rate: Before: 0.03, After: 148.50, Threshold: 1.00)

...

RuntimeError: scap failed: average error rate on 9/9 canaries increased by 10x (rerun with --force to override this check, see https://logstash.wikimedia.org for details)
21:21:59 sync-file failed: <RuntimeError> scap failed: average error rate on 9/9 canaries increased by 10x (rerun with --force to override this check, see https://logstash.wikimedia.org for details)

Relevant logs. Does not like $blocks = array_merge( $blocks, $xffblocks );.

Bumping this. It's been 18 months now. If this were being exploited in the wild, would anyone even know?

In T285159#8505738, @suffusion_of_yellow wrote:

If this were being exploited in the wild, would anyone even know?

Likely no, as we don't have much in the way of proactive incident monitoring for stuff like this that I'm aware of. Typically we handle incident response in a fairly reactive fashion.

Looking at the patch again from T285159#7553567, it still applies to current mw core master, so it might just be an issue of variable init/guarding for $blocks = array_merge( $blocks, $xffblocks );., as I believe that type-errored, generating the errors I mentioned in T285159#7812740, which prevented a successful deployment. So maybe something like:

if( ! isset( $blocks ) || ! is_array( $blocks ) ) {
  $blocks = [];
}

just prior to $blocks = array_merge( $blocks, $xffblocks );.? Or maybe just initializing $blocks = []; earlier on as it doesn't seem to be coming from... anywhere. Admittedly, I don't completely know the intentions of the original patch writer, but that doesn't look correct as written and php definitely didn't like it. If anyone would like to provide some additional CR here, I'd be more than happy to deploy an updated patch during the next security deployment window.

Actually, I'm not sure why we can't just replace $blocks = array_merge( $blocks, $xffblocks ); with return $xffblocks;. I might be missing something, but $blocks = array_merge( $blocks, $xffblocks ); looks like a mistake or copypasta issue, as $blocks is seemingly undefined in this scope and nothing is even being returned after that assignment as the current code does. Proposed updated patch:

Haven't tested it, but nothing obviously wrong with the latest patch. I don't claim to understand the blocking code all that well though.

In T285159#8505829, @sbassett wrote:

Actually, I'm not sure why we can't just replace $blocks = array_merge( $blocks, $xffblocks ); with return $xffblocks;. I might be missing something, but $blocks = array_merge( $blocks, $xffblocks ); looks like a mistake or copypasta issue, as $blocks is seemingly undefined in this scope and nothing is even being returned after that assignment as the current code does.

That code was outdated due to https://gerrit.wikimedia.org/r/c/mediawiki/core/+/730029.

Proposed updated patch:

01-T285159-rev3.patch1 KBDownload

+2

In T285159#8538205, @Zabe wrote:

01-T285159-rev3.patch1 KBDownload

+2

I'll try to get this deployed during next week's security window.

deployed to prod, see SAL here - https://sal.toolforge.org/log/bpXC4IUBa_6PSCT9drYj

In T285159#8538205, @Zabe wrote:

That code was outdated due to https://gerrit.wikimedia.org/r/c/mediawiki/core/+/730029.

That's in REL1_38+... So presumably the old patch is still needed for REL1_35?

Change 904634 had a related patch set uploaded (by Reedy; author: Skizzerz):

[mediawiki/core@REL1_35] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904637 had a related patch set uploaded (by Reedy; author: Skizzerz):

[mediawiki/core@REL1_40] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904638 had a related patch set uploaded (by Reedy; author: Skizzerz):

[mediawiki/core@master] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904639 had a related patch set uploaded (by Reedy; author: Skizzerz):

[mediawiki/core@REL1_38] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904642 had a related patch set uploaded (by Reedy; author: Skizzerz):

[mediawiki/core@REL1_39] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904642 merged by jenkins-bot:

[mediawiki/core@REL1_39] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904639 merged by jenkins-bot:

[mediawiki/core@REL1_38] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904637 merged by jenkins-bot:

[mediawiki/core@REL1_40] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904634 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: Do not apply autoblocks to untrusted XFF headers

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

Change 904638 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Do not apply autoblocks to untrusted XFF headers

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