Page MenuHomePhabricator

Exposed suppressed log in RevisionDelete page
Closed, ResolvedPublic

Description

Expose suppress log (Special:Log/suppress) by set logid by manually

Pre-requirements:

  • The wiki haves one or more tags for add or remove tags.

Step to reproduce:

  1. Logged in as user
  2. Go to Special:Log
  3. Check any entry
  4. Click [Edit tags of selected log entries] (MediaWiki:log-edit-tags) button
  5. Set logid as suppress log (e.g. increase or decrease logid by any logid)
  6. View the page

e.g. https://test.wikipedia.org/w/index.php?action=historysubmit&type=logging&revisiondelete=1&ids%5B224251%5D=1

Expected:
MUST prohibit access to the log

Original reporter : User:Ohgi

Related with T222036 - Case 2

Event Timeline

Rxy created this task.Apr 28 2019, 2:56 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 28 2019, 2:56 PM
Rxy triaged this task as Unbreak Now! priority.Apr 28 2019, 2:57 PM
Rxy updated the task description. (Show Details)
Rxy added a subscriber: Ohgi.
Rxy updated the task description. (Show Details)Apr 28 2019, 3:11 PM
Rxy added a project: Patch-For-Review.EditedApr 28 2019, 8:27 PM

This patch is security fix patch also covers T222036 Case 2 (must applied F28838873 T222036.patch too)

This patch is similar with F28839051 (T222038.patch), but additionally contains fixing to LogFormatter too (may overkill too).

Rxy moved this task from Backlog to Security on the User-Rxy board.
Reedy renamed this task from Expose suppressed log in RevisionDelete page to Exposed suppressed log in RevisionDelete page.Apr 29 2019, 3:00 PM
Reedy added a subscriber: Reedy.Apr 29 2019, 3:13 PM

What is the difference between this and case 2 in T222036?

Rxy added a comment.Apr 29 2019, 3:58 PM

What is the difference between this and case 2 in T222036?

I guess based trouble is same with T222036 Case2.

the task is wrote before I read or investigate codes. And I assumed to Case2 is specific to EditTags, but that assume is wrong and actually problem is LogEventsList::userCan() related.

Rxy updated the task description. (Show Details)Apr 30 2019, 4:07 PM

+1 on this patch

I do note some of the error messages are not great, but that was a pre-existing problem.

This patch is security fix patch also covers T222036 Case 2 (must applied F28838873 T222036.patch too)


This patch is similar with F28839051 (T222038.patch), but additionally contains fixing to LogFormatter too (may overkill too).

I think that's a reasonable hardening measure, but we should do that after the initial issue is fixed, and probably publicly.

T222038.patch was deployed to 1.34.0-wmf.1 and 1.34.0-wmf.3.

Chatting w/ @Bawolff and @Reedy, T222038-formatter.patch should just be pushed up publicly to gerrit.

sbassett closed this task as Resolved.Apr 30 2019, 9:53 PM
sbassett claimed this task.

Note: T222038-formatter.patch is now up on gerrit: https://gerrit.wikimedia.org/r/507870

sbassett added a comment.EditedMay 2 2019, 8:10 PM

Well, duh, r/507870 is failing CI builds because LogFormatter::canViewLogType() doesn't exist on master. Set WIP for now. Given that the sec patches are already deployed, I think this can probably just wait in gerrit until it can be merged.

Rxy added a comment.May 3 2019, 1:13 AM

Well, duh, r/507870 is failing CI builds because LogFormatter::canViewLogType() doesn't exist on master. Set WIP for now. Given that the sec patches are already deployed, I think this can probably just wait in gerrit until it can be merged.

It seems the gerrit patch is not same with F28839068 ...

Also I can't modify the gerrit patch due to recent permission changes

@Rxy - Apologies, the patch set should be fixed in gerrit now. I had manually modified F28839068 so as not to include the duplicate changes in F28839051 for includes/logging/LogEventsList.php, since that was already deployed as a security patch on Tuesday. Unfortunately, in doing so, I conflated userCanViewLogType and canViewLogType which, again, should now be resolved in the patch set.

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

Change 514768 had a related patch set uploaded (by Reedy; owner: Rxy):
[mediawiki/core@REL1_27] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514768 merged by Reedy:
[mediawiki/core@REL1_27] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514779 had a related patch set uploaded (by Reedy; owner: Rxy):
[mediawiki/core@REL1_30] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514779 merged by Reedy:
[mediawiki/core@REL1_30] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514855 had a related patch set uploaded (by Reedy; owner: Rxy):
[mediawiki/core@REL1_31] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514759 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514955 had a related patch set uploaded (by Reedy; owner: Rxy):
[mediawiki/core@REL1_32] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514979 had a related patch set uploaded (by Reedy; owner: Rxy):
[mediawiki/core@REL1_33] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514955 merged by jenkins-bot:
[mediawiki/core@REL1_32] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514979 merged by jenkins-bot:
[mediawiki/core@REL1_33] SECURITY: Add permission check for user is permitted to view the log type

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

Change 514855 merged by jenkins-bot:
[mediawiki/core@REL1_31] SECURITY: Add permission check for user is permitted to view the log type

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

Change 507870 merged by jenkins-bot:
[mediawiki/core@master] Add permission check for user is permitted to view the log type

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