CC'ing for review & deployment.

In T238451#5668847, @Daimona wrote:

CC'ing for review & deployment.

T238451.patch2 KBDownload

+1. Maybe mention the task for reference?

In T238451#5669753, @Urbanecm wrote:

In T238451#5668847, @Daimona wrote:

CC'ing for review & deployment.

T238451.patch2 KBDownload

+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.

In T238451#5669763, @Urbanecm wrote:

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

In T238451#5668844, @Daimona wrote:

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.

In T238451#5673548, @sbassett wrote:

In T238451#5668844, @Daimona wrote:

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 :)

In T238451#5677820, @Daimona wrote:

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.

References:

1. sal: https://tools.wmflabs.org/sal/log/AW6O3XlDfYQT6VcDzFev
2. gerrit: https://gerrit.wikimedia.org/r/q/Id09fd57195d71884674ac0470f137ca30c56e13c
3. No CVE for this and won't be tracked at T234983

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