Page MenuHomePhabricator

abusefilterchecksyntax and abusefilterevalexpression should require the same permissions
Closed, ResolvedPublic

Description

https://en.wikipedia.org/w/api.php?action=abusefilterchecksyntax&format=json&filter=norm('F00BAR')

{"error":{"code":"permissiondenied","info":"You don't have permission to check syntax of abuse filters.","*":"See https://en.wikipedia.org/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/mailman/listinfo/mediawiki-api-announce> for notice of API deprecations and breaking changes."},"servedby":"mw1227"}

https://en.wikipedia.org/w/api.php?action=abusefilterevalexpression&format=json&expression=norm('F00BAR')

{"abusefilterevalexpression":{"result":"FOBAR"}}

Evaluating an expression requires first checking its syntax. If abusefilterchecksyntax is a DoS vector, so is abusefilterevalexpression. So either (A) both should be restricted, or (B) both should be available to all users. I don't know which is correct, but with (evil) regex involved, I imagine it's (A).

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFri, Nov 15, 9:40 PM
Daimona claimed this task.Sat, Nov 16, 1:18 PM
Daimona added a subscriber: Daimona.

I don't know whether it can be considered a DoS vector. Sure, evil regexps etc., but I don't know how easy it would be to exploit. At any rate, I do agree that they should have the same priority. Patch coming shortly.

CC'ing for review & deployment.

Urbanecm triaged this task as High priority.Sat, Nov 16, 8:36 PM
Urbanecm moved this task from Backlog / Other to Patch pending review on the Security board.

CC'ing for review & deployment.

+1. Maybe mention the task for reference?

CC'ing for review & deployment.

+1. Maybe mention the task for reference?

What task?

When reading the code, I would be wondering which DoS vuln is in this part of code. Maybe that's explained elsewhere, but this very task would at least give tips where I can ask for more info.

When reading the code, I would be wondering which DoS vuln is in this part of code. Maybe that's explained elsewhere, but this very task would at least give tips where I can ask for more info.

As I said, I'm unsure whether the risk of DoS is concrete. I basically just copied what other modules (and the UI) do. But anyway, that module is doing a single thing (evaluate an expression), so there's not much to say about where the DoS may be hiding.

Okay, makes sense

I don't know whether it can be considered a DoS vector. Sure, evil regexps etc., but I don't know how easy it would be to exploit.
_
As I said, I'm unsure whether the risk of DoS is concrete. I basically just copied what other modules (and the UI) do. But anyway, that module is doing a single thing (evaluate an expression), so there's not much to say about where the DoS may be hiding.

Unless we have extremely strong suspicions or a demonstrated attack vector can be presented, I'd agree that we shouldn't consider this a potential DoS, at least for the time being. The patch above looks good to me for the permissions issue. Since there isn't currently a demonstrated vulnerability here IMO (perhaps a very mild Vuln-Infoleak) I feel like this could be done through gerrit as a code-hardening measure.

I don't know whether it can be considered a DoS vector. Sure, evil regexps etc., but I don't know how easy it would be to exploit.
_
As I said, I'm unsure whether the risk of DoS is concrete. I basically just copied what other modules (and the UI) do. But anyway, that module is doing a single thing (evaluate an expression), so there's not much to say about where the DoS may be hiding.

Unless we have extremely strong suspicions or a demonstrated attack vector can be presented, I'd agree that we shouldn't consider this a potential DoS, at least for the time being.

I'm unsure... I think we're pretty good at preventing classic ReDoS attacks (i.e., if you try one of the usual examples of catastrophic backtracking, PHP will refuse to match the regex against the text and the filter will bail out immediately). Of course, this doesn't exclude the likelihood of other DoS vectors.

The patch above looks good to me for the permissions issue. Since there isn't currently a demonstrated vulnerability here IMO (perhaps a very mild Vuln-Infoleak) I feel like this could be done through gerrit as a code-hardening measure.

I'm fine with doing everything on gerrit, but I'd like to have someone be ready to review & merge the patch. Which is to say, even if the risk is mild, I don't want to let the patch sit on gerrit for a week :)

I'm unsure... I think we're pretty good at preventing classic ReDoS attacks (i.e., if you try one of the usual examples of catastrophic backtracking, PHP will refuse to match the regex against the text and the filter will bail out immediately). Of course, this doesn't exclude the likelihood of other DoS vectors.

Sure, that's fine. I just don't want anyone (including myself) to go on wild goose chases for DoS vectors which might exist, without any further evidence provided.

I'm fine with doing everything on gerrit, but I'd like to have someone be ready to review & merge the patch. Which is to say, even if the risk is mild, I don't want to let the patch sit on gerrit for a week :)

Looks like there isn't a train this week, so maybe sometime tomorrow 2019-11-21 outside of the SWATs or Friday 2019-11-22? Happy to +2 and/or deploy this once it's merged to wmf/1.35.0-wmf.5.

@sbassett Sure! Today is fine by me, I'm certainly available in the next 2-3 hours. If that works for you, please ping me on IRC so I can know when to push to gerrit.

Daimona closed this task as Resolved.Thu, Nov 21, 4:51 PM
Daimona removed a project: Patch-For-Review.

Patch merged & deployed, and backports merged as well.

Daimona changed the visibility from "Custom Policy" to "Public (No Login Required)".Thu, Nov 21, 4:51 PM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptThu, Nov 21, 4:51 PM

FWIW: PHP has pcre.backtrack_limit, so I don't think ReDoS is possible.

sbassett moved this task from Patch pending review to Done on the Security board.Thu, Nov 21, 4:58 PM