Page MenuHomePhabricator

Nonblockable user accounts can be created in the format 1.2.3.4-5.6.7.8
Closed, ResolvedPublicSecurity

Description

Because of varying definitions of what an IP range is, it is possible to create user accounts with names that are considered to be valid, yet are recognized by some of the blocking code as IP ranges. Then, either the "ranges" are rejected for being too large (/0 block), or the "ends" of the ranges are simply ignored, making it impossible to block the accounts via the web interface. (Blocking via the API seems to be possible if the deprecated userid parameter is used, though admins should not be expected to know how to do this.) An example of such a username is "1.2.3.4-5.6.7.8".

Steps to reproduce:

  1. While logged out, go to Special:CreateAccount.
  2. Enter the username "1.2.3.4-5.6.7.8" (no quotes) and a valid password.
  3. Click "Create your account".
  4. Log in to an administrator account.
  5. Go to Special:Log/newusers.
  6. Click the block link for the user "1.2.3.4-5.6.7.8".
  7. From the Expiration drop-down list, select "indefinite".
  8. Click "Block this user".
  9. Log in to the "1.2.3.4-5.6.7.8" user account.
  10. Make an edit to any unprotected page.
  11. Log in to an administrator account.
  12. Go to Special:Block.
  13. For "Username, IP address, or IP range:", enter "1.2.3.4-5.6.7.8".
  14. From the Expiration drop-down list, select "indefinite".
  15. Click "Block this user".

Explanation:

UserNameUtils::isValid attempts to check whether the username is actually an IP address or an IP range:

  • It checks for usernames that are valid IPv4 or IPv6 addresses, using the function IPUtils::isValid. The documentation for that function states, "Ranges are NOT considered valid."
  • It checks for usernames that look like IPv4 addresses, using the regex ^\d{1,3}\.\d{1,3}\.\d{1,3}\.(?:xxx|\d{1,3})$.
  • It checks for usernames that contain "/", which may be IP ranges in CIDR notation.

An "explicit" IPv4 range in the form "<start IP>-<end IP>" does not fall within any of these cases. In contrast, BlockUtils::parseBlockTarget, after checking whether the target is a valid IPv4 or IPv6 address using IPUtils::isValid, then checks whether the target is a valid IPv4 or IPv6 range using IPUtils::isValidRange. The documentation for that function states than an IP range is valid if "given with valid CIDR prefix or in explicit notation".

IPUtils::sanitizeRange is then called. That function first calls parseCIDR, which will return [ false, false ] in this case. Then, it calls parseRange, which will return the start and end of the range in hex format. Because parseCIDR returned false as the number of bits, sanitizeRange returns only the start of the range, because the range "wasn't actually a range".

BlockUtils::validateTarget, after calling BlockUtils::parseBlockTarget, checks which type of target was returned. For a range, it splits on the "/" in a range in CIDR notation. It tries to set $range to the part after the slash, generating a PHP notice or warning. It then casts the resulting null to the integer 0 before calling validateIPv4Range. That function, of course, rejects a /0 block as too large.

Details

Risk Rating
Medium
Author Affiliation
Wikimedia Communities

Event Timeline

Of course, an administrator could instead block the IP address the vandal is using, though only if they have access to that information. At least in Wikimedia's case, that would be only a relatively small number of administrators with CheckUser access.

Can you confirm that the user also cannot be blocked by specifying their user ID in the block API?

Can you confirm that the user also cannot be blocked by specifying their user ID in the block API?

No, that doesn't work either. For this API request made using Special:ApiSandbox:

{
	"action": "block",
	"format": "json",
	"user": "#3",
	"expiry": "never",
	"reason": "asdf",
	"token": "9dc9fa90ada25c227b7762e9db8eed5665dd2300+\\",
	"formatversion": "2"
}

I get:

{
    "block": {
        "user": "1.2.3.4",
        "userID": 0,
        "expiry": "infinite",
        "id": 5,
        "reason": "asdf",
        "anononly": false,
        "nocreate": false,
        "autoblock": false,
        "noemail": false,
        "hidename": false,
        "allowusertalk": false,
        "watchuser": false,
        "partial": false,
        "pagerestrictions": null,
        "namespacerestrictions": null
    }
}

And the account in question is still able to edit the site.

Can you confirm that the user also cannot be blocked by specifying their user ID in the block API?

No, that doesn't work either. For this API request made using Special:ApiSandbox:

Apparently, the deprecated userid parameter works though:

{
    "warnings": {
        "main": {
            "warnings": "Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/postorius/lists/mediawiki-api-announce.lists.wikimedia.org/> for notice of API deprecations and breaking changes."
        },
        "block": {
            "warnings": "The parameter \"userid\" has been deprecated."
        }
    },
    "block": {
        "user": "1.2.3.4-5.6.7.8",
        "userID": 3,
        "expiry": "infinite",
        "id": 6,
        "reason": "asdf",
        "anononly": false,
        "nocreate": false,
        "autoblock": false,
        "noemail": false,
        "hidename": false,
        "allowusertalk": false,
        "watchuser": false,
        "partial": false,
        "pagerestrictions": null,
        "namespacerestrictions": null
    }
}
DannyS712 added a project: MediaWiki-Blocks.

Interestingly, UserNameUtils does have an IP range-checking function, which just wraps IPUtils:isValidRange, but it isn't used within UserNameUtils::isValid or... anywhere else, from what I can tell (it's used in ExternalUserNames). So to stop this going forward, it should be a fairly trivial patch along the lines of:

Author: SBassett <sbassett@wikimedia.org>
Date:   Tue Feb 27 10:14:30 2024 -0600

    SECURITY: Add additional IP range validation to isValid function
    
    Bug: T358535

diff --git a/includes/user/UserNameUtils.php b/includes/user/UserNameUtils.php
index 3429720376b..09e2ab00072 100644
--- a/includes/user/UserNameUtils.php
+++ b/includes/user/UserNameUtils.php
@@ -112,6 +112,7 @@ class UserNameUtils implements UserRigorOptions {
        public function isValid( string $name ): bool {
                if ( $name === ''
                        || $this->isIP( $name )
+                       || $this->isValidIPRange( $name )
                        || str_contains( $name, '/' )
                        || strlen( $name ) > $this->options->get( MainConfigNames::MaxNameChars )
                        || $name !== $this->contentLang->ucfirst( $name )

Looking at the centralauth.globaluser table with a basic IPv4 range regexp, I'm not seeing anything:

wikiadmin2023@10.192.48.205(centralauth)> select gu_name, gu_email from globaluser where gu_name regexp '^[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\-[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}$';
Empty set (29.535 sec)

I know that's not everything (IPv6, local accounts, et al) but hopefully it's at least somewhat reassuring.

Hey all - does my above security patch seem reasonable? It would be nice to get some CR on that, especially if I'm missing some context or it's incomplete. Thanks.

Actually, account creation of this variety doesn't seem possible on most Wikimedia wikis, due to various rules under the global Title blacklist. So I might just put my above patch up as a hardening measure for MediaWiki core.

Change 1010263 had a related patch set uploaded (by SBassett; author: SBassett):

[mediawiki/core@master] Add additional IP range validation to isValid function

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

Change 1010263 merged by jenkins-bot:

[mediawiki/core@master] Add additional IP range validation to isValid function

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

With the above core mitigation now merged, is there anything left to do here other than resolve the task and make it public?

sbassett lowered the priority of this task from High to Medium.
sbassett moved this task from Watching to Our Part Is Done on the Security-Team board.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Medium.
sbassett removed a project: Patch-For-Review.