API responses for unpatrolled or (not) autopatrolled recent changes require privileges but may be cached publicly
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	LucasWerkmeister
	Dec 17 2018, 1:02 PM

Description

I just noticed a discrepancy in ApiQueryRecentChanges.php. Since the unpatrolled flag is equivalent to !patrolled (I think? they both add rc_patrolled = 0 to the query AFAICT), filtering for such entries requires appropriate permissions:

// Check permissions                                                                                                                                                              
if ( isset( $show['patrolled'] )
    || isset( $show['!patrolled'] )
    || isset( $show['unpatrolled'] )
    || isset( $show['autopatrolled'] )
    || isset( $show['!autopatrolled'] )
) {
    if ( !$user->useRCPatrol() && !$user->useNPPatrol() ) {
        $this->dieWithError( 'apierror-permissiondenied-patrolflag', 'permissiondenied' );
    }
}

However, we fail to set the cache mode to private in this case:

public function getCacheMode( $params ) {
    if ( isset( $params['show'] ) ) {
        foreach ( $params['show'] as $show ) {
            if ( $show === 'patrolled' || $show === '!patrolled' ) {
                return 'private';
            }
        }
    }
    // unrelated...
}

It looks like this has been missing since the flag was introduced in I356a8625c7, hence CCing @Krinkle and @Anomie.

Likewise, the autopatrol flag added in If64ba8b845 is also missing, CC @Ladsgroup.

Details

Subject	Repo	Branch	Lines +/-
SECURITY: Fix cache mode for (un)patrolled recent changes query	mediawiki/core	REL1_33	+17 -12
SECURITY: Fix cache mode for (un)patrolled recent changes query	mediawiki/core	REL1_32	+17 -12
SECURITY: Fix cache mode for (un)patrolled recent changes query	mediawiki/core	REL1_31	+17 -12
SECURITY: Fix cache mode for (un)patrolled recent changes query	mediawiki/core	master	+17 -12
SECURITY: Fix cache mode for (un)patrolled recent changes query	mediawiki/core	REL1_30	+17 -10
SECURITY: Fix cache mode for (un)patrolled recent changes query	mediawiki/core	REL1_27	+17 -10

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Stalled	None	T302086 Set scap minimum python version to 3.7
Resolved	None	T247045 Migrate all of production metal and VMs to Buster or later
Resolved	akosiaris	T249724 Track and remove jessie based container images from production
Resolved	Jdforrester-WMF	T224908 Drop jessie testing support
Resolved	Jdforrester-WMF	T224907 Drop php55 testing support
Resolved	Reedy	T205039 Release MediaWiki 1.27.6/1.30.2/1.31.2/1.32.2
Resolved	Reedy	T205041 Tracking bug for 1.27.6/1.30.2/1.31.2/1.32.2 security release
Resolved	Lucas_Werkmeister_WMDE	T212118 API responses for unpatrolled or (not) autopatrolled recent changes require privileges but may be cached publicly

Event Timeline

LucasWerkmeister created this task.Dec 17 2018, 1:02 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 17 2018, 1:02 PM

LucasWerkmeister added projects: MediaWiki-Recent-changes, MediaWiki-Action-API.Dec 17 2018, 1:04 PM

Restricted Application added a project: Growth-Team. · View Herald TranscriptDec 17 2018, 1:04 PM

Suggested fix:

P7921 Fix for T212118 (PS1)

1	From 0cd5bf5623c40bf22be7b2309be504ee118b9674 Mon Sep 17 00:00:00 2001
2	From: Lucas Werkmeister <mail@lucaswerkmeister.de>
3	Date: Mon, 17 Dec 2018 14:02:39 +0100
4	Subject: [PATCH] Fix cache mode for (un)patrolled recent changes query
5
6	Restricting the list of recent changes to patrolled, not patrolled,
7	autopatrolled, not autopatrolled, or unpatrolled recent changes requires
8	special permissions (as does displaying that status in the properties of
9	returned entries), but we only set the cache mode to private in the
10	first two cases.
11
12	Bug: T212118
13	Change-Id: I4c3fe6e47f80ebf97fa37875c704328d08772d26
14	---
15	includes/api/ApiQueryRecentChanges.php \| 25 +++++++++++++++----------
16	1 file changed, 15 insertions(+), 10 deletions(-)
17
18	diff --git a/includes/api/ApiQueryRecentChanges.php b/includes/api/ApiQueryRecentChanges.php
19	index 7c6b4634e5..1c79af1802 100644
20	--- a/includes/api/ApiQueryRecentChanges.php
21	+++ b/includes/api/ApiQueryRecentChanges.php
22	@@ -214,12 +214,7 @@ public function run( $resultPageSet = null ) {
23	}
24
25	// Check permissions
26	- if ( isset( $show['patrolled'] )
27	- \|\| isset( $show['!patrolled'] )
28	- \|\| isset( $show['unpatrolled'] )
29	- \|\| isset( $show['autopatrolled'] )
30	- \|\| isset( $show['!autopatrolled'] )
31	- ) {
32	+ if ( $this->includesPatrollingFlags( $show ) ) {
33	if ( !$user->useRCPatrol() && !$user->useNPPatrol() ) {
34	$this->dieWithError( 'apierror-permissiondenied-patrolflag', 'permissiondenied' );
35	}
36	@@ -642,12 +637,22 @@ public function extractRowInfo( $row ) {
37	return $vals;
38	}
39
40	+ /**
41	+ * @param array $flagsArray flipped array (string flags are keys)
42	+ * @return bool
43	+ */
44	+ private function includesPatrollingFlags( array $flagsArray ) {
45	+ return isset( $flagsArray['patrolled'] ) \|\|
46	+ isset( $flagsArray['!patrolled'] ) \|\|
47	+ isset( $flagsArray['unpatrolled'] ) \|\|
48	+ isset( $flagsArray['autopatrolled'] ) \|\|
49	+ isset( $flagsArray['!autopatrolled'] );
50	+ }
51	+
52	public function getCacheMode( $params ) {
53	if ( isset( $params['show'] ) ) {
54	- foreach ( $params['show'] as $show ) {
55	- if ( $show === 'patrolled' \|\| $show === '!patrolled' ) {
56	- return 'private';
57	- }
58	+ if ( $this->includesPatrollingFlags( array_flip( $params['show'] ) ) ) {
59	+ return 'private';
60	}
61	}
62	if ( isset( $params['token'] ) ) {
63	--
64	2.19.1
65

Anomie moved this task from Unsorted to Needs Review on the MediaWiki-Action-API board.Dec 17 2018, 2:13 PM

In T212118#4827670, @LucasWerkmeister wrote:

Suggested fix:
P7921

Code-Review+1: Seems sane, haven't tested but looks straightforward enough.

You might combine the if added on line 58 into the one on line 53, i.e. do if ( X && Y ) { ... } rather than if ( X ) { if ( Y ) { ... } }.

Good point, done:

P7944 Fix for T212118 (PS2)

1	From e7ce9090b580ecc9b9a4d84dc5dbfa072683fe65 Mon Sep 17 00:00:00 2001
2	From: Lucas Werkmeister <mail@lucaswerkmeister.de>
3	Date: Mon, 17 Dec 2018 14:02:39 +0100
4	Subject: [PATCH] Fix cache mode for (un)patrolled recent changes query
5
6	Restricting the list of recent changes to patrolled, not patrolled,
7	autopatrolled, not autopatrolled, or unpatrolled recent changes requires
8	special permissions (as does displaying that status in the properties of
9	returned entries), but we only set the cache mode to private in the
10	first two cases.
11
12	Bug: T212118
13	Change-Id: I4c3fe6e47f80ebf97fa37875c704328d08772d26
14	---
15	includes/api/ApiQueryRecentChanges.php \| 28 ++++++++++++++++----------
16	1 file changed, 17 insertions(+), 11 deletions(-)
17
18	diff --git a/includes/api/ApiQueryRecentChanges.php b/includes/api/ApiQueryRecentChanges.php
19	index 7c6b4634e5..2ceeb3d604 100644
20	--- a/includes/api/ApiQueryRecentChanges.php
21	+++ b/includes/api/ApiQueryRecentChanges.php
22	@@ -214,12 +214,7 @@ public function run( $resultPageSet = null ) {
23	}
24
25	// Check permissions
26	- if ( isset( $show['patrolled'] )
27	- \|\| isset( $show['!patrolled'] )
28	- \|\| isset( $show['unpatrolled'] )
29	- \|\| isset( $show['autopatrolled'] )
30	- \|\| isset( $show['!autopatrolled'] )
31	- ) {
32	+ if ( $this->includesPatrollingFlags( $show ) ) {
33	if ( !$user->useRCPatrol() && !$user->useNPPatrol() ) {
34	$this->dieWithError( 'apierror-permissiondenied-patrolflag', 'permissiondenied' );
35	}
36	@@ -642,12 +637,23 @@ public function extractRowInfo( $row ) {
37	return $vals;
38	}
39
40	+ /**
41	+ * @param array $flagsArray flipped array (string flags are keys)
42	+ * @return bool
43	+ */
44	+ private function includesPatrollingFlags( array $flagsArray ) {
45	+ return isset( $flagsArray['patrolled'] ) \|\|
46	+ isset( $flagsArray['!patrolled'] ) \|\|
47	+ isset( $flagsArray['unpatrolled'] ) \|\|
48	+ isset( $flagsArray['autopatrolled'] ) \|\|
49	+ isset( $flagsArray['!autopatrolled'] );
50	+ }
51	+
52	public function getCacheMode( $params ) {
53	- if ( isset( $params['show'] ) ) {
54	- foreach ( $params['show'] as $show ) {
55	- if ( $show === 'patrolled' \|\| $show === '!patrolled' ) {
56	- return 'private';
57	- }
58	+ if ( isset( $params['show'] ) &&
59	+ $this->includesPatrollingFlags( array_flip( $params['show'] ) )
60	+ ) {
61	+ return 'private';
62	}
63	}
64	if ( isset( $params['token'] ) ) {
65	--
66	2.19.1
67

If it’s okay with you, I can try to deploy the fix in a few days (as @Lucas_Werkmeister_WMDE, where I have production access).

I wonder if we actually have to go through the full security fix process here (pre-release announcement, contact Red Hat for CVE, etc.). I haven’t yet been able to actually exploit this bug in any way: I seem to get a private cache mode no matter what I do, even though I can’t see in the code where it’s coming from.

I'm not sure if Wikimedia wikis ever return a public cache mode for logged-in users, I think we might cache in varnish and then always return private to the client.

Then, too, you have to specify at least one of maxage or smaxage to tell the API you want public caching, and if you're logged in you also need to specify uselang because the default (uselang=user) overrides 'public' to 'anon-public-user-private'.

Okay, good points. With https://test.wikidata.org/w/api.php?action=query&format=json&maxage=120&uselang=en&list=recentchanges&rcshow=unpatrolled, I get cache-control: s-maxage=0, max-age=120, public when logged in, but also x-cache-status: pass, which IIUC means that Varnish won’t cache the response. And when requesting the same URL anonymously, there’s still an error about the patrol right being required.

But if this depends on Wikimedia’s cache setup, and other installs may respect this cache-control: …, public header, then I guess this could be a security issue for them.

In T212118#4847025, @LucasWerkmeister wrote:

Good point, done:

P7944 Fix for T212118 (PS2)
1 From e7ce9090b580ecc9b9a4d84dc5dbfa072683fe65 Mon Sep 17 00:00:00 2001
2 From: Lucas Werkmeister <mail@lucaswerkmeister.de>
3 Date: Mon, 17 Dec 2018 14:02:39 +0100
4 Subject: [PATCH] Fix cache mode for (un)patrolled recent changes query
5
6 Restricting the list of recent changes to patrolled, not patrolled,
7 autopatrolled, not autopatrolled, or unpatrolled recent changes requires
8 special permissions (as does displaying that status in the properties of
9 returned entries), but we only set the cache mode to private in the
10 first two cases.
11
12 Bug: T212118
13 Change-Id: I4c3fe6e47f80ebf97fa37875c704328d08772d26
14 ---
15 includes/api/ApiQueryRecentChanges.php | 28 ++++++++++++++++----------
16 1 file changed, 17 insertions(+), 11 deletions(-)
17
18 diff --git a/includes/api/ApiQueryRecentChanges.php b/includes/api/ApiQueryRecentChanges.php
19 index 7c6b4634e5..2ceeb3d604 100644
20 --- a/includes/api/ApiQueryRecentChanges.php
21 +++ b/includes/api/ApiQueryRecentChanges.php
22 @@ -214,12 +214,7 @@ public function run( $resultPageSet = null ) {
23 }
24
25 // Check permissions
26 - if ( isset( $show['patrolled'] )
27 - || isset( $show['!patrolled'] )
28 - || isset( $show['unpatrolled'] )
29 - || isset( $show['autopatrolled'] )
30 - || isset( $show['!autopatrolled'] )
31 - ) {
32 + if ( $this->includesPatrollingFlags( $show ) ) {
33 if ( !$user->useRCPatrol() && !$user->useNPPatrol() ) {
34 $this->dieWithError( 'apierror-permissiondenied-patrolflag', 'permissiondenied' );
35 }
36 @@ -642,12 +637,23 @@ public function extractRowInfo( $row ) {
37 return $vals;
38 }
39
40 + /**
41 + * @param array $flagsArray flipped array (string flags are keys)
42 + * @return bool
43 + */
44 + private function includesPatrollingFlags( array $flagsArray ) {
45 + return isset( $flagsArray['patrolled'] ) ||
46 + isset( $flagsArray['!patrolled'] ) ||
47 + isset( $flagsArray['unpatrolled'] ) ||
48 + isset( $flagsArray['autopatrolled'] ) ||
49 + isset( $flagsArray['!autopatrolled'] );
50 + }
51 +
52 public function getCacheMode( $params ) {
53 - if ( isset( $params['show'] ) ) {
54 - foreach ( $params['show'] as $show ) {
55 - if ( $show === 'patrolled' || $show === '!patrolled' ) {
56 - return 'private';
57 - }
58 + if ( isset( $params['show'] ) &&
59 + $this->includesPatrollingFlags( array_flip( $params['show'] ) )
60 + ) {
61 + return 'private';
62 }
63 }
64 if ( isset( $params['token'] ) ) {
65 --
66 2.19.1
67

Can you upload this as .patch file attached to this task? I'm not sure what visibility restrictions you set, but I can't see that paste.

In T212118#4885078, @LucasWerkmeister wrote:

I wonder if we actually have to go through the full security fix process here (pre-release announcement, contact Red Hat for CVE, etc.). I haven’t yet been able to actually exploit this bug in any way: I seem to get a private cache mode no matter what I do, even though I can’t see in the code where it’s coming from.

No, just steps 1-3. The security team handles doing the release. I'll clarify on the wiki page.

Can you upload this as .patch file attached to this task?

Does this work? (The paste is only visible to the people I explicitly listed as subscribers, and I wasn’t sure who’d need to see it. I didn’t know about the option to attach the patch to this task directly.)

T212118.patch2 KBDownload

No, just steps 1-3. The security team handles doing the release. I'll clarify on the wiki page.

Okay, thanks :) if it’s more typical to coalesce several security patches into one release, I feel less bad about bothering folks with this (IMO) not really critical bug.

LucasWerkmeister added a subscriber: Lucas_Werkmeister_WMDE.Jan 22 2019, 11:43 AM

Lucas_Werkmeister_WMDE claimed this task.Jan 22 2019, 11:43 AM

Sorry, that patch wasn’t even syntactically valid and I only noticed it on mwdebug1002 :( here’s a new one.

T212118.patch2 KBDownload

Unassigning my work account for now since I don’t intend to deploy this in the immediate future.

Manually copying the SAL entry:

Mentioned in SAL (#wikimedia-operations) [2019-01-22T12:02:10Z] <Lucas_WMDE> tried and failed to deploy patch for T212118

Let’s try again. I’ve tested the patch locally this time, but I’ll test it again on the debug server as well of course: without the patch, visiting this API URL from my private account returns Cache-Control: s-maxage=0, max-age=120, public. (My staff account is in the Wikidata staff group and can see deleted revisions, so it gets private cache mode due to that anyways.) With the patch, that should no longer be the case.

Deployed:

Mentioned in SAL (#wikimedia-operations) [2019-02-04T16:50:30Z] <Lucas_WMDE> !log deployed patch for T212118

Should we close this task now or leave it open until the patch has been released publicly?

In T212118#4924889, @LucasWerkmeister wrote:

Should we close this task now or leave it open until the patch has been released publicly?

Since it's part of core, it should stay open but as private and be released as a security release (with pre-release and all the fun). It usually handled by WMF security team. Since this might cause issues for third party installations. @Reedy did it last time IIRC.

Anomie moved this task from Needs Review to Pending deploy on the MediaWiki-Action-API board.Feb 4 2019, 5:44 PM

@Ladsgroup, @LucasWerkmeister - not sure when it will be released, but this should be the current tracking task for the next security release: T205041. I think the standard procedure is to create a subtask there for sec patches to be included.

Legoktm added a parent task: T205041: Tracking bug for 1.27.6/1.30.2/1.31.2/1.32.2 security release.Mar 8 2019, 9:23 PM

Legoktm mentioned this in T205048: Obtain CVEs for 1.27.6/1.30.2/1.31.2/1.32.2 security releases.Apr 7 2019, 11:48 PM

Closing for ease of auditing from parent task

Reedy mentioned this in T205041: Tracking bug for 1.27.6/1.30.2/1.31.2/1.32.2 security release.May 28 2019, 2:50 PM

Reedy mentioned this in T224499: RELEASE-NOTES for 1.27.6/1.30.2/1.31.2/1.32.2.May 29 2019, 12:32 AM

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 6 2019, 4:01 PM

Change 514766 had a related patch set uploaded (by Reedy; owner: Lucas Werkmeister):
[mediawiki/core@REL1_27] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

Change 514766 merged by Reedy:
[mediawiki/core@REL1_27] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

Change 514777 had a related patch set uploaded (by Reedy; owner: Lucas Werkmeister):
[mediawiki/core@REL1_30] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

Change 514777 merged by Reedy:
[mediawiki/core@REL1_30] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

ReleaseTaggerBot added projects: MW-1.27-release-notes, MW-1.30-release-notes.Jun 6 2019, 5:00 PM

Change 514853 had a related patch set uploaded (by Reedy; owner: Lucas Werkmeister):
[mediawiki/core@REL1_31] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

Change 514757 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.10; 2019-06-18).Jun 6 2019, 8:00 PM

Change 514953 had a related patch set uploaded (by Reedy; owner: Lucas Werkmeister):
[mediawiki/core@REL1_32] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

Change 514853 merged by jenkins-bot:
[mediawiki/core@REL1_31] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

ReleaseTaggerBot added a project: MW-1.31-release-notes.Jun 6 2019, 11:00 PM

Change 514977 had a related patch set uploaded (by Reedy; owner: Lucas Werkmeister):
[mediawiki/core@REL1_33] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

Change 514953 merged by jenkins-bot:
[mediawiki/core@REL1_32] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

Change 514977 merged by jenkins-bot:
[mediawiki/core@REL1_33] SECURITY: Fix cache mode for (un)patrolled recent changes query

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

ReleaseTaggerBot added projects: MW-1.33-notes, MW-1.32-notes.Jun 7 2019, 12:01 AM

sbassett moved this task from Pending deployment / release to Done on the acl*security board.Sep 26 2019, 2:28 PM

• chasemp added a project: Security.Feb 10 2020, 10:51 PM

• chasemp removed a project: acl*security.Feb 20 2020, 8:12 PM

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:38 PM

	F27989891: T212118.patch
	Jan 22 2019, 12:05 PM

	F27914999: T212118.patch
	Jan 16 2019, 9:50 PM

API responses for unpatrolled or (not) autopatrolled recent changes require privileges but may be cached publiclyClosed, ResolvedPublicActions

Description

Details

Related ObjectsSearch...

Event Timeline

API responses for unpatrolled or (not) autopatrolled recent changes require privileges but may be cached publicly
Closed, ResolvedPublic
Actions

Related Objects
Search...