Page MenuHomePhabricator

Old public versions of private filters are publicly viewable (CVE-2019-18987)
Closed, ResolvedPublic

Description

It's possible to view old, public versions of abusefilters that are currently marked private, by putting the ID of a public filter in the URL.

For example, version 22545 of private filter 1008 on enwiki was accidentally marked public, but the mistake was soon caught.

If we try to view it the "normal" way:

https://en.wikipedia.org/wiki/Special:AbuseFilter/history/1008/item/22545

then we get an error, as expected. But if we pretend we're looking at filter 1 instead:

https://en.wikipedia.org/wiki/Special:AbuseFilter/history/1/item/22545

then we are shown the filter pattern, as if it were public. This leaves no way to fix the mistake of accidentally un-ticking the "private" box.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSun, Nov 10, 9:13 PM
Urbanecm triaged this task as High priority.
Urbanecm added a subscriber: Urbanecm.

Thanks for noticing this!

Restricted Application added a project: User-Urbanecm. · View Herald TranscriptSun, Nov 10, 10:11 PM
Urbanecm added a project: Patch-For-Review.
Urbanecm added a subscriber: Daimona.

Fix is quite simple: Add a check into AbuseFilterViewEdit to make sure history ID and filter ID matches. My patch throws an exception if those two IDs doesn't match, because IIRC they should always match unless user is tampering the URL. @Daimona, could you review, please?

@Urbanecm: That was quick! Your commit message says users can "view any version of any filter", but the problem isn't that severe. Only old public versions are visible, which is only a problem when filter managers tick the wrong box. At least, I get an error for:

https://en.wikipedia.org/wiki/Special:AbuseFilter/history/1/item/22558

because version 22558 of filter 1008 was correctly marked private.

Thanks, updated commit message:

Geez. I've never liked that extra parameter (for the filter ID) in diff URLs. First because it's redundant. Second, because it unnecessarily complicates things. So, we should first put a stopgap in place, then I'd like to remove the parameter altogether.

Re: patch above:

My patch throws an exception if those two IDs doesn't match, because IIRC they should always match unless user is tampering the URL.

While that's *probably* true, I don't think it's fair to just throw an Exception which would the bubble up to the user. So, we should show a pretty error message. However, that would mean adding a new message, new code, new translations, just to remove it soon. Hence, I'd be fine with adding the $row->af_id !== $filter case to the if just below. The error shown will be

The filter you specified does not exist

But that's fine, if someone is just messing with the URL.

@Daimona Something like this?

@Daimona Something like this?

Exactly, thanks! I just forgot one thing: adding a @fixme comment right above the if, saying something like:

@fixme Temporary stopgap for T237887

Just in case that code survives for more than expected.

Done

Done

Tested, -1 because it prevents the creation of new filters. The check should probably contain also `$filter !== 'new', and maybe $row->af_id should be cast to int.

Thanks. Changed the condition to be $history_id && $row->af_id !== $filter, given $history_id is null when not present.

Thanks. Changed the condition to be $history_id && $row->af_id !== $filter, given $history_id is null when not present.

OK, makes sense. One last nit: you should put spaces inside parentheses in the if condition, otherwise PHPCS will complain. After that is done, I'm fine with the patch.

Done. Could i get one final review then please?

Done. Could i get one final review then please?

Code is looking good, manual testing says it works, and PHPCS and phan are fine with it. So, +2, you can deploy whenever you want. I don't know whether I'll be available for live testing, but the links provided in the task description should suffice.

19:18 <Urbanecm> !log Deploy security patch for T237887

@sbassett Could you do the final honours?

19:18 <Urbanecm> !log Deploy security patch for T237887

@sbassett Could you do the final honours?

Sure, I assume you mean 1) CVE 2) backports 3) tracking at T234983 and then making this task public?

19:18 <Urbanecm> !log Deploy security patch for T237887

@sbassett Could you do the final honours?

Sure, I assume you mean 1) CVE 2) backports 3) tracking at T234983 and then making this task public?

Yes.

Sure, I assume you mean 1) CVE 2) backports 3) tracking at T234983 and then making this task public?

Yes.

Yes, will try to complete this week.

Exactly, thanks! I just forgot one thing: adding a @fixme comment right above the if, saying something like:

@fixme Temporary stopgap for T237887

Just in case that code survives for more than expected.

@Daimona, @Urbanecm - I have the backports ready to go, but I just noticed this ^ again and the comment within the patch. I wanted to make sure that we're fine with this living on master and any supported release branches where it will apply. I assume we are, given the nature of the comment, but I just wanted to double-check that the patch wasn't intended to be a very short-lived solution.

Exactly, thanks! I just forgot one thing: adding a @fixme comment right above the if, saying something like:

@fixme Temporary stopgap for T237887

Just in case that code survives for more than expected.

@Daimona, @Urbanecm - I have the backports ready to go, but I just noticed this ^ again and the comment within the patch. I wanted to make sure that we're fine with this living on master and any supported release branches where it will apply. I assume we are, given the nature of the comment, but I just wanted to double-check that the patch wasn't intended to be a very short-lived solution.

I'm fine with that. The only problem with that patch is that we show a somewhat generic error message ("filter doesn't exist") instead of a specific one ("IDs don't match"). However, you can only get the error if you manually mess up with the URL, so you should already know what's going on. Aside from that, the solution is not short-lived.

As I said, after the backports are merged, I'll try to remove that extra param (I still don't know what the new URL format should look like, though). If it turns out to be too hard to implement, then I'll split the IF case, and add a new message, which we can also backport if we want.

At any rate, waiting for your green light before pushing anything to gerrit.

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

Change 550949 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/AbuseFilter@REL1_34] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550950 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/AbuseFilter@REL1_33] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550951 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/AbuseFilter@REL1_32] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550954 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550951 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_32] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550949 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_34] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550950 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_33] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550948 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] SECURITY: Make sure provided filter id match provided history ID in history view

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

Change 550954 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Make sure provided filter id match provided history ID in history view

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

sbassett renamed this task from Old public versions of private filters are publicly viewable to Old public versions of private filters are publicly viewable (CVE-2019-18987).Fri, Nov 15, 4:13 PM
sbassett closed this task as Resolved.Fri, Nov 15, 4:16 PM
sbassett moved this task from Pending deployment / release to Done on the Security board.

Ok, backports done, CVE filed (CVE-2019-18987), tracking in T234983. I'm going to resolve this for now. Thanks everyone.