Page MenuHomePhabricator

$wgRateLimits (rate limit / ping limiter) entry for 'user' overrides that for 'newbie' (CVE-2018-0503)
Closed, ResolvedPublic

Description

Contrary to the documentation, $wgRateLimits entry for 'user' overrides that for 'newbie'.

For example, with the following configuration, newly registered accounts are able to edit 10 pages per hour instead of 5:

$wgRateLimits[ 'edit' ] = [
  'user' => [ 10, 60*60 ],
  'newbie' => [ 5, 60*60 ],
];

This seems to be the opposite of what documentation in DefaultSettings.php says:

'newbie' => [ x, y ], // each new autoconfirmed accounts; overrides 'user'

(although that should probably be 'non-autoconfirmed'…).

Event Timeline

matmarex created this task.Jul 3 2017, 3:42 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 3 2017, 3:42 PM

This affects some of the built-in rate limits too, e.g. for page moves.

Looks like the issue was introduced in 2008 by 5ac6ff8294a54bce7994b2eeb55a246609016b99 (rSVN35130), due to the changed order of operations? I'm finding this a bit hard to believe that no one noticed.

matmarex set Security to Software security bug.Jul 3 2017, 3:59 PM
matmarex added a project: Security.
matmarex changed the visibility from "Public (No Login Required)" to "Custom Policy".

Probably should have filed this as a security task, whoops.

Proposed patch:

On a more general note, configuring this would be simpler if it allowed setting a limit for the 'autoconfirmed' group, which acted the same as all the other group limits, instead of using the 'newbie' limit with this custom behavior. Currently setting a limit for 'autoconfirmed' is ignored (the code uses User::getGroups() instead of getAllGroups()).

Bawolff added a subscriber: Bawolff.

Proposed patch:


On a more general note, configuring this would be simpler if it allowed setting a limit for the 'autoconfirmed' group, which acted the same as all the other group limits, instead of using the 'newbie' limit with this custom behavior. Currently setting a limit for 'autoconfirmed' is ignored (the code uses User::getGroups() instead of getAllGroups()).

Patch looks good.

It would be nice to add unit tests for this somehow, as the logic in that method is kind of convoluted, but that's a separate issue.

Reedy added a subscriber: Reedy.Jul 10 2017, 10:08 PM
[23:08:07] <logmsgbot> !log reedy@tin Synchronized php-1.30.0-wmf.7/includes/: (no justification provided) (duration: 01m 33s)

Deployed

Reedy lowered the priority of this task from High to Medium.Jul 10 2017, 10:08 PM

Do we still need to be carrying this in production? Why hasn't this been fixed in a released version?

mmodell raised the priority of this task from Medium to High.May 22 2018, 8:19 PM

Its waiting on next swcurity release. Hopefully in next 3 weeks maybe...

Reedy closed this task as Resolved.Jul 7 2018, 5:55 PM
Reedy assigned this task to matmarex.

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 20 2018, 9:34 PM
MoritzMuehlenhoff renamed this task from $wgRateLimits (rate limit / ping limiter) entry for 'user' overrides that for 'newbie' to $wgRateLimits (rate limit / ping limiter) entry for 'user' overrides that for 'newbie' (CVE-2018-0503).Sep 21 2018, 7:24 AM