Hey @TJones, @pfischer - let us know when there's a patch ready for this issue and, assuming it's for ext:CirrusSearch, we'll want to review it on this task and get it into Wikimedia production via the security deployment process. Thanks.

Unsubbing myself. I just moved the ticket on the workboard during triage.

Looking over time out errors from the cirrus updater perspective I captured 461 unique page ids.
I'm not entirely sure what to do, some of these pages eventually parse successfully but I suspect they are slow enough to be close the 60sec excimer timeout.
@Dreamy_Jazz regarding the dos vector, is there something different that action=parse is doing but not the cirrus API?

In T401220#11086502, @dcausse wrote:

Looking over time out errors from the cirrus updater perspective I captured 461 unique page ids.
I'm not entirely sure what to do, some of these pages eventually parse successfully but I suspect they are slow enough to be close the 60sec excimer timeout.
@Dreamy_Jazz regarding the dos vector, is there something different that action=parse is doing but not the cirrus API?

Sorry just saw that ApiParse is protected via the PoolCounter, would you consider this a valid protection if we did the same in the cirrus API? (cc @sbassett)

Applying PoolCounter should address the immediate concern.

I filed it as a DoS concern because I didnt see what would prevent a user from very quickly loading the API multiple times.

I dont have enough context on this code to say for sure what the best solution is, so feel free to use your best judgement on what to do here.

+1 for PoolCounter as a means of addressing the potential Vuln-DoS here, especially if there isn't another obvious means of optimizing the cirrusbuilddoc query API's performance.

In T401220#11086616, @sbassett wrote:

+1 for PoolCounter as a means of addressing the potential Vuln-DoS here, especially if there isn't another obvious means of optimizing the cirrusbuilddoc query API's performance.

Thanks!

I don't think we can do much about much about perf, I think this is a known problem that some pages are extremely costly to parse.

I'll get a patch ready but wanted to confirm something with you.
This API is marked internal and not meant to be used by external users. The bulk of it comes from the cirrus updater process that talks to the mw-api-int k8s cluster, this process connects to the cluster with a dedicated user sets in PrivateSettings.php, I'm tempted to add a new setting in CirrusSearch to let such user bypass the poolcounter protection by doing something like:

		if ( $this->getUser()->getName() === $engine->getConfig()->get( "CirrusSearchStreamingUpdaterUsername" ) ) {
			// Bypass poolcounter protection for the internal cirrus user
			$this->doExecute( $engine );
		} else {
			// Protect against too many concurrent requests
			// Use a global key, this API is internal and could only be useful for manual debugging purposes
			// so no real need to have it on a per user basis.
			$worker = new PoolCounterWorkViaCallback( 'CirrusSearch-QueryBuildDocument', 'QueryBuildDocument',
				[
					'doWork' => function () use ( $engine ) {
						return $this->doExecute( $engine);
					},
					'error' => function () {
						$this->dieWithError( 'apierror-concurrency-limit' );
					},
				]
			);
			$worker->execute();
		}

I'd set $wgCirrusSearchStreamingUpdaterUsername = 'CirrusSearch Streaming Updater' in PrivateSettings.php, not necessarily because it's secret (it's public: http://meta.wikimedia.org/wiki/User:CirrusSearch_Streaming_Updater) but rather for clarity so that it's defined closer to where the user secrets are defined.

Does this sound sane to you? (cc @EBernhardson )

Semi-related, eventhough the action=parse API is using the pool counter I don't seem to find where we actually configured this pool (not seeing this T243803#6043986 config bits in our codebase), so I wonder if we should do this as well.

I'm definitely not a subject matter expert on CirrusSearch, but this at least seems reasonable as config in PS.php. You might also want to ask @CDanis et al if they have any concerns about this.

Patch for CirrusSearch:

I will need a change to the mw-config after applying this patch to actually enable the protection:

Pinging @EBernhardson for review, thanks!

Patches look reasonable to me, this should address the issue.

In T401220#11103444, @EBernhardson wrote:

Patches look reasonable to me, this should address the issue.

Thanks, both. Security-Team can security-deploy the ext:CirrusSearch patch tomorrow or by next Monday's security deployment window, at the latest. The config patch will need to go out publicly as a standard config-deploy after that. Once those are both out, it would be great if someone from the search team could confirm the patches are behaving as expected in Wikimedia production.

In T401220#11103451, @sbassett wrote:

In T401220#11103444, @EBernhardson wrote:

Patches look reasonable to me, this should address the issue.

Thanks, both. Security-Team can security-deploy the ext:CirrusSearch patch tomorrow or by next Monday's security deployment window, at the latest. The config patch will need to go out publicly as a standard config-deploy after that. Once those are both out, it would be great if someone from the search team could confirm the patches are behaving as expected in Wikimedia production.

Sure, please let us know when you plan to do the deploy and I'll try to be around, thanks.

In T401220#11099817, @dcausse wrote:

Patch for CirrusSearch:

T401220.patch4 KBDownload

I will need a change to the mw-config after applying this patch to actually enable the protection:

T401220-2.patch1 KBDownload

Pinging @EBernhardson for review, thanks!

CirrusSearch patch deployed, mw-config patch to go out later this week

Thanks for the deploy!
Unfortunately the config deploy did no go as expected, I realized that my approach depends on the NetworkSession extension being enabled everywhere but unfortunately this was disabled on all public wikis in T373826.
I'm going to try re-enabling it everywhere and see.

If the above approach is a no-go another way could be to use the mw-config ClusterConfig class and add a new trait to detect if the server is running in the kube-mw-api-int cluster, patch:

In T401220#11118482, @dcausse wrote:

If the above approach is a no-go another way could be to use the mw-config ClusterConfig class and add a new trait to detect if the server is running in the kube-mw-api-int cluster, patch:

T401220-3.patch3 KBDownload

I think Security-Team would probably prefer this approach since it doesn't require re-enabling a possibly still problematic extension.

Discussed with @Clement_Goubert about this and they pointed out that the rest-gateway might be hitting api-int from external requests but it does not hit the action API for now so this is safe. Clément suggested that it would not hurt to use both approaches and I agree, I adapted the patch with this idea:

In T401220#11122589, @dcausse wrote:

Discussed with @Clement_Goubert about this and they pointed out that the rest-gateway might be hitting api-int from external requests but it does not hit the action API for now so this is safe. Clément suggested that it would not hurt to use both approaches and I agree, I adapted the patch with this idea:

T401220-3.patch4 KBDownload

Patch makes sense to me on its face, but I'm admittedly not a domain expert on this code. If its basically just enabling PoolCounter for the cirrusbuilddoc query based upon certain conditions, that sounds fine. Since this is part of wmf-config, I'd imagine we'd just config-deploy the patch via gerrit during a security deployment or backport window.

@sbassett I finally deployed the config patch, sorry for the delay. I could trigger the poolcounter protection using apache ab with a concurrency at 15.

In T401220#11135649, @dcausse wrote:

@sbassett I finally deployed the config patch, sorry for the delay. I could trigger the poolcounter protection using apache ab with a concurrency at 15.

Excellent, thanks! I'd note that we are tracking this issue for the next supplemental security release (T397776), due out later this month. Some of the patches here seem fairly Wikimedia-specific though? I think we would defer to the Search Platform team to better understand what might make the most sense to release for ext:CirrusSearch, within the context of a supplemental security release targeted mainly towards external operators of MediaWiki.

I think it's indeed very WMF specific, unless we strongly suggest third parties to install the PoolCounter extension to protect all mediawiki endpoints allowing a parse I'm not totally sure that it deserves any special treatment because it's imo no more dangerous than a default MW install without any poolcounter set. Would it be OK to just ship the patch to master and ignore it from the security release?

In T401220#11140236, @dcausse wrote:

Would it be OK to just ship the patch to master and ignore it from the security release?

I would probably check in with @Reedy to see what they believe would be appropriate in this situation. The ext:CirrusSearch patch is simple to release. It's not bundled and would be a part of the upcoming supplemental release. The rest of the code implicates core and the likely not-commonly-used NetworkSession extension, and is the more Wikimedia-specific config, it seems.

Patch is blocking this week's MediaWiki train!

Patch 01-T401220.patch is currently failing to apply for version 1.45.0-wmf.19 of extensions/CirrusSearch. MW train cannot move forward until the patch is fixed (T396380)
Please note you can disregard any existing previous messages in this task from SecurityPatchBot concerning version 1.45.0-wmf.19. To unblock the train, run one of the commands in this message

If the patch needs to be rebased

A new version of the patch can be placed at the right location in the deployment server with the following Scap command:

REVISED_PATCH=<path_to_revised_patch>
scap update-patch --message-body 'Rebase to solve merge conflicts' /srv/patches/1.45.0-wmf.19/extensions/CirrusSearch/01-T401220.patch "$REVISED_PATCH"

If the patch has been made public

The patch can be dropped in the deployment server with the following Scap command:

scap remove-patch --message-body 'Dropping patch already made public' /srv/patches/1.45.0-wmf.19/extensions/CirrusSearch/01-T401220.patch

In T401220#11183588, @dcausse wrote:

Just updated the patch

Thanks!

Change #1189195 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] SECURITY: protect cirrusbuilddoc via PoolCounter

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

Change #1189223 had a related patch set uploaded (by Reedy; author: DCausse):

[mediawiki/extensions/CirrusSearch@REL1_44] SECURITY: protect cirrusbuilddoc via PoolCounter

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

Change #1189225 had a related patch set uploaded (by Reedy; author: DCausse):

[mediawiki/extensions/CirrusSearch@REL1_43] SECURITY: protect cirrusbuilddoc via PoolCounter

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

Change #1189223 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@REL1_44] SECURITY: protect cirrusbuilddoc via PoolCounter

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

Change #1189225 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@REL1_43] SECURITY: protect cirrusbuilddoc via PoolCounter

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