Discourse doesn't re-verify an address changed by SSO

fefrei · July 8, 2015, 1:20pm

Correct, this is not about the default behavior. Normally, Discourse trusts the E-Mail address provided. If the SSO payload sets require_activation=true (which shouldn’t be the case for you), it doesn’t trust the address for signups (which is intended), but trusts it for address changes (which this report is about).

sam · July 8, 2015, 1:22pm

you got to opt in with require_activation=true it’s a legit edge case of a bug

Simon_Cossar · August 17, 2016, 1:48am

I’m running into this when sso overrides email is enabled on Discourse and a user changes their email address on a WordPress site. How big of a security risk is this?

Adding user.active = false to DiscourseSingleSignOn#change_external_attributes_and_override for the case when the user’s email doesn’t match the email sent with the sso request forces the user to confirm their new email address. Unfortunately it does this by redirecting them to users/account_created, so it’s a bit of a hacky solution.

Edit: the problem is that WordPress will let a user change their email without verifying it. The wp-discourse plugin attempts to verify the email on registration, and if it can’t it sets require_activation to true. Possibly the simplest solution is to recommend that sites using the plugin should not enable sso overrides email.

fefrei · August 17, 2016, 6:18pm

I haven’t tested this, but…

If there is an “unclaimed” developer email address in app.yml (rather unlikely to happen), this could allow any user to gain administrative rights.
It allows spamming, because you can change your address to an address you want to spam.
It might allow taking over a staged account if changing the email address merges the accounts, but I’d assume it doesn’t (and errors out instead).

I don’t see any other risk right now, but user identity is strongly tied to the email address in Discourse, so it’s hard to estimate the impact.

Simon_Cossar · August 17, 2016, 10:45pm

A proper fix for this seems like it could be time consuming. Would it be possible to change the setting’s description to include a warning to not enable it unless the email address is verified by the SSO provider? Or even more specifically, to not enable it with the wp-discourse plugin?

codinghorror · August 17, 2016, 10:55pm

Absolutely, it should have a huge warning, with details.

sam · August 17, 2016, 11:19pm

Sure this is a bug we should fix. I need to add some tests for it.

fefrei · February 9, 2017, 8:12pm

Just a small update, because I was asked via PM:

The bug is still open in the latest beta release, v1.8.0.beta4 +284 (9fa29ca89815878f768885f7413b8beaae3a414c).

codinghorror · February 9, 2017, 9:25pm

Maybe @sam should add some tests for it?

rysher · April 25, 2017, 6:44pm

Did @sam add some tests for it? It seems that this issue still exists.

sam · April 25, 2017, 10:26pm

It is on my list, I have not addressed anything here yet.

sascha · April 28, 2017, 1:28am

I’d like to vote for this as well. Our IdP currently allows users to change their email address without verification, which in 2 steps allows a user to completely hijack an existing Discourse user.

Change the email address to some other existing user, sign on using SSO, it will update the external_id of that user.
Change the email address back, now the external_email of that user is updated as well.

Does it make sense to provide a way of disabling 1)? The external_id of a user should be immutable and the email address should not be used to look up users?

And of course, updating a users email address in Discourse with require_activation=true should send an activation email for that address.

DeanMarkTaylor · April 28, 2017, 2:00am

This goes against the requirements set out in the SSO topic:

sascha · April 28, 2017, 2:58am

If Discourse updates the external_id of a user based on their email address, isn’t that violating the requirement itself that external_id will never change? It’s effectively merging two separate external users into one discourse user if their email address is the same.

fefrei · April 28, 2017, 9:07am

Did you verify that this actually happens? (By my amateurish understanding of the code, it does.)

If it does, I think this is a (separate) security-relevant #bug – while Discourse should match users by email if the external_id is unknown, it should not find a user that already has a different external_id and update it.

Maybe this code should back out with an error if user already has a (different) external_id set?

github.com

discourse/discourse/blob/f34907b5235b435a61a6eafd51c06831356e6f16/app/models/discourse_single_sign_on.rb#L131


    split = remove_groups.split(",")
    if split.length > 0
      GroupUser
          .where(user_id: user.id)
          .where('group_id IN (SELECT id FROM groups WHERE name in (?))',split)
          .destroy_all
    end
  end
end


def match_email_or_create_user(ip_address)
  unless user = User.find_by_email(email)
    try_name = name.presence
    try_username = username.presence


    user_params = {
      email: email,
      name: try_name || User.suggest_name(try_username || email),
      username: UserNameSuggester.suggest(try_username || try_name || email),
      ip_address: ip_address
    }

sascha · May 4, 2017, 6:20am

This is actually what happens in my installation, the documentation / official SSO howto mentions this. I believe this is contradicting the aspect before that external_id is immutable, if Discourse updates the external_id, if an existing user is found by email address. While this could be desirable because it makes moving users between external systems easier, I see the security risk here as it allows an external system to both change the external_id as well as the email address of existing users, completely disconnecting it from the original external system.

I don’t know the motivation behind this behavior though and do not know if failing in the case that the external_id is already populated is desirable. After all the external fields are not editable by administrators in the front end and thus it would not be (easily) possibly to rectify a misconfiguration.

rysher · May 7, 2017, 10:51pm

It is definitely a critical security issue, if sso overwrites external_id, it shouldn’t. But as I understand the code, it should set external_id via match_email_or_create_user only if there have been no user with such external_id found on sso login:

  def lookup_or_create_user(ip_address=nil)
    sso_record = SingleSignOnRecord.find_by(external_id: external_id)

    if sso_record && (user = sso_record.user)
      sso_record.last_payload = unsigned_payload
    else
      user = match_email_or_create_user(ip_address)
      sso_record = user.single_sign_on_record
    end
...
end

I’ll be able to check the behavior on my local setup tomorrow.

rysher · May 7, 2017, 11:40pm

I’ve checked it and confirm that external_id can be overwritten this way. And in combination with this topic’s issue an account can be hi-jacked this way. I agree with @fefrei that it should be added as a separate security-relevan #bug.

sam · May 17, 2017, 3:10am

This has taken quite a while to get to, but I think this sorts it out correctly. Be sure to read the test.

sam · May 16, 2017, 8:20pm