Page MenuHomePhabricator

striker does not (?) honour TitleBlacklist for shell names
Closed, InvalidPublic

Description

IIRC Extension:OpenStackManager checks not only the wikitech account name, but also (and more importantly) the shell user name against TitleBlacklist, so users can't create accounts with the shell name root, for example. If I interpret striker/register/utils.py correctly, for shell names it only checks that they are not already in LDAP.

Event Timeline

scfc created this task.Jan 26 2017, 5:39 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 26 2017, 5:39 AM
scfc added a subscriber: bd808.
Restricted Application added a project: Cloud-Services. · View Herald TranscriptJan 26 2017, 5:39 AM

@bd808 -- Can you take a look at this?

dpatrick triaged this task as Unbreak Now! priority.Jan 31 2017, 9:36 PM
dpatrick moved this task from Backlog / Other to Other WMF team on the Security board.

This was covered by T147024: Striker should respect TitleBlacklist bans on new account names where various related commits and analysis are documented.

The check of the shell name against AbuseFilter and other bans is done in striker.register.forms.ShellUsername.clean_shellname

def clean_shellname(self):
    """Validate that shellname is available."""
    shellname = self.cleaned_data['shellname']
    if not utils.shellname_available(shellname):
        raise forms.ValidationError(_('Shell username is already in use.'))

    # Check that it isn't banned by some abusefilter type rule
    user = utils.check_username_create(shellname)
    if user['ok'] is False:
        raise forms.ValidationError(user['error'])

    return shellname

The striker.register.utils.check_username_create function is:

def check_username_create(name):
    """Check to see if a given name would be allowed as a username.

    Returns True if the username would be allowed. Returns either False or a
    reason specifier if the username is not allowed.
    Returns a dict with these keys:
    - ok : Can a new user be created with this name (True/False)
    - name : Canonicalized version of the given name
    - error : Error message if ok is False; None otherwise
    """
    # Make sure to use the anon client here because on-wiki rights can affect
    # the result of the cancreate check.
    mwapi = mediawiki.Client.anon_client()
    user = mwapi.query_users_cancreate(name)[0]
    # Example response:
    # [{'missing': True, 'name': 'Puppet',
    # 'cancreate': False, 'cancreateerror': [{'message':
    # 'titleblacklist-forbidden-new-account', 'params': ['
    # ^(User:)?puppet$ <newaccountonly>', 'Puppet'], 'type': 'error'}]}]
    ret = {
        'ok': False,
        'name': user['name'],
        'error': None,
    }
    if user['missing'] and user['cancreate']:
        ret['ok'] = True
    elif 'userid' in user:
        ret['error'] = _('%(name)s is already in use.') % ret
    elif 'cancreateerror' in user:
        try:
            ret['error'] = mwapi.get_message(
                    user['cancreateerror'][0]['message'],
                    *user['cancreateerror'][0]['params'],
                    lang=translation.get_language().split('-')[0])
        except Exception:
            logger.exception('Failed to get expanded message for %s', user)
            ret['error'] = user['cancreateerror'][0]['message']
    return ret

The striker.mediawiki.Client.query_users_cancreate method implementation is:

def query_users_cancreate(self, *users):
    """Check to see if the given usernames could be created or not.

    Note: if this Client is authenticated to the target wiki, the result
    that you get from this request may or may not be the same result that
    an anonymous user would get.
    """
    result = self.site.api(
        'query', formatversion=2,
        list='users',
        usprop='cancreate', ususers='|'.join(users),
    )

    logger.debug(result)
    # Example result:
    # {'query': {'users': [{'missing': True, 'name': 'Puppet',
    # 'cancreate': False, 'cancreateerror': [{'message':
    # 'titleblacklist-forbidden-new-account', 'params': ['
    # ^(User:)?puppet$ <newaccountonly>', 'Puppet'], 'type': 'error'}]}],
    # 'userinfo': {'anon': True, 'messages': False, 'name':
    # '137.164.12.107', 'id': 0}}, 'batchcomplete': True}
    # TODO: error handling
    return result['query']['users']

I believe this gives the same functional result as the explicit TitleBlacklistHooks::acceptNewUserName check done in OpenStackNovaUser::AbortNewAccount. The results for the shell name root can be checked with this API Sandbox query:

{
    "batchcomplete": true,
    "query": {
        "users": [
            {
                "name": "Root",
                "missing": true,
                "cancreate": false,
                "cancreateerror": [
                    {
                        "message": "titleblacklist-forbidden-new-account",
                        "params": [
                            " ^(User:)?root$ <newaccountonly>",
                            "Root"
                        ],
                        "code": "titleblacklist-forbidden",
                        "type": "error"
                    }
                ]
            }
        ]
    }
IMPORTANT: Running this query with a privileged user account (like mine) will yield a false positive. This is because the action=query&list=users API action does not have a flag to specify that the tboverride user privilege should be ignored. This is something that it would be nice to fix in the API at some point to make such checks a bit safer to perform regardless of the calling user's privileges. action=titleblacklist handles this with the tbnooverride flag.
scfc added a comment.Jan 31 2017, 10:59 PM

@bd808: Thanks. Being unfamiliar with the Django structure, the check wasn't obvious to me. With your explanation I think the checks are satisfactory and this task can be closed as invalid.

bd808 closed this task as Invalid.Jan 31 2017, 11:03 PM

Thanks for checking my work @scfc. I could have just as easily missed this critical bit of logic.

Can this ticket be made public?

bd808 changed the visibility from "Custom Policy" to "Public (No Login Required)".Feb 3 2017, 5:58 PM
Restricted Application added subscribers: Jay8g, TerraCodes. · View Herald TranscriptFeb 3 2017, 5:58 PM
bd808 moved this task from Backlog to Done on the Striker board.Mar 4 2017, 3:52 AM
Restricted Application removed a subscriber: TerraCodes. · View Herald TranscriptMar 4 2017, 3:52 AM