[patch] Add optional username regexp to registration checks

[Sebastian Krysmanski]

Today I stumbled upon a big security hole or at least a way to almost render a Trac environment useless in the AccountManagerPlugin. The problem is that the check for existing users when creating a new one in the Registration module is flawed.

The check doesn't check for the usernames "anonymous" and "authenticated". So it's easily possible to register as "anonymous" or "authenticated". While you can't login as "anonymous" you can do so as "authenticated". While this is already a bad thing it gets worse. Since there is the possibility to delete my account one could register as "authenticated" and then delete the account again. Doing so he/she would thereby remove the group "authenticated" from the permessions store. This way all registered user would be demoted to "anonymous" (unless they were given some permissions directly).

Also it's possible to register as an already existing permission group (like "anonymous" and "authenticated") and inherit all of its permissions. Say I've created a group called "developers", added the permission "TICKET_ADMIN" to this group and assigned all the members of my development team to this group. Now they all have the permission "TICKET_ADMIN". However, a user could register with the name "developers" and also gain the permission "TICKET_ADMIN". And that's very bad! (Fortunately the user has to know the name of the permission group which he/she can't find out so easily. But he/she could simply try some common names.)

I've attached a patch to this ticket the fixes this problem. It also !add the possibility for the !admin to define a regular expression for valid usernames. Moreover the patch prohibits usernames containing characters that are harmful for some password stores (this fixes #4682 and #2630).

[authenticated]

As you can see I (manski) have already register as "authenticated". I've only done this to protect this Trac environment.

[Sebastian Krysmanski]

Patch against r5836 and Trac 0.11.4

[Sebastian Krysmanski]

A workaround for this problem is:

• disable the registration module
• or create users for all permission groups in the password store

[Noah Kantrowitz]

Hmm, there is already a simple check for existing groups, that should probably be strengthened rather than invoking some kind of regex as your patch does. Other than explicitly blacklisting "anonymous" and "authenticated" I'm not really sure how best to do that though.

[Noah Kantrowitz]

Also, this is at best a nuisance, you cannot gain permissions you shouldn't otherwise have.

[Sebastian Krysmanski]

Replying to coderanger:

Hmm, there is already a simple check for existing groups, that should probably be strengthened rather than invoking some kind of regex as your patch does. Other than explicitly blacklisting "anonymous" and "authenticated" I'm not really sure how best to do that though.

Sorry that I've included it. The regexp is optional. (The admin can define at regular expression for valid user names.) The actual check is in the lines above the regexp check.

[Sebastian Krysmanski]

Replying to coderanger:

Also, this is at best a nuisance, you cannot gain permissions you shouldn't otherwise have.

Well, you can gain permissions of existing Trac groups (as described above).

[Noah Kantrowitz]

Replying to manski:

Replying to coderanger:

Also, this is at best a nuisance, you cannot gain permissions you shouldn't otherwise have.

Well, you can gain permissions of existing Trac groups (as described above).

Only if those groups have the same permissions as "authenticated".

[Sebastian Krysmanski]

Replying to coderanger:

Replying to manski:

Replying to coderanger:

Also, this is at best a nuisance, you cannot gain permissions you shouldn't otherwise have.

Well, you can gain permissions of existing Trac groups (as described above).

Only if those groups have the same permissions as "authenticated".

No, I think that exactly the thing that is not checked. If a group has the same rights as "authenticated" the following condition is evaluated as "true":

!python
permission_system = perm.PermissionSystem(env) 
if permission_system.get_user_permissions(user) != \ 
   permission_system.get_user_permissions('authenticated'): 
    error.message = 'Another account with that name already exists.' 
    raise error 

However, if the group has more (oder less) permissions than "authenticated" the condition above will be evaluated as "false" and therefore no error will be raised.

[Noah Kantrowitz]

Replying to manski:

Replying to coderanger:

Replying to manski:

Replying to coderanger:

Also, this is at best a nuisance, you cannot gain permissions you shouldn't otherwise have.

Well, you can gain permissions of existing Trac groups (as described above).

Only if those groups have the same permissions as "authenticated".

No, I think that exactly the thing that is not checked. If a group has the same rights as "authenticated" the following condition is evaluated as "true":

!python
permission_system = perm.PermissionSystem(env) 
if permission_system.get_user_permissions(user) != \ 
   permission_system.get_user_permissions('authenticated'): 
    error.message = 'Another account with that name already exists.' 
    raise error 

However, if the group has more (oder less) permissions than "authenticated" the condition above will be evaluated as "false" and therefore no error will be raised.

Pretty sure you have that backwards. If you try to register a username and that username has any permissions different from "authenticated", the registration is rejected.

[Thijs Triemstra]

So is there a security hole in this plugin or not?

[olly@…]

I think it's "remote denial of service" at worst.

I tested, but couldn't register using the name of the group with TRAC_ADMIN and WIKI_ADMIN privileges on my 0.11.4 trac installation, so the privilege-escalation scenario in the original description doesn't seem to work (as suggested by previous comments).

But (as also mentioned in the description) an attacker probably could register with user name authenticated and then delete that account, so deleting the permissions for group authenticated. which on a typical trac installation with this plugin installed would remove all create and modify rights from normal users - i.e. a remote denial of service. But I've not tested this myself.

[Sebastian Krysmanski]

Replying to olly@survex.com:

I tested, but couldn't register using the name of the group with TRAC_ADMIN and WIKI_ADMIN privileges on my 0.11.4 trac installation, so the privilege-escalation scenario in the original description doesn't seem to work (as suggested by previous comments).

Sorry, but that's not correct. These are not groups. These are permissions. Groups are basically usernames without password. You can add a "subject" (this is a user oder group) to a group using the permissions admin panel.

In my example I created a group called "developers" that has the permission TICKET_ADMIN. And if then one registers as "developers" (as just somebody has done on this page to check whether he/she can gain rights through this) the user would gain the TICKET_ADMIN right.

[anonymous]

You misunderstand me. I have a named group in my trac install with permissions TRAC_ADMIN and WIKI_ADMIN, and I tried to register using the name of that group as my username, but it doesn't let me. I wasn't trying to register with user name TRAC_ADMIN or WIKI_ADMIN (and if I had tried that, it would presumably have worked, not failed. But that's OK, that wouldn't give me any magic powers).

I just tried your exact example - I created a new group "developers" with permission TICKET_ADMIN. Then I logged out and tried to create a new user account called "developers", but I got this error:

Error

Another account with that name already exists.

I can create user "developers" via the users section in the web admin UI, but an attacker wouldn't have access to that (if they did, they could give themselves whatever permissions they want anyway).

[d.nedelchev]

Or maybe just to allow only all-uppercase group-names (like the permissions itself)? In fact a group here is a set of permissions so it sounds meaningful... All-uppercase subjects are automatically excluded from allowed subjects list (admin). However, (I've just tried it) they are allowed usernames for the registration module (and obviously it should be fixed there)! The only issue I guess will be backward-compatibility with the existing non-all-uppercase group-names like "anonymous", "authenticated" and any custom group since now all-uppercase groups are prohibited in the admin-panel (as well as all-uppercase usernames). What do you think, is it a good or a bad idea?

P.S. I've read in the comments this issue allows a user to obtain only permissions which already have. I've tried it on my local sandbox environment and I would say, this issue actually is NOT a security hole (possibly even not a potential one). However I think it could be made more clean. In any case the Registration module should be fixed to prevent creating all-uppercase usernames.

[Sebastian Krysmanski]

I don't really understand what you're saying (partially because you didn't say what Trac version you're using; I'm using Trac 0.11.x).

Replying to d.nedelchev:

Or maybe just to allow only all-uppercase group-names (like the permissions itself)?

What do you mean by this? "Allow" where and what? And again: all-uppercase names are not group names. They are permission/action names. (Just have a look at the note on the bottom of the "Permissions" admin panel.)

The only issue I guess will be backward-compatibility with the existing non-all-uppercase group-names like "anonymous", "authenticated" and any custom group since now all-uppercase groups are prohibited in the admin-panel (as well as all-uppercase usernames).

How would this be an issue? An issue with what?

P.S. I've read in the comments this issue allows a user to obtain only permissions which already have. I've tried it on my local sandbox environment and I would say, this issue actually is NOT a security hole (possibly even not a potential one). However I think it could be made more clean.

I'm not sure how I could produce the problem with "developers" group in the first place. I tried to reproduce the problem today but was unsuccessful - even with the versions/revions noted in the patch. So I seems as if I made an error there (not sure why I came to my conclusion). Existing permission groups can't be "registered" - except for "authenticated" which may not be a "security hole" but a least a big problem as a bad user could bring down the whole Trac environment by exploiting this problem.

[Steffen Hoffmann]

According to my own investigations I've found as well, that the issue is only related to the "authenticated" group. Still it should be addressed better than now.

[Steffen Hoffmann]

#7575 provided a use case for the regexp username check suggested here, so it has been closed as a duplicate with reference to this ticket.

[Steffen Hoffmann]

(In [9247]) AccountManagerPlugin: Fix a small Python doc-string typo, refs #5295.

[Steffen Hoffmann]

(In [9260]) AccountManagerPlugin: Extend username checks before registration.

We've got some suggestions and even patches to improve checking for invalid usernames on registration attempts. Therefor checks

• against a list of reserved names (refs #5295)
• for colon corrupting HtPasswdStore (closes #4682)
• for characters '[' and ']' corrupting SvnServePasswordStore (closes #2630)
• for surrounding whitespace and removes it (closes #7087)

are added now. Thanks to all contributors, especially to manski, for exceptional help by reviewing tickets and bundling related issues.

[Steffen Hoffmann]

The last remaining part is a proposal for adding an admin-configurable regexp for valid usernames.

To properly keep track of this, I change the ticket to reflect this right now.

[Steffen Hoffmann]

Finishing the previously started changes.

[Mitar]

I am using following patch for some time now and it works great.

[Steffen Hoffmann]

Your latest patch is not based on current trunk and doesn't contain new information beyond the previous one done by manski. Thanks for taking care anyway.

Would you dare to test it with latest changes, that already add all improvements with exception of the username-whitelist-regexp feature, please? I'm looking forward to hear your experiences.

[Mitar]

I know, it is an old patch I made some time ago. It is just to explain what has been in production for some time now.

Currently I do not have any (non-production) system where I could test new improvements. I will be doing some new deployments in few months and I can test things then.

[Steffen Hoffmann]

See #8076 for some more proposed regexp to check for a valid email too.

[Mitar]

Username regex patch against r9785

[Mitar]

I have just attached a patch which adds a configuration option to limit allowed usernames with a regex. It is made against current trunk (r9785).

[Steffen Hoffmann]

Replying to mitar:

I have just attached a patch which adds a configuration option to limit allowed usernames with a regex. It is made against current trunk (r9785).

Thank you for the update on this ticket. At first glance it looks good. I'll have a second look and some testing, if this is not disturbing other things and fits into the overall concept. As we're accumulating more and more sanitizing and validation of various user input, I'm actually rethinking the current code structure. It might be good to do more modularization on this part for a gain in structure and overall readability.

[Mitar]

Could this be included into the core? This is one of two patches I am using in my Debian package.

[Steffen Hoffmann]

Thanks for the reminder. I did hold it back by now for a reason. I stopped adding more checks after some really critical ones, to have a chance for re-thinking the whole approach.

Meanwhile I'm determined to add a more configurable, flexible way of including additional checks defining a standard check interface (see #2897 and #7577 for details). I might even want to rework existing checks to use that interface.

And I'd like to use ordered extensions by means of the OrderedExtensionOption, or would you consider arbitrary, non-deterministic sequences of check fields an additional feature? Personally I don't think so.

[Mitar]

I think OrderedExtensionOption is a good idea.

[Mitar]

Any progress on this? For example regex support could be used to enforce using e-mail addresses as usernames.

[Steffen Hoffmann]

Replying to mitar:

Any progress on this? For example regex support could be used to enforce using e-mail addresses as usernames.

Finally, yes, I've made this a part of current t-h.o migration planning.

[Steffen Hoffmann]

(In [11829]) AccountManagerPlugin: Allow for optional case-less username duplicate checking, refs #5295.

Option username_dup_case enables more thoroughly testing for a username duplicate when validating new usernames.

This is the implementation of an idea, how to improve user registration checks at trac-hacks.org, mentioned by Ryan Ollos.

[Steffen Hoffmann]

(In [11839]) AccountManagerPlugin: Make case-less username duplicate checking mandatory, refs #5295.

The longer I thought about it, the more I've been convinced, that a username differing only by character case is spelling trouble for any environment. I remembered having such problems when switching ignore_auth_case to True for a Trac instance some time ago and couldn't think of a use-case anyway, so I made case-less checks mandatory.

Even better, pulling the obsoleted new option now is fighting feature bloat: With 25+ options AccountManager is not exactly easy to configure, so every dispensed option is a blessing.

Additionally I hope, that a highlighted username improves error messages.

[Mitar]

Yes. I don't like how ignore_auth_case works in Trac, where it forces lower case. ignore_auth_case should still be case preserving. Just that it does not matter the case when authenticating.

I think this should be how account manager should also behave. Everything else is just a problem waiting to happen.

Also Trac permission system, which sees groups and users as same, so if some user register with an username same as a group name ...

It is not just enough to prevent registration in this case. Because for example, in one my case, I have external authentication source. And that source do not have access to Trac groups to be able to prevent registration. So I just hope nobody will notice that.

There should be some better system solution to this.

[Steffen Hoffmann]

(In [11917]) AccountManagerPlugin: Add interface for configurable user registration procedure, refs #874, #2707, #2897, #4651, #5295, #7577, #8076.

The current user registration process lacks flexibility, as can be witnessed by the history of one of the oldest still pending tickets for this plugin: #874.

[Steffen Hoffmann]

(In [11923]) AccountManagerPlugin: Finish registration check rewrite, refs #874, #2707, #2897, #4651, #5295, #7577, #8076.

Now moving check execution into AccountManager, and _create_user() is gone, replaced by more clearly structured and modularized code.

Registration checking for requests from the admin panel is re-activated and minor improvements related to RegistrationError processing done too.

Note: The test, that detected admin requests to skip additional checking of existing permissions for each new username, has been done differently. In the future each check has to decide on its own, like this is done in the UsernamePermCheck now.

[Steffen Hoffmann]

Replying to mitar:

Yes. I don't like how ignore_auth_case works in Trac, where it forces lower case. ignore_auth_case should still be case preserving. Just that it does not matter the case when authenticating.

While I see your point, this would be exceptionally cumbersome in reality, because IPasswordStores are not at all designed to guess the username in any way. So we would need to harvest - potentially multiple - store(s) for all available users on every authentication request/login, just to be able to pass the password to every candidate. Do you think this wont be critical performance-wise?

[Steffen Hoffmann]

(In [11960]) AccountManagerPlugin: Add configurable regular expression registration checks, refs #5295 and #8076.

Credits

• for the email check part: suggested and drafted by T. Christ
• for the username check part:
  • suggested and darfted by Sebastian Krysmanski
  • further developed by M. Mitar

Thank you all for inspiring contributions.

I release this in the hope, that it's gonna get better than just the sum of the single patches. Surely it will, because we're using flexibility provided by the IAccountRegistrationInspector interface now. And while case-insensitive matching was deemed preferable this is no longer hard-coded but added to Python regexp by prepending (?i), so you've really full control over your new configurable regexp-driven checks.

And now README.update has caught up with recent changes and new features too.

[Steffen Hoffmann]

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.