Page MenuHomePhabricator

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

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.

Event Timeline

Suggested fix:

1From 0cd5bf5623c40bf22be7b2309be504ee118b9674 Mon Sep 17 00:00:00 2001
2From: Lucas Werkmeister <mail@lucaswerkmeister.de>
3Date: Mon, 17 Dec 2018 14:02:39 +0100
4Subject: [PATCH] Fix cache mode for (un)patrolled recent changes query
5
6Restricting the list of recent changes to patrolled, not patrolled,
7autopatrolled, not autopatrolled, or unpatrolled recent changes requires
8special permissions (as does displaying that status in the properties of
9returned entries), but we only set the cache mode to private in the
10first two cases.
11
12Bug: T212118
13Change-Id: I4c3fe6e47f80ebf97fa37875c704328d08772d26
14---
15 includes/api/ApiQueryRecentChanges.php | 25 +++++++++++++++----------
16 1 file changed, 15 insertions(+), 10 deletions(-)
17
18diff --git a/includes/api/ApiQueryRecentChanges.php b/includes/api/ApiQueryRecentChanges.php
19index 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--
642.19.1
65

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:

1From e7ce9090b580ecc9b9a4d84dc5dbfa072683fe65 Mon Sep 17 00:00:00 2001
2From: Lucas Werkmeister <mail@lucaswerkmeister.de>
3Date: Mon, 17 Dec 2018 14:02:39 +0100
4Subject: [PATCH] Fix cache mode for (un)patrolled recent changes query
5
6Restricting the list of recent changes to patrolled, not patrolled,
7autopatrolled, not autopatrolled, or unpatrolled recent changes requires
8special permissions (as does displaying that status in the properties of
9returned entries), but we only set the cache mode to private in the
10first two cases.
11
12Bug: T212118
13Change-Id: I4c3fe6e47f80ebf97fa37875c704328d08772d26
14---
15 includes/api/ApiQueryRecentChanges.php | 28 ++++++++++++++++----------
16 1 file changed, 17 insertions(+), 11 deletions(-)
17
18diff --git a/includes/api/ApiQueryRecentChanges.php b/includes/api/ApiQueryRecentChanges.php
19index 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--
662.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.

Good point, done:

1From e7ce9090b580ecc9b9a4d84dc5dbfa072683fe65 Mon Sep 17 00:00:00 2001
2From: Lucas Werkmeister <mail@lucaswerkmeister.de>
3Date: Mon, 17 Dec 2018 14:02:39 +0100
4Subject: [PATCH] Fix cache mode for (un)patrolled recent changes query
5
6Restricting the list of recent changes to patrolled, not patrolled,
7autopatrolled, not autopatrolled, or unpatrolled recent changes requires
8special permissions (as does displaying that status in the properties of
9returned entries), but we only set the cache mode to private in the
10first two cases.
11
12Bug: T212118
13Change-Id: I4c3fe6e47f80ebf97fa37875c704328d08772d26
14---
15 includes/api/ApiQueryRecentChanges.php | 28 ++++++++++++++++----------
16 1 file changed, 17 insertions(+), 11 deletions(-)
17
18diff --git a/includes/api/ApiQueryRecentChanges.php b/includes/api/ApiQueryRecentChanges.php
19index 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--
662.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.

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

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.

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

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?

Ladsgroup added a subscriber: Reedy.

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.

@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.

Closing for ease of auditing from parent task

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

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

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

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