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

sso

(Felix Freiberger) #3

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 Saffron) #4

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


(Simon Cossar) #5

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.


(Felix Freiberger) #6

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

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?


(Jeff Atwood) #8

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


(Sam Saffron) #9

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


(Felix Freiberger) #10

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


(Jeff Atwood) #11

Maybe @sam should add some tests for it?


(Rysher) #13

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


(Sam Saffron) #14

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


(Sascha Hlusiak) #15

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.

  1. Change the email address to some other existing user, sign on using SSO, it will update the external_id of that user.
  2. 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.


(Dean Taylor) #16

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


(Sascha Hlusiak) #17

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.


(Felix Freiberger) #18

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?


(Sascha Hlusiak) #19

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) #20

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) #21

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 Saffron) #22

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


(Sam Saffron) #23