Page MenuHomePhabricator

Suppressed edits remain examinable in AbuseFilter
Closed, ResolvedPublic

Description

If an oversighted contribution caused an AbuseFilter hit, the resulting log's examine function allows non-oversight access to the suppressed data.

Using https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467 as an example:

  • Logged in (non-oversight) users can view the old_wikitext/new_wikitext, and thus the oversighted material.
  • Logged out users cannot view the old_wikitext/new_wikitext but can view the summary, which in this case was oversighted.

(originally reported to oversight-l)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Legoktm set Security to Software security bug.Oct 15 2018, 8:05 PM
Legoktm added a project: Security.
Legoktm changed the visibility from "Public (No Login Required)" to "Custom Policy".

In the future please report these kinds of information leaks as security tasks.

Amorymeltzer triaged this task as Unbreak Now! priority.Oct 15 2018, 8:06 PM
Amorymeltzer added a project: Trust-and-Safety.

Sort of related to T44734,

Amorymeltzer updated the task description. (Show Details)Oct 15 2018, 8:26 PM
Samtar added a subscriber: Samtar.EditedOct 15 2018, 8:53 PM

Using https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467:

  • Logged in (non-oversight) users can view the old_wikitext/new_wikitext with the oversighted material.
  • Logged out users cannot view the old_wikitext/new_wikitext but can view the summary, which in this case was oversighted.
Keegan added a subscriber: Risker.Oct 15 2018, 10:15 PM
Rxy added a subscriber: Rxy.Oct 15 2018, 11:06 PM
Rxy added a comment.Oct 16 2018, 12:54 AM

I wrote the patch for this.

Rxy moved this task from Backlog to Security on the User-Rxy board.
Rxy added subscribers: Bawolff, Reedy, Anomie.
Legoktm added a subscriber: Huji.
Vituzzu added a subscriber: Vituzzu.

Thanks @Vituzzu for adding me. I received this task via mail so I already knew what this was about. However, as I understood it, it's invalid. There are two types of /examine interface, one for AbuseLog entries and one for RC. Oversighting the AbuseLog makes the former invisible to non-oversighters, and I just tested it on my wiki and BC to be sure. As for the second: it can only be accessed via search form on Special:AbuseFilter/examine, but it's perfectly normal that it remains visible: suppressing AbuseLog doesn't imply suppressing the related edit, and examinating RC uses as visibility the one for RC. So, if you want to hide /examine in this case, you have to oversight the actual edit. Again, to me this works as expected but I'd like to hear confirm before closing the task as invalid. Also, BTW, the patch above doesn't work (if I correctly understood the problem here).

lfaraone added a comment.EditedOct 16 2018, 8:38 PM

@Daimona, if you view the linked edits, you see that the actual edits and summaries are suppressed, but the contents of those edit summaries are visible via "examine" even if logged out.

Daimona renamed this task from Suppressed AbuseLog entries remains still public and examinable to Suppressed edits remain examinable in AbuseFilter.Oct 16 2018, 8:42 PM

Thanks @Vituzzu for adding me. I received this task via mail so I already knew what this was about. However, as I understood it, it's invalid. There are two types of /examine interface, one for AbuseLog entries and one for RC. Oversighting the AbuseLog makes the former invisible to non-oversighters, and I just tested it on my wiki and BC to be sure. As for the second: it can only be accessed via search form on Special:AbuseFilter/examine, but it's perfectly normal that it remains visible: suppressing AbuseLog doesn't imply suppressing the related edit, and examinating RC uses as visibility the one for RC. So, if you want to hide /examine in this case, you have to oversight the actual edit. Again, to me this works as expected but I'd like to hear confirm before closing the task as invalid. Also, BTW, the patch above doesn't work (if I correctly understood the problem here).

For clarity, you're saying that the fact https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467 's summary is visible to logged-out users means everything is "working as expected"?

@lfaraone Thanks, now I got it, but the title and description of the task were totally wrong. As for the patch above, now it makes sense and it does work. However, we should also add some notice to /examine, next to the "examine" link (like we currently do for suppressed AbuseLog entries). I'll try to see if I can improve the patch myself now.

Thanks @Vituzzu for adding me. I received this task via mail so I already knew what this was about. However, as I understood it, it's invalid. There are two types of /examine interface, one for AbuseLog entries and one for RC. Oversighting the AbuseLog makes the former invisible to non-oversighters, and I just tested it on my wiki and BC to be sure. As for the second: it can only be accessed via search form on Special:AbuseFilter/examine, but it's perfectly normal that it remains visible: suppressing AbuseLog doesn't imply suppressing the related edit, and examinating RC uses as visibility the one for RC. So, if you want to hide /examine in this case, you have to oversight the actual edit. Again, to me this works as expected but I'd like to hear confirm before closing the task as invalid. Also, BTW, the patch above doesn't work (if I correctly understood the problem here).

For clarity, you're saying that the fact https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467 's summary is visible to logged-out users means everything is "working as expected"?

No, I'm saying that the description of this task reports a problem, but the actual task is about a totally different one.

No, I'm saying that the description of this task reports a problem, but the actual task is about a totally different one.

Certainly didn't read as such - if it'd help, I can reword the task description.
Hopefully it goes without saying (but I'll say it, lest the task get closed as invalid) that this bug is exposing non-public personal information to all users - a breach of the privacy policy - and should be taken seriously.

Samtar updated the task description. (Show Details)Oct 16 2018, 8:58 PM

No, I'm saying that the description of this task reports a problem, but the actual task is about a totally different one.

Certainly didn't read as such - if it'd help, I can reword the task description.
Hopefully it goes without saying (but I'll say it, lest the task get closed as invalid) that this bug is exposing non-public personal information to all users - a breach of the privacy policy - and should be taken seriously.

It definitely is. I just didn't understand it at first. Thanks for rewording the description, I'm about to submitted an amended patch.

Daimona added a comment.EditedOct 16 2018, 9:06 PM

This is my patch, where the examine part is copied from Rxy's patch (and slightly beautified). Also, please note that this vulnerability also affects Special:AbuseFilter/test (fixed by the change in AbuseFilterChangesList)

(file was updated, it wouldn't have passed CI, and that's the last thing we need).

Also, we should probably make a better distinction between deleted and suppressed stuff, but it's fine to merge this for the moment.

Huji added a comment.Oct 16 2018, 10:04 PM

I like Daimona's patch better. I am going to test it momentarily; assuming that it checks out, what is our approach here? Do we have Daimona post it publicly, have me merge it within minutes, have Daimona create a cherry pick for WMF current build, have someone SWAT it ASAP, or do we try to minimize the amount of time things are public in some other way (e.g. I submit and merge the patch myself, or even, we have someone who can SWAT it submit and merge the patch in both places, based on my verbal approval here?)

Huji added a comment.Oct 16 2018, 11:17 PM

@Daimona: your patch checks out. But I also noticed a related issue:

  • User A suppresses an edit that triggered a filter
  • User B (which is a sysop and not an oversight) goes to Special:AbuseLog/123 to view that log
  • User B sees the text of edit and its summary, even though they were suppressed!

Do we want to address this at the same time?

Please pardon me if I'm being naïve, but isn't the underlying issue that the AbuseLog doesn't follow revdel actions? In a perfect world, wouldn't AbuseLog entries/examinations of successful actions respect the revdel/oversight status of that action? This need not wait for that, but am I wrong in thinking that's the ideal long-term solution?

I am going to test it momentarily; assuming that it checks out, what is our approach here? Do we have Daimona post it publicly, have me merge it within minutes, have Daimona create a cherry pick for WMF current build, have someone SWAT it ASAP, or do we try to minimize the amount of time things are public in some other way (e.g. I submit and merge the patch myself, or even, we have someone who can SWAT it submit and merge the patch in both places, based on my verbal approval here?)

A member from the security team will also take a look and secretly deploy it once ready, verify that it works, and then make it public shortly afterwards.

Rxy added a comment.Oct 17 2018, 2:01 AM

This is my patch, where the examine part is copied from Rxy's patch (and slightly beautified). Also, please note that this vulnerability also affects Special:AbuseFilter/test (fixed by the change in AbuseFilterChangesList)


(file was updated, it wouldn't have passed CI, and that's the last thing we need).
Also, we should probably make a better distinction between deleted and suppressed stuff, but it's fine to merge this for the moment.

Thanks for improve my patch :)

@Daimona: your patch checks out. But I also noticed a related issue:

  • User A suppresses an edit that triggered a filter
  • User B (which is a sysop and not an oversight) goes to Special:AbuseLog/123 to view that log
  • User B sees the text of edit and its summary, even though they were suppressed!

Do we want to address this at the same time?

That is related with "SpecialAbuseLog::isHidden".

rEABF /includes/special/SpecialAbuseLog.php ( L1106-1120 , L545)

These should be use "Revision::userCan" or "Revision::userCanBitfield".

Daimona added a comment.EditedOct 17 2018, 4:31 AM

@Huji I guess the last option is the best one (someone who can deploy will auto-merge and immediately deploy). As for the mentioned problem yes, we should fix it now; the only thing we can (shortly) delay is separately handle deleted and suppressed entries (in SpecialAbuseLog::isHidden and in the added lines in AbuseFilterChangesList, where we check for visibility != 0). @Rxy (or anyone else) of course feel free to amend my patch to solve it: I probably won't have time to do it today. @Amorymeltzer yes, indeed, but this is not related to the suppression of AbuseLog entries, as the original description was about.

PS I just realized that the problem reported by Huji is related to my comment about separatig deleted and suppressed revisions. Correctly handling them now would solve it as a bonus, so if someone has the time for it, I'd suggest to do it now (it shouldn't be hard).

This is an updated version which should also solve the discrepancy between deleted and suppressed revs. An important note: I haven't tested it! It should be tested in several cases: visible, deleted or suppressed revision, and basic/sysop/oversighter user.

Rxy added a comment.EditedOct 17 2018, 6:12 AM

This is an updated version which should also solve the discrepancy between deleted and suppressed revs. An important note: I haven't tested it! It should be tested in several cases: visible, deleted or suppressed revision, and basic/sysop/oversighter user.

CodeReview: -1

Thanks for update the patch. but I get error message in my wiki :

PHP Parse error:  syntax error, unexpected ';' in /w/extensions/AbuseFilter/includes/Views/AbuseFilterViewExamine.php on line 169


$ docker run -it -v "$PWD:/opt" --rm php:7.0 php -l
 /opt/includes/Views/AbuseFilterViewExamine.php

Parse error: syntax error, unexpected ';' in /opt/includes/Views/AbuseFilterViewExamine.php on line 169
Errors parsing /opt/includes/Views/AbuseFilterViewExamine.php


$ docker run -it -v "$PWD:/opt" --rm php:7.1 php -l
 /opt/includes/Views/AbuseFilterViewExamine.php

Parse error: syntax error, unexpected ';' in /opt/includes/Views/AbuseFilterViewExamine.php on line 169
Errors parsing /opt/includes/Views/AbuseFilterViewExamine.php


$ docker run -it -v "$PWD:/opt" --rm php:7.2 php -l /opt/includes/Views/AbuseFilterViewExamine.php

Parse error: syntax error, unexpected ';' in /opt/includes/Views/AbuseFilterViewExamine.php on line 169
Errors parsing /opt/includes/Views/AbuseFilterViewExamine.php

I tried L168
if ( is_int( SpecialAbuseLog::isHidden( $row ) ) {

to

if ( is_int( SpecialAbuseLog::isHidden( $row ) ) ) {
;

I tested that but not work correctly due to $rev->userCan first param is should be provide Revision::SUPPRESSED_ALL instead of "SpecialAbuseLog::isHidden".

because Revision::userCan is already called $this->getVisibility() in rMW /includes/Revision.php $1199

Daimona added a comment.EditedOct 17 2018, 1:41 PM

@Rxy thanks. I'm trying to include every possible case in my patch, but I don't really know how to use all of these deletion constants, so I'm going very slowly.

Also, I'm trying to fulfill the following conditions:

  1. Make it possible to use /examine according to revision visibility (the main point of the task)
  2. Do the same not only for edits, but also for log entries!
  3. Show the examine link if and only if the user can examine the given row
  4. If a revision is hidden but the user can see it, don't hide its elements (only leave them striked)
  5. Ensure that the fix is applied to APIs as well

For 3, we need to conditionally execute the return added in insertExtra, checking if the user can see the row. For 4, we need to add in AbuseFilterChangesList the methods insertUserRelatedLinks and insertComment, and add a case for when the revision is hidden but the user can see it. For 2, we surely need to add a log-specific elseif next to the added lines in ViewExamine. Also, SpecialAbuseLog::isHidden can keep returning 'implicit' for deleted revisions.


New version. Tested in all cases (anonymous/sysop/oversight user vs visible/deleted/oversighted element), and it seems to work.

Huji added a comment.Oct 17 2018, 10:56 PM

I still need an answer as to how we merge a patch like this.

Huji added a comment.EditedOct 18 2018, 12:26 AM

@Aklapper great direction, thank you!

Rxy added a subscriber: Y-dash.Oct 18 2018, 1:12 AM
Daimona added a comment.EditedOct 18 2018, 8:29 AM

I'm leaving here a note, which I'll investigate later today, and to also hear some thoughts. Maybe in ChangesList deleted/suppressed edits shouldn't be shown at all if the user can't see them (instead of showing a censored and greyed line): I think "normal" user may understand the performer and/or the target of the action even if it's hidden, by properly filtering on Special:AbuseFilter/examine and Special:AbuseFilter/test.

UPDATE: AFAICS, we don't do this for other RC lists, for instance the watchlist. So maybe we can keep them, huh?

Rxy added a comment.EditedOct 18 2018, 1:25 PM


New version. Tested in all cases (anonymous/sysop/oversight user vs visible/deleted/oversighted element), and it seems to work.

Thanks for working this. Almost ok for me. but why you pass $entry->getDeleted() in F26619762$123 ?

And I noticed added permission checks are redundant with ChangesList::userCan ( rMW / includes/changes/ChangesList.php$633-639 )

I submit Patch Set 5 in here:

@Rxy thanks for your fixes. I consequently amended my patch, which is below. Again, I tested every possible case, even more than I originally did (visible/deleted(a single field or all fields)/suppressed(a single field or all fields) edit/log with an anonymous/admin/oversighter account) and it should be fine. It also passes PHPCS.

Reedy added a comment.Oct 18 2018, 2:50 PM

What’s the difference between the two patches?

Which one do we want to proceed with?

@Reedy There's no difference, mine includes Rxy's one: Rxy submitted the first one, I checked it out and added other things. Then Rxy amended a point in my patch in his own version; then I did the same fix in my own file, plus a couple other fixes and re-published it as the last version above. So, unless I missed something, my last patch includes all we need.

Reedy added a comment.Oct 18 2018, 3:07 PM

Phab shows 1KB difference ;)

Reedy added a comment.Oct 18 2018, 3:08 PM
This comment was removed by Reedy.

@Reedy yeah, because the last Rxy's patch is just an amending of mine, and a proper subset of my last patch :-)

Reedy added a comment.Oct 18 2018, 4:47 PM

CR-1, there's a missing return in here. I guess this path wasn't tested?

+	/**
+	 * Insert a formatted comment. Like the parent, but don't hide details if user can see them.
+	 * @param RecentChange $rc
+	 * @return string
+	 */
+	public function insertComment( $rc ) {
+		if ( $this->isDeleted( $rc, Revision::DELETED_COMMENT ) ) {
+			if ( $this->userCan( $rc, Revision::DELETED_COMMENT ) ) {
+				' <span class="history-deleted">' .
+					Linker::commentBlock( $rc->mAttribs['rc_comment'], $rc->getTitle() ) . '</span>';
+			} else {
+				return ' <span class="history-deleted">' .
+					$this->msg( 'rev-deleted-comment' )->escaped() . '</span>';
+			}
+		} else {
+			return Linker::commentBlock( $rc->mAttribs['rc_comment'], $rc->getTitle() );
+		}
+	}

And this just adds complexity. There's a return in the if block above, so the else isn't needed. Just use an if (ie don't change this code)

 			return true;
-		}
-		if ( $row->afl_rev_id ) {
+		} elseif ( $row->afl_rev_id ) {
 			$revision = Revision::newFromId( $row->afl_rev_id );

I still need an answer as to how we merge a patch like this.

T207085#4672999 ...

CR-1, there's a missing return in here. I guess this path wasn't tested?

It actually was tested, but I probably didn't notice missing comments. Or, actually, I noticed them but unconsciously thought they were fine like that. Either way, done.

And this just adds complexity. There's a return in the if block above, so the else isn't needed. Just use an if (ie don't change this code)

Yes, it was just for clarity: since there's a return in the first if, changing the second to an elseif helps understand at a glance that the two ifs cannot be executed together. Removed anyway.

Reedy lowered the priority of this task from Unbreak Now! to High.Oct 18 2018, 10:18 PM

Patch deployed.

https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467 - "You cannot view the details for this entry because its associated revision is hidden from public view." both logged in (enwiki sysop) and as anon

Works as my WMF account

Do people want to test it further before we push the patch to gerrit?

Rxy awarded a token.Oct 18 2018, 10:25 PM
Rxy added a comment.Oct 18 2018, 10:27 PM

@Reedy Thanks for deploy.

@Daimona Thanks for submit the patch

@Reedy Some more testing wouldn't be bad, if we want to do it. Also, do we want to backport to 1.32?

@Reedy Some more testing wouldn't be bad, if we want to do it. Also, do we want to backport to 1.32?

It should be backported to all applicable and supported release branches

AbuseFilter isn't a bundled extension, so when it is in gerrit/hits master, this can be done in the open too

Daimona added a comment.EditedOct 20 2018, 9:35 AM

@Reedy Alright, thanks, I can push to gerrit once we're fine with it. The backports should be on wmf/1.32.0-wmf.26, REL1_27, REL1_30, REL1_31 and REL1_32; if we want to be fast, I can auto-merge there, but I guess it won't be necessary. I'll do some testing later today, if someone wants to coordinate testing to cover all possible cases you'll find me on IRC as "Daimona".

Daimona claimed this task.Oct 21 2018, 10:05 AM

Is everyone fine with this pushed on gerrit? Ping @Reedy, @Rxy if no oppositions are raised I'll push the patch tomorrow or so.

Pushed to gerrit, for all mentioned branches. I only merged the one on master, since I'd like someone else to double-check backports to be sure that everything is fine, CC @Reedy @Legoktm. Please note that I had to manually +2verify the one on REL1_31 due to T189560. Also: can this task be made public once everything will be merged?

Huji added a comment.Oct 23 2018, 5:22 PM

Not once merged, but once deployed.

@Huji the fix is already deployed to WMF wikis per T207085#4679131. There's still this patch to be merged on wmf.26 (could you please merge it?), but it doesn't really matters, it's just for completeness' sake.

Huji added a comment.Oct 23 2018, 11:48 PM

I can only +1 it, as my +2 rights don't expand into the WMF branches.
Great work by the way!

Ah, I didn't know. If someone with merge access on wmf. branches will come across the patch, that'd be nice! Otherwise, we can just abandon it as the new version is already on group0 and the fix is already deployed on other wikis.

Daimona closed this task as Resolved.Oct 26 2018, 3:01 PM

Nothing more to do here, this task can now be made public.

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".

@Daimona Thanks again for dealing with this. Am I correct that this doesn't apply on edits that have since been deleted? This is probably closer to T44734, but as far as I can tell, if a page is deleted, and if one of its now-deleted edits is then suppressed, if that edit had initially trigged an abusefilter, that entry it still needs manual hiding.

@Amorymeltzer Hmm I don't think it's intended, probably it's just some edge-case, possibly due to the fact that the page doesn't exist (maybe we handle non-existing pages differently, or Revision class does so). I'll investigate tomorrow anyway. Could you please confirm that the reproduction steps below are correct?
*Create a page with some revisions (and trigger a filter with one of these)
*Delete the page
*Suppress the revision (already deleted) which triggered the AF
*Go to Special:AbuseLog for that same revision, which will only check for "deletedtext" right (i.e. it's visible to sysops (who aren't oversighters) but not to "normal" users)
Also, I'm unsure if that task is related... The reason could be similar, but in that case solving the problem isn't trivial at all.

@Daimona Correct, although to your last point, after deleting the page oversighting the deleted revision(s), the AF info was visible to a non-sysop.

@Amorymeltzer I checked, and I confirm that it's a duplicate of T44734. Also, please note that this only happens in Special:AbuseLog, and not in Special:AbuseFilter/test or /examine (as it did for the problem originally reported in this task). And when it comes to specific AbuseLog entries, suppressing them is the right way, at least while waiting for a fix to T44734 (which won't be easy).