Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force users to login to both services before merging #3

Open
zimme opened this issue Oct 22, 2015 · 11 comments
Open

Force users to login to both services before merging #3

zimme opened this issue Oct 22, 2015 · 11 comments

Comments

@zimme
Copy link
Contributor

zimme commented Oct 22, 2015

Security concerns, brought up by @brettle.

Where do we draw the line on trusting the providers?

This will give the user the control of trust instead of the app developer.

@brettle
Copy link

brettle commented Oct 23, 2015

First by way of background for others, here is my security concern. Consider the following attack scenario:

The app supports login with either the Giggle service or the LinkedOut service. User has a Giggle account and logs into the app using his Giggle account which provides his verified email addreess of [email protected]. User does not want to allow LinkedOut to authenticate him to the app. Perhaps user has a weaker password on his LinkedOut account. Perhaps user doesn't trust LinkedOut for some reason. Perhaps user doesn't have a LinkedOut account and doesn't plan to get one. Attacker guesses users weak LinkedOut password or, due to a LinkedOut vulnerability, manages to otherwise break into user's LinkedOut account (or create a new one) with "verified" email address of [email protected]. Attacker signs in to app using that LinkedOut account and, because it has the same verified email address as the user's Giggle account, this package logs the attacker into the user's app account. When the user signed up with the app using his Giggle account, he did not realize (and had no reason to expect) that his app account security was in any way related to the security of his LinkedOut account. As a result, the app has betrayed the user's trust.

Note that this is not about which providers the package should trust or which providers the app developer trusts. It is about which providers the user trusts. A provider may be perfectly secure, but the user might still not trust it to authenticate him to the app because he knows that the password (or other means) he uses to authenticate with the provider is weak.

Also note that fixing this would not preclude the app developer from providing a means for an administrator to merge accounts that the admin has determined (hopefully via some secure means) actually belong to the same person. The issue with the current implementation is that it provides a way for an attacker to initiate such a merge.

@zimme
Copy link
Contributor Author

zimme commented Oct 23, 2015

Where do we draw the line between security and convenience? Having perfect security isn't always easy when you wanna make your site usable to the masses?

@brettle
Copy link

brettle commented Oct 23, 2015

As I'm sure you know, one major rule of usability is the "rule of least surprise". In the security context, being vulnerable to some attacks is more surprising than others. For example, when I sign up for an app using my email address and a password, I am not at all surprised that an attacker who can access my email account (or intercept messages to it) can reset my password and access my app account. I'm also not surprised that an attacker who gains access to the app's database can access my hashed password. In fact, those are two reasons that I might prefer to login with an OAuth service where I have a strong password (and/or additional security precautions). However, if I used such a service to sign up for an app, I would be quite surprised if someone else logged in to my app account using an account I have on a different service.

Of course, what is surprising in one context might not be surprising in another. But since this is a package, we know very little about the context of the app which is using, and it is better to err on the side of more security even if that means less convenience. It's also worth pointing out that logging in is usually a relatively rare event in terms of the user's overall interactions with an app. So the convenience sacrificed in the name of security is arguably less important here than in most other places.

That said, I don't think this needs to be a particularly big inconvenience. If the UI distinguishes between signing up and signing in, I think a user will be much less likely to accidentally create multiple accounts using different services. He will probably remember that he already has an account, but might not be sure which service he used. When he tries to sign in with the wrong service, it fails (instead of creating a new account) and he tries again with a different service. And if he wants to be able to login with multiple services, IMO it's not particularly inconvenient to login with one service and then add others while logged in.

Lastly, if the app developer wants to completely ban multiple accounts with the same email address (with the knowledge that doing so necessarily allows an attacker to determine which email addresses already have accounts), he can register a validateNewUser handler that checks for that.

Hope all that makes sense.

@zimme
Copy link
Contributor Author

zimme commented Oct 23, 2015

It makes sense, but the big problem here is that this would need changes to core packages functionality, as .loginWith<service> creates a new account if one doesn't exist.

We could of course override that behaviour with this package and the useraccounts system, but then that needs to be VERY clear as we're altering default behaviour of the framework provided account services.

Another option would be to make our own forks of the oauth flow and have that behaviour be different and more secure than the core packages.

@brettle
Copy link

brettle commented Oct 24, 2015

We could throw a LoginCancelledError (and delete any newly created account) after adding a service to the logged in user's account. The error causes the user to remain logged in to their current account. That is what brettle:accounts-add-service does.

@zimme
Copy link
Contributor Author

zimme commented Oct 24, 2015

If you're logged in with an account and you login with a provider, then one-account will add that service to the currently logged in account if and only if there isn't another account connected to this service account. If that service was added, then we just let the login go though, to get the lastest data from the service account, it won't create a new account.

@brettle
Copy link

brettle commented Oct 24, 2015

I think I got confused as to what we are talking about. I'm now realizing that when you said "this would need changes to core packages", the "this" you were referring to was probably my suggestion that "the UI distinguishes between signing up and signing in". Is that right? If not, please clarify. Thanks!

@zimme
Copy link
Contributor Author

zimme commented Oct 24, 2015

It's the register vs login thing I was referring to.

Today with Meteor.loginWith<service> it tries to login with an existing account or it creates a new one if no existing account is found.

Today you can change accounts when you're already logged in by logging in to another account.

I think we can change this behaviour with our package but we need to be very clear in our documentation that this is different from the usual behaviour of the accounts system in meteor.

I think that Meteor.loginWith should only work when you're not logged in or when the account you're trying to login with is the same as the one you're already logged in with. (password confirmation, verify that you're still you to access advanced settings etc).

I think that Meteor.loginWith should add the service to an existing account if and only if the user can login with that existing account too. to confirm they want to add this service to the existing account.

I think that we should have Meteor.connectWith<service> to add a service to a currently logged in account. Maybe this and the previous paragraph is the same functionality?

@brettle
Copy link

brettle commented Oct 24, 2015

Thanks for clarifying and sorry for the earlier confusion. I agree that we need to be very clear in the documentation about how we are changing behavior. That said, I don't think what we're talking about is necessarily more radical than what a lot existing accounts-related packages do and I think if we do it right it will actually make the accounts system behavior more consistent and easier to understand. For example, right now, Meteor.loginWith<service> creates a new account if no existing account is found, except when <service>==Password. 😃

Here's my prefs for how it should work:

Meteor.loginWith<service> should never log you out of an account you are logged into and should never create a new account.

Meteor.createUserWith<service> should fail if you are already logged in or there is an existing account with the same id on the specified service. Otherwise it should create a new account and log you into it.

That leaves the "add service to existing user" functionality. I don't see any harm in adding this to Meteor.loginWith<service>. Specifically, calling Meteor.loginWith<service> while already logged in would add the service to your account. It would fail if the there is already an existing account with the same id on the specified service.

@splendido
Copy link
Member

I agree we'd need specular API for password and oauth:

  • Meteor.loginWith<service>
  • Meteor.createUserWith<service>

should be the only two methods to log-in users or create new ones and, at this point, service could simply be the first parameter to be passed to the methods ;-)

With this two separated paths, it should be esy enough to put in place a clear logic for accounts melding/merging/linking

@splendido
Copy link
Member

...to keep backward compatibility we could suggest to add a configuration parameter to the Accounts package to prevent Meteor.loginWith<service> creating new accounts. When set to true the explicit use of Meteor.createUserWith<service> will be needed also for 3rd party services.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants