VOOZH about

URL: https://phabricator.wikimedia.org/T187638

⇱ ⚓ T187638 When a log event is (partially) hidden Special:Redirect/logid can link to the incorrect log and reveal hidden information (CVE-2018-0504)

Maniphest T187638

When a log event is (partially) hidden Special:Redirect/logid can link to the incorrect log and reveal hidden information (CVE-2018-0504)
Closed, ResolvedPublic

Description

On the English Wikipedia links to https://en.wikipedia.org/w/index.php?title=Special%3ALog&limit=1&offset=20180216090941&type=rights&user=Oshwah, which shows

08:34, 16 February 2018 Oshwah (talk | contribs) changed group membership for Cyrus noto3at bulaga from extended confirmed user to extended confirmed user, pending changes reviewer and rollbacker (Granting pending changes reviewer and rollbacker. User is active in RC patrolling, reverting vandalism, and reviewing - would make good use of the tools.)

reveals the hidden user. The information may also be suppressed.

Linking to https://en.wikipedia.org/w/index.php?title=Special%3ALog&limit=1&offset=20180216090941&type=rights shows the correct log event.

09:09, 16 February 2018 (Username or IP removed) (log details removed) (Confirmed as recently active IP. Granting 'confirmed' flag manually.)
Log event from the API
{
 "batchcomplete": "",
 "query": {
 "logevents": [
 {
 "logid": 89126214,
 "actionhidden": "",
 "type": "rights",
 "action": "rights",
 "userhidden": "",
 "timestamp": "2018-02-16T09:09:40Z",
 "comment": "Confirmed as recently active IP. Granting 'confirmed' flag manually."
 }
 ]
 }
}
Log event hiding the above log event
{
 "batchcomplete": "",
 "continue": {
 "lecontinue": "20180214180709|89093360",
 "continue": "-||"
 },
 "query": {
 "logevents": [
 {
 "logid": 89126301,
 "ns": -1,
 "title": "Special:Log/rights",
 "pageid": 0,
 "logpage": 0,
 "params": {
 "type": "logging",
 "ids": [
 "89126214"
 ],
 "old": {
 "bitmask": 0
 },
 "new": {
 "bitmask": 5,
 "content": "",
 "user": ""
 }
 },
 "type": "delete",
 "action": "event",
 "user": "Oshwah",
 "timestamp": "2018-02-16T09:18:12Z",
 "comment": "[[WP:RD5|RD5]]: Other valid deletion under [[WP:DEL#REASON|deletion policy]]"
 }
 ]
 }
}

Special:Redirect/logid was implemented in rMW1b294bd3c538: Make Special:Redirect page redirect to log events by ID for T71107: Allow browsing log events by log ID.

Related Objects

Event Timeline

Bawolff subscribed.
Comment Actions

Apologies if this shouldn't be a Security task.

This definitely is a security task (leaking secret info)

Thank you for reporting this.

)

Bawolff added a project: Patch-For-Review.
Comment Actions

There's also an issue with Special:redirect/user if the user has been revdel'd

T187638.patch3 KBDownload
Comment Actions

Code-Review-1

+			'log_type' => LogPage::DELETED_ACTION,

We were discussing this on T188145, I don't think this is correct.

+		// get rid of log_id and log_deleted.
+		array_shift( $logparams );
 		array_shift( $logparams );

This should probably just use .

+				if ( !LogEventsList::userCan(
+					$rowMain, $fieldToBitsMapping[$cond], $user
+				) ) {
+					// This field is redacted for main log entry,
+					// so treat as always matching.
+					$matchedRows[] = $row;
+					continue;
+				}

This could go outside the inner loop, just assign and continue. If you change the code below to use instead of , you could even skip the assignment.

Also, won't this and the bits below generate undefined index warnings when trying to access for ?

Comment Actions

Thanks for the review. PS2:

T187638-v2.patch3 KBDownload
Comment Actions
- array_shift( $logparams );
+ // get rid of log_id and log_deleted.
+ unset( $logparams[0] );
+ unset( $logparams[1] );

Huh? $logparams doesn't have a [0] or [1] ... Oh, I see, this needs a rebase on top of rMW27c61fb1e94d: Add `actor` table and code to start using it.

+ if (
+ $logKey === 'log_user_text' &&
+ !LogEventsList::userCan(
+ $rowMain, LogPage::DELETED_USER, $user
+ )
+ ) {
+ continue;
+ }
 $query[$keys[$logKey]] = $matchedRows[0]->$logKey;

I'd prefer it if you used either or consistently here.

Comment Actions

Could this be fixed by removing the log look-up entirely from Special:Redirect and having a log-ID parameter to Special:Log? (Have done so in https://gerrit.wikimedia.org/r/#/c/428265/ for T191608).

Comment Actions

that would certainly simplify the code

Anomie added a subscriber: Umherirrender.
Comment Actions

Could this be fixed by removing the log look-up entirely from Special:Redirect and having a log-ID parameter to Special:Log? (Have done so in https://gerrit.wikimedia.org/r/#/c/428265/ for T191608).

Do we want to backport this patch then?

Reedy assigned this task to Samwilson.
Comment Actions

Closing as patches merged.

To be opened up to public with security release

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 20 2018, 9:34 PM
MoritzMuehlenhoff renamed this task from When a log event is (partially) hidden Special:Redirect/logid can link to the incorrect log and reveal hidden information to When a log event is (partially) hidden Special:Redirect/logid can link to the incorrect log and reveal hidden information (CVE-2018-0504).Sep 21 2018, 7:25 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits