Page MenuHomePhabricator

Older hidden versions of a currently-public AbuseFilter are exposed via diffs (CVE-2019-18612)
Closed, ResolvedPublic

Description

There is a problem with hidden abusefilters, for example: this filter is actually not hidden: https://de.wikipedia.org/wiki/Spezial:Missbrauchsfilter/3, but older versions are hidden, for example, the last hidden version: https://de.wikipedia.org/wiki/Spezial:Missbrauchsfilter/history/3/item/40, without sysop rights not visible. The version after this is not hidden, so visible for all users: https://de.wikipedia.org/wiki/Spezial:Missbrauchsfilter/history/3/item/42. The problem now is, that the difference is visible for all users: https://de.wikipedia.org/wiki/Spezial:Missbrauchsfilter/history/3/diff/prev/42, and also the difference beetween hidden versions: https://de.wikipedia.org/wiki/Spezial:Missbrauchsfilter/history/3/diff/prev/40, so non sysops can construct the hidden version, thats the problem.

Details

Related Gerrit Patches:
mediawiki/extensions/AbuseFilter : REL1_32SECURITY: Check visibility for each version in ViewDiff
mediawiki/extensions/AbuseFilter : REL1_31SECURITY: Check visibility for each version in ViewDiff
mediawiki/extensions/AbuseFilter : REL1_33SECURITY: Check visibility for each version in ViewDiff
mediawiki/extensions/AbuseFilter : REL1_34SECURITY: Check visibility for each version in ViewDiff
mediawiki/extensions/AbuseFilter : masterSECURITY: Check visibility for each version in ViewDiff

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Jul 5 2015, 3:55 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 5 2015, 3:55 PM
Luke081515 triaged this task as High priority.
Luke081515 updated the task description. (Show Details)
Luke081515 added projects: AbuseFilter, WMF-NDA.
Luke081515 changed Security from None to Other confidential issue.
Luke081515 edited subscribers, added: Luke081515; removed: Aklapper.
Krenair changed Security from Other confidential issue to Software security bug.Jul 5 2015, 3:57 PM
Restricted Application added a project: Security. · View Herald TranscriptJul 5 2015, 3:57 PM
Krenair changed the visibility from "Custom Policy" to "Custom Policy".
Krenair changed the edit policy from "Custom Policy" to "Custom Policy".
Krenair renamed this task from Problem with hidden AbuseFilters, not completly hidden, difference is visible to Older hidden versions of a currently-public AbuseFilter are exposed via diffs.Jul 5 2015, 7:44 PM
Krenair added a subscriber: Krenair.
hoo added a subscriber: hoo.Jul 5 2015, 8:32 PM
Krenair merged a task: Restricted Task.Feb 23 2016, 3:24 AM
Krenair added subscribers: Se4598, Ayokura, Aklapper.
Krenair changed the visibility from "Custom Policy" to "Custom Policy".Feb 24 2016, 11:13 AM
Krenair changed the edit policy from "Custom Policy" to "Custom Policy".
Daimona claimed this task.Jan 18 2019, 12:27 PM
Daimona added a subscriber: Daimona.

To fix this (fairly simple) and avoid showing diff links.

Urbanecm added a subscriber: matej_suchanek.

Fixed conflict with newer version of code, confirmed it works locally.


Fixed conflict with newer version of code, confirmed it works locally.

A piece was missed there, which I restored in the patch below. Plus: use canViewPrivate everywhere, order by afh_id in a query, and add a @todo about caching for when the code will be public.

Thank you Daimona. Actions are always public, so it is not necessary to hide them, I think.

Thank you Daimona. Actions are always public, so it is not necessary to hide them, I think.

I wouldn't be too sure about that (e.g. throttle params are a bit sensitive), but I agree that it's probably fine not to hide them. Anyway, we can think about it once the code is public.

Note: when viewing the diff right before the hidden version, you'll still see a "newer change" button at the bottom; clicking it will just tell you that you cannot see the revision. I guess we can make it disappear in a follow-up.

@sbassett, @Reedy Waiting for your green light before pushing publicly :)

Deployed earlier today as-of this patch.

Deployed earlier today as-of this patch.

Thanks.

@sbassett, @Reedy Waiting for your green light before pushing publicly :)

@Daimona @Urbanecm - can we give this at least a couple of days in production to assess functionality and stability? Then we can do the backports through gerrit. Also tracking in T234983 and will plan to request a CVE soon, though that shouldn't hold up the backports.

Deployed earlier today as-of this patch.

Thanks.

@sbassett, @Reedy Waiting for your green light before pushing publicly :)

@Daimona @Urbanecm - can we give this at least a couple of days in production to assess functionality and stability? Then we can do the backports through gerrit. Also tracking in T234983 and will plan to request a CVE soon, though that shouldn't hold up the backports.

Sure!

@Daimona @Urbanecm - can we give this at least a couple of days in production to assess functionality and stability? Then we can do the backports through gerrit. Also tracking in T234983 and will plan to request a CVE soon, though that shouldn't hold up the backports.

Sure, thanks. Can we also make this task public once the backports are on gerrit?

i guess so, given no sensitive things are here.

Sure, thanks. Can we also make this task public once the backports are on gerrit?

i guess so, given no sensitive things are here.

I'm not seeing anything on the task that would prevent it from being made public. And given that it's for a non-bundled extension, we don't have to hold the task for a mediawiki security release or anything like that. The new announcements I've been trying to do (e.g. T234983) are designed to occur after backports and any other public disclosure (CVE, etc.)

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Mon, Oct 28, 8:33 PM
sbassett changed the edit policy from "Custom Policy" to "All Users".

Change 546723 had a related patch set uploaded (by SBassett; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] SECURITY: Check visibility for each version in ViewDiff

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

Change 546726 had a related patch set uploaded (by SBassett; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@REL1_34] SECURITY: Check visibility for each version in ViewDiff

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

Ugh, security backports conflicting with https://gerrit.wikimedia.org/r/480069.

Change 546723 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] SECURITY: Check visibility for each version in ViewDiff

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

Change 546726 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_34] SECURITY: Check visibility for each version in ViewDiff

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

sbassett lowered the priority of this task from High to Normal.Mon, Oct 28, 9:50 PM
sbassett added a comment.EditedMon, Oct 28, 9:57 PM

Backports for master and REL1_34 were merged, CVE requested. It appears that for the other supported release branches (1.31, 1.32, 1.33) it's just an issue of line differences for the patch. I'll try to get some additional patches uploaded to gerrit for these branches. @Daimona - would you be able to review those just in case the patch isn't relevant? (I think it still is - whoops, it's not.)

Change 546752 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Check visibility for each version in ViewDiff

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

Change 546753 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/AbuseFilter@REL1_32] SECURITY: Check visibility for each version in ViewDiff

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

Change 546754 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/AbuseFilter@REL1_33] SECURITY: Check visibility for each version in ViewDiff

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

Change 546754 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_33] SECURITY: Check visibility for each version in ViewDiff

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

sbassett renamed this task from Older hidden versions of a currently-public AbuseFilter are exposed via diffs to Older hidden versions of a currently-public AbuseFilter are exposed via diffs (CVE-2019-18612).Tue, Oct 29, 3:51 PM

FTR: I've updated the backports for 1.31, 32, and 33 to avoid using AbuseFilter::canViewPrivate (introduced in 1.34). For the 1.31 one, I also had to upgrade PHPCS to 19.1.0 (and disable a new sniff) because older versions fail on PHP7.3 (T233759).

As I've already pointed out, things would've been easier with phan running on REL branches (T226945 / T231966).

@Daimona - great, thanks for the help! Once the remaining backports are merged, I'll resolve this task.

Change 546752 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Check visibility for each version in ViewDiff

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

Change 546753 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_32] SECURITY: Check visibility for each version in ViewDiff

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

sbassett closed this task as Resolved.Thu, Oct 31, 4:05 PM
sbassett removed a project: Patch-For-Review.