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 Normal.Jul 10 2017, 10:08 PM
Reedy moved this task from Patch pending deployment to Pending release on the Security board.

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 Normal 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