Page MenuHomePhabricator

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 [[Special:Redirect/logid/89126214]] 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.)

&user=Oshwah 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.

Event Timeline

JJMC89 created this task.Feb 18 2018, 4:51 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 18 2018, 4:51 AM

Apologies if this shouldn't be a Security task.

Bawolff added a subscriber: Bawolff.

Apologies if this shouldn't be a Security task.

This definitely is a security task (leaking secret info)

Thank you for reporting this.

)

JJMC89 updated the task description. (Show Details)Feb 18 2018, 9:44 PM
Bawolff added a project: Patch-For-Review.

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

Anomie added a subscriber: Anomie.Feb 24 2018, 12:38 AM

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

+				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 $matchedRows = $logsSameTimestamps and continue. If you change the code below to use $mainRow instead of $matchedRows[0], you could even skip the assignment.

Also, won't this and the bits below generate undefined index warnings when trying to access $fieldToBitsMapping[$cond] for $cond === 'log_timestamp'?

Thanks for the review. PS2:

Anomie added a comment.Mar 5 2018, 6:12 PM
-               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 $rowMain or $matchedRows[0] consistently here.

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

that would certainly simplify the code

Anomie merged a task: Restricted Task.May 3 2018, 2:30 PM
Anomie added a subscriber: Umherirrender.
Reedy added a subscriber: Reedy.Jul 7 2018, 10:46 AM

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 closed this task as Resolved.Jul 7 2018, 5:29 PM
Reedy assigned this task to Samwilson.

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