Security: Add a mandatory external unique id field to pluggable auth API

[thegcat]

The current API for pluggable authentication is easily prone to account hijacking, either if the authentication source allows changing email addresses and/or if the authentication source doesn't enforce email address uniqueness.

The 2 authentication plugins we have surveyed (LDAP Auth and OIDC Auth) are unfortunately both subject to this vulnerability. We are in the process of opening issues with the authors of those plugins to make them aware of this issue.

The general fix (also applicable to both those cases) would be to store a known unique and non-modifiable attribute returned by the upstream identity provider alongside the user's email address in the Pretix User object, to store this when the authentication plugin creates a user from an external identity source, and to compare it on subsequent login attempts.

Pretix should offer such a field on the User object so that not every authentication plugin has to implement their own.

I would also suggest that the pluggable authentication API be changed to make such a unique and non-modifiable upstream identity provider attribute mandatory. This would greatly improve the security of auth plugins by alleviating the whole class of email address base account hijacking by default. Auth plugins that are happy and sure the email address is immutable and unique in the identity provider could still throw in the email address in said field, or even always the same hardcoded value for every user, but it would be a conscious decision by the plugin developer and not a potentially insecure default.

I would furthermore suggest this unique and non-modifiable upstream identity provider attribute be made mandatory by the Pretix API such that plugin authors have to adhere by it. For example the docs makes the requirement that an auth plugin only authenticates users it created itself, the OIDC plugin does not check that (I would argue this requirement should also be checked by Pretix code and not left to auth plugin authors, but that's another issue).

[thegcat]

Regarding that last point I misread the docs, the docs only say a login should be refused if the user was not created by the auth plugin, sorry.

[thegcat]

Though the requirement the auth plugin must set the auth_backend to itself when returning a user but should refuse logging in users not associated with their auth plugin is a bit confusing. This effectively means that an auth plugin that does not refuse to log in users associated with other auth plugins will "greedily" accrue users that log in with it?

[raphaelm]

Yes, accruing users is not how this is intended. I'm not sure if there's ever a reason to allow cross-backend usage of user accounts or if we should just consider that forbidden in the docs (we probably cannot technically enforce that, though).

[raphaelm]

🚨 First of all, mostly for anyone else reading this and having similar thoughts, if you find security-sensitive issues in pretix or its ecosystem, please do not open a public issue or discussion, but coordinate with security@pretix.eu first so we can figure out if this is something we need to fix very very quickly before it gets publicly known. This case is on the very edge of this distinction and I'm not sure where it should have fallen, but now here we are :)

I think this one is only directly exploitable for account takeover if the authentication backend source contains unverified/untrusted email address assignments. Since this is mostly used in contexts where the authentication backend is a company's central identity storage, this should be a very rare case.

Pretix should offer such a field on the User object so that not every authentication plugin has to implement their own.

I agree.

I would also suggest that the pluggable authentication API be changed to make such a unique and non-modifiable upstream identity provider attribute mandatory.

I don't think that's something we can easily do without a long deprecation period. Mostly because if we do this change, then every plugin will need to find its own way of filling up the identity attribute for all existing users. Possibly this will require all users to log in at least once after the change to work. (I'm also not sure if every possible identity service provides this, but in that case you could still fall back to using the email address as well).

For example the docs makes the requirement that an auth plugin only authenticates users it created itself, the OIDC plugin does not check that (I would argue this requirement should also be checked by Pretix code and not left to auth plugin authors, but that's another issue).

I don't think we technically cannot really enforce that, since the plugins just work with plain django objects and django authentication APIs.

[thegcat]

🚨 First of all, mostly for anyone else reading this and having similar thoughts, if you find security-sensitive issues in pretix or its ecosystem, please do not open a public issue or discussion, but coordinate with security@pretix.eu first so we can figure out if this is something we need to fix very very quickly before it gets publicly known. This case is on the very edge of this distinction and I'm not sure where it should have fallen, but now here we are :)

I had thought about that. As this requires intervention from plugin authors and probably is an edge case I'm not even sure some plugin authors might be interested in fixing, I decided it'd be better to have this open from the start. To be honest I wasn't even sure this would be something Pretix core would be interested in solving, I'm happy to be wrong on this 👍

Should there be a next time I'll try to reach out so that we can work this out on your terms, sorry for that.

I would also suggest that the pluggable authentication API be changed to make such a unique and non-modifiable upstream identity provider attribute mandatory.

I don't think that's something we can easily do without a long deprecation period. Mostly because if we do this change, then every plugin will need to find its own way of filling up the identity attribute for all existing users. Possibly this will require all users to log in at least once after the change to work. (I'm also not sure if every possible identity service provides this, but in that case you could still fall back to using the email address as well).

For example the docs makes the requirement that an auth plugin only authenticates users it created itself, the OIDC plugin does not check that (I would argue this requirement should also be checked by Pretix code and not left to auth plugin authors, but that's another issue).

I don't think we technically cannot really enforce that, since the plugins just work with plain django objects and django authentication APIs.

I'm not versed enough in the whole Python/Django Ecosystem, but I'd have thought something along the lines of "external auth plugins must inherit from this Pretix Core class which expects this method to return an email address and an external identifier, and our code will take of the rest". Sure, the dynamic nature of the language (and the Open Source nature of Pretix) doesn't guarantee that every auth plugin follows this, but it would probably be a high enough technical hurdle (either plugging into the Django auth without going through the pretix.cfg or monkey-patching the Core class) that only people with enough knowledge and understanding of the reasons for those requirements can circumvent them.

I'm happy with any solution Pretix core can provide that avoids NIH and potentially insecure and/or slow to update solutions in each plugin. Thank you for the quick response on this.

[raphaelm]

I had thought about that. As this requires intervention from plugin authors and probably is an edge case I'm not even sure some plugin authors might be interested in fixing, I decided it'd be better to have this open from the start.

For plugins listed on marketplace.pretix.eu, we have contact information that we could use to reach out to the authors, or we might help with creating patches if the plugins are widely used or used by pretix enterprise customers, etc.

Again, I think there's no harm done here, but I want to be clear for future cases (also from other people reading this): Email first is the way to go :)

I'm not versed enough in the whole Python/Django Ecosystem, but I'd have thought something along the lines of "external auth plugins must inherit from this Pretix Core class which expects this method to return an email address and an external identifier, and our code will take of the rest".

I think the proposed PR is a sensible middle-ground, it does require plugins to change something now (which is unavoidable due to the nature of the current code design), but it makes it easier to do it right than do it wrong in the future.

[raphaelm]

Here's a proposal for a fix, happy for feedback: #2466