[patch] Add optional username regexp to registration checks

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

Patch against r5836 and Trac 0.11.4

A workaround for this problem is:

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

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.

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

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.

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).

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".

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.

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.

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

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.

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.

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).

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.

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.

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.

#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.

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

(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.

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.

Finishing the previously started changes.

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

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.

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.

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

Username regex patch against r9785

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).

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.

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

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.

I think OrderedExtensionOption? is a good idea.

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