search

imsweb / django-saml-sp

A Django application for running one or more SAML service providers (SP)
BSD 3-Clause "New" or "Revised" License
15 stars 10 forks source link

NameID spoofing vulnerability #8

Closed thefedoration closed 2 years ago

thefedoration commented 3 years ago

There's a potential attack vector that would enable a user to log into another user's account.

If the setup of an IdP is possible in the application UI to certain users (and not just staff users in Django admin), a user could set up an IdP that has the ability to spoof an existing user's nameid and log into an existing user's account. This can be done with a test IdP like Samling.

I can see 2 ways that this can be avoided:

  1. Upon SAML auth, a database object (SAML Profile) is created that associates a user with an IdP. If a SAML Request comes in to log a user into an existing account, user needs to have an existing SAML profile to that Idp.
  2. Enabling a setting to configure a SAML user's username instead of just using the nameid, like get_saml_username(idp, user) so that application owners can prefix usernames with the IdP slug, and ensure that a SAML authentication request can only log a user into an account that was created with the same SAML IdP.
thefedoration commented 3 years ago

@dcwatson @ruttenb any thoughts on this security vulnerability?

dcwatson commented 3 years ago

Thanks for the report. I like your second solution, probably with an additional setting to allow for automatic username prefixing. What I'll probably do is split out the username generation (or perhaps even just a get_user) into a method of the SAMLAuthenticationBackend authentication backend, then update it to use a SP_USERNAME_PREFIX setting. Whether that's a simple function or a True/False, I'm not sure yet. Then you can also easily just override the backend to suit your own needs.

For now, if this poses a problem, you can use your own authentication backend to do this.

thefedoration commented 3 years ago

That sounds like a good implementation. Sounds good, I'll handle on my side for now. If you could update this issue when you make enhancement, that would be great. Thanks!

dcwatson commented 3 years ago

This is going to be part of a larger update. But for this issue specifically, there are now a number of new settings:

  1. SP_UNIQUE_USERNAMES - always generate usernames (for new users, or when looking up users to associate with an IdP) keyed to the IdP that authenticated them.
  2. IdP. associate_users - similar to IdP.create_users, specifies whether unassociated users (i.e. Django users that exist but have not logged in via an IdP yet) will be automatically looked up via username and associated upon SSO login.
  3. IdP. username_prefix and IdP. username_suffix - lets you specify custom username prefixes and suffixes for an IdP. Could be used in addition to, or in place of SP_UNIQUE_USERNAMES

I also added a model to associate an IdP with Django users via a nameid. This is a piece I had been handling separately in my own use, but makes sense to have built in.

dcwatson commented 2 years ago

Going to close this out, as I'll be releasing 0.5 shortly.