If a SSO provider sets require_activation=true, users must confirm their E-Mail address after they first sign in with SSO.
When a user changes his E-Mail address in a non-SSO setting, he must verify his address before the change is saved.
When the setting sso overrides email is enabled, and a user changes this E-Mail address in the external system, his address is changed in Discourse without verification, even with require_activation=true. I think that this is inconsistent. I’d expect that in this setting, the account is disabled whenever the address changes, requiring the user to verify his new address.
Discourse trusts our SSO provider (user directory) enough to authenticate users, so why shouldn’t it also trust that system to validate users’ email addresses?
Our users would become extremely annoyed if they had to validate their email addresses twice on each change.
OK, maybe I misunderstood. I just hope this is not the default behavior.
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).
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.
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?
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.
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.
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?
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.
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.
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.