Modify

Opened 8 years ago

Closed 4 years ago

Last modified 3 years ago

#831 closed defect (fixed)

[patch] Case sensitive Authentication, but Case in-sensitive Authorization.

Reported by: andrew.krock@… Owned by: hasienda
Priority: high Component: AccountManagerPlugin
Severity: critical Keywords: auth permission
Cc: trac-hacks@…, snoopotic@…, mmitar@… Trac Release: 0.11

Description

I recently had an issue with my trac install and one of my programers. I am useing the following plugins:

  • WebAdmin (from the trac website)
  • TracAccountManager? (from trac-hacks)

My new programmer complained that he did not have the adequate permissions that he should have, so I created a test account named "test", and added it to the same permission-groups as that programmers account. All the permissions were set properly, the problem came from the fact that his account name contained uppercase letters. Creating an account called "TEST" and not enabling any permissions, gave me all the permissions assigned to the acount "test". To the login system "TEST" and "test" are completely different, however to the authorization (permission) system "TEST" and "test" are the exact same accounts, and furthermore will only apply the permissions set to the account "test" to both accounts when logged in.

This is a pretty big security hole in the AccountManagerPlugin System, as anyone can register an account using any combination of uppercase letters for any of your users or even permission groups.

This is pretty severe, I don't wish to have to add permissions for every combination of my admin accounts just to prevent anyone from hacking into my trac.

Attachments (2)

ignore_auth_case.r5836.patch (2.8 KB) - added by manski 5 years ago.
Patch against r5836 and Trac 0.11.4
username_case.patch (4.3 KB) - added by mitar 3 years ago.
Patch which allows case preservation, but still case insensitive authentication, against r10250

Download all attachments as: .zip

Change History (26)

comment:1 Changed 8 years ago by mgood

  • Keywords needinfo added

What is the value of "ignore_auth_case" in your trac.ini? The default is "false" in which case the Trac permission system will be case-sensitive, since this is normal for common authentication setups. I believe that option was added due to browsers being inconsistent about casing with NTLM authentication. So, if you've set it to "true" please set it back to "false" so that Trac uses it's normal case-sensitive behavior.

comment:2 Changed 8 years ago by mgood

If this is just a case of accidentally enabling "ignore_auth_case" I can update the registration component to disable registration and log that the setting needs turned off to prevent possible security issues.

comment:3 Changed 8 years ago by mgood

In #155 I checked that new users couldn't register an account that received any existing permissions beyond what all authenticated users had. It looks like my patch for that didn't take into account the possibility of "ignore_auth_case". So, I could account for it there, but it does seem safer to simply enforce case-sensitive user accounts.

comment:4 follow-up: Changed 8 years ago by andrew.krock@…

your right, it was ignore_auth_case set to true. Sorry, I didn't think of that, you might wish to patch it to either enforce ignore_auth_case to be disabled or to not allow users to register names w/ different case if auth_case is enabled... To protect idiots like myself from doing things like this.

comment:5 Changed 8 years ago by andrew.krock@…

Yup changed ignore_auth_case, and once again im all safe, secure, and cozy.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by ThurnerRupert

Replying to andrew.krock@gmail.com:

your right, it was ignore_auth_case set to true. Sorry, I didn't think of that, you might wish to patch it to either enforce ignore_auth_case to be disabled or to not allow users to register names w/ different case if auth_case is enabled... To protect idiots like myself from doing things like this.

this would be great! we have case insensitive set, and the users regularly fall over that by registering a different case user and then wondering why login is not possible.

comment:7 in reply to: ↑ 6 Changed 8 years ago by mgood

  • Keywords needinfo removed

Replying to ThurnerRupert:

this would be great! we have case insensitive set, and the users regularly fall over that by registering a different case user and then wondering why login is not possible.

All the authentication methods supported by the AccountManager are case sensitive, so there's no reason to enable this setting, and as Andrew pointed out it may actually lead to vulnerabilities. The patch I'm going to commit will disable registration until ignore_auth_case has been disabled.

comment:8 follow-up: Changed 8 years ago by mgood

  • Resolution set to fixed
  • Status changed from new to closed

(In [1535]) disable registration if ignore_auth_case is true to prevent permission hijacking (fixes #831)

comment:9 follow-up: Changed 8 years ago by ThurnerRupert

  • Resolution fixed deleted
  • Status changed from closed to reopened

we use this setting to switch later on to a company login system which is case insensitive. it should not be that a plugin invalidates a valid trac setting, isn't it? why the registration module just does not respect the ignore_auth_case setting and makes the username lowercase on registration, like the rest of trac is doing?

comment:10 in reply to: ↑ 9 Changed 8 years ago by anonymous

Replying to ThurnerRupert:

we use this setting to switch later on to a company login system which is case insensitive. it should not be that a plugin invalidates a valid trac setting, isn't it? why the registration module just does not respect the ignore_auth_case setting and makes the username lowercase on registration, like the rest of trac is doing?

and login maybe too.

comment:11 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by anonymous

Replying to mgood:

(In [1535]) disable registration if ignore_auth_case is true to prevent permission hijacking (fixes #831)

I'm sorry, but this is bad behavior for a plugin to take. Trac's case-insensitivity setting should not make portions of this plugin not function. This plugin should, as other posters have mentioned, simply lowercase the username as Trac does in all other instances when case-insensitivity is enable. This would simplify everything and, frankly, is not difficult to implement.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by mgood

Replying to anonymous:

I'm sorry, but this is bad behavior for a plugin to take. Trac's case-insensitivity setting should not make portions of this plugin not function.

Not if that feature poses a security risk.

This plugin should, as other posters have mentioned, simply lowercase the username as Trac does in all other instances when case-insensitivity is enable. This would simplify everything and, frankly, is not difficult to implement.

No, the security hole still exists if the names are simply converted to lowercase. This would be ok if you were starting with a new password file, but there could be problems if you have existing users registered with mixed-case names. The password store backends provided with this plugin are all case-sensitive, so I don't think it's wise to mix them with the ignore_auth_case setting. If you'd like to provide a patch that makes sure that ignore_auth_case is handled in a safe way I'll be glad to look at it, but for right now I'd rather disable this than leave open a potential security hole.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by anonymous

Replying to mgood:

This plugin should, as other posters have mentioned, simply lowercase the username as Trac does in all other instances when case-insensitivity is enable. This would simplify everything and, frankly, is not difficult to implement.

No, the security hole still exists if the names are simply converted to lowercase. This would be ok if you were starting with a new password file, but there could be problems if you have existing users registered with mixed-case names. The password store backends provided with this plugin are all case-sensitive, so I don't think it's wise to mix them with the ignore_auth_case setting. If you'd like to provide a patch that makes sure that ignore_auth_case is handled in a safe way I'll be glad to look at it, but for right now I'd rather disable this than leave open a potential security hole.

the case is much more simple:

  • if a user feel its a security hole, he does not set ignore_auth_case, or converts all the usernames to lowercase, or lets re-register everybody.
  • if a user sets ignore_auth_case, he expects it to work throughout trac.
  • so this is "separation of concerns". authmanager plugin is not responsible for this, but trac.
  • the password stores have to be case sensitive. otherwise it would be a real security problem.

comment:14 Changed 6 years ago by trac-hacks@…

  • Cc trac-hacks@… added

ccing

comment:15 in reply to: ↑ 13 Changed 6 years ago by pacopablo

  • Owner changed from mgood to pacopablo
  • Status changed from reopened to new

Replying to anonymous:

I'm of the same mind as anonymous. Having the registration module be disabled if you have ignore_auth_case on is stupid. If my users backend into any type of MS system, case-insensitive auth is expected. Also, if account manager simply lowercases all of users then there is no security hole as you won't be able to create multiple accounts with differing case.

As far as the existing users issue, I think documentation and a bit of manual intervention is acceptable.

I'll whip up a patch when I get a chance. If I don't hear any objections, I'll commit it. Please speak-up if you feel that this is an undesirable change

comment:16 follow-up: Changed 5 years ago by anonymous2

  • Trac Release changed from 0.9 to 0.11

I have the same problem:

If a user logs in with "test" everything works as it is supposed to.

If he is using "Test" instead he gets logged in and even can see/edit the perfernces of "test" but he doesn't have his rights. So for example the WIKI page returns an error about no rights.

ignore_auth_case is set to "false".

So the auth function from this plugin seems to ignore the case and lets the user in, but trac itself doesn't give him the rights.

Changed 5 years ago by manski

Patch against r5836 and Trac 0.11.4

comment:17 follow-up: Changed 5 years ago by manski

I've added a patch to fix this problem. (For some reasons the Trac's patch view doesn't display the changes to api.py; but they're there if you download the original format).

comment:18 Changed 5 years ago by anonymous

  • Cc snoopotic@… added

does the patch also apply for 0.11.5?

comment:19 in reply to: ↑ 16 Changed 4 years ago by hasienda

  • Keywords needinfo auth permission added
  • Owner changed from pacopablo to hasienda
  • Summary changed from Case sensitive Authentication, and Case in-sensitive Authorization. to Case sensitive Authentication, but Case in-sensitive Authorization.

Replying to anonymous2:

I have the same problem:

If a user logs in with "test" everything works as it is supposed to.

If he is using "Test" instead he gets logged in and even can see/edit the perfernces of "test" but he doesn't have his rights. So for example the WIKI page returns an error about no rights.

ignore_auth_case is set to "false".

So the auth function from this plugin seems to ignore the case and lets the user in, but trac itself doesn't give him the rights.

Did anyone check with Trac 0.12? Always been on 0.12 I can't reproduce this behavior right now. Could it be, this issue is limited to Trac <= 0.11 somehow?

comment:20 in reply to: ↑ 17 Changed 4 years ago by hasienda

  • Keywords needinfo removed
  • Priority changed from highest to high
  • Severity changed from blocker to critical
  • Status changed from new to assigned
  • Summary changed from Case sensitive Authentication, but Case in-sensitive Authorization. to [patch] Case sensitive Authentication, but Case in-sensitive Authorization.

Replying to manski:

I've added a patch to fix this problem. (For some reasons the Trac's patch view doesn't display the changes to api.py; but they're there if you download the original format).

I've prepared a slightly modified version of your patch for local testing. If all goes well, I'll commit it before the weekend.

Thank you for another great contribution to a highly disputed issue, while others did just raise concerns.

BTW, I feel sad about missing Trac-compliance in such a critical area, but this is at least partially due to ineffective discussion by really capable developers, and I feel we have an exceptionally negative example within this ticket. Wake-up guys, as a matter of fact this can't survive here as a 'blocker' for 4(!) years now, if you're serious about anything else you said here.

comment:21 Changed 4 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [9286]) AccountManagerPlugin: Allow for sane registration and username handling even with case-insensitive authentication, closes #831.

Finally allow using the Trac option ignore_auth_case = true.

comment:22 Changed 3 years ago by hasienda

(In [10521]) AccountManagerPlugin: Compare to unchanged user list, refs #831.

Initial implementation by changeset [4638] led to false-positives and DOS,
because the password check has always been done against authentication stores
without changing their username casing.

comment:23 Changed 3 years ago by mitar

  • Cc mmitar@… added

OK, I must say that this is the most unbelievable behavior. Why would authentication be case sensitive in the first place? Authentication should be case preserving (username is in the case the user registered it) but case insensitive (any case combination used for login should map to only one user). It is crazy that tEST nad Test are two users.

And authorization should then be linked to this one user (it can internally use lower case as a database key, if this is needed, or just preserving the case).

Of course how to do that when Trac itself mangles with names is problematic. But maybe we could simply do not allow registration with the same username when an user with the same username (but case insensitively) already exist.

And when login, we could try to get original case from the password backend and authenticate with that (or ask password backend itself to authenticate in the case-insensitive manner). But at the end the case should be that of the registered user, even if the user has specified different case when login.

Changed 3 years ago by mitar

Patch which allows case preservation, but still case insensitive authentication, against r10250

comment:24 Changed 3 years ago by mitar

I have attached a patch against a bit old revision. I have extended API a bit so that password store can return proper username when user logs in.

Also I found two bugs:

  • False value returned from password stores did not break the traversal of password stores, but was handled the same as None, this is in conflict with documentation
  • In _create_user function, name and email was stored into session with values from request, not with values which were checked, for example through EmailVerificationModule

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.