domp icon indicating copy to clipboard operation
domp copied to clipboard

Possible security issue

Open felipekk opened this issue 10 years ago • 4 comments

Hello,

I'm adding your gem to my app, but when going through the code, I've noticed that you use this to locate an existing user record: existing_user = current_user || User.where('email = ?', auth_params['info']['email']).first

This assumes that the email address that the provider sent you can be trusted. Couldn't a user use this to hijack another account?

Say I have a regular user Alice. She registered through my website and is not using OAuth. Bob is a hacker and he wants to be able to use Alice's account on my website. He knows her email address. He knows she doesn't have Facebook, and that you can login to the website using Facebook. So he creates a fake Facebook account using Alice's email address, and uses that Facebook account to login to Alice's account on my website. Isn't this possible?

Of course, it may seem an edge case (Facebook probably requires email verification), but since this gem supposedly supports any OAuth provider...

One possible solution to this is to require that the user be logged in before allowing him to login to an existing account with a "new" provider.

felipekk avatar Oct 03 '14 23:10 felipekk

This is was I did:

authentication = provider.user_authentications.where(uid: auth_params.uid).first
existing_user = current_user || (authentication.nil? ? nil : User.where(id: authentication.user_id).first)

This basically checks if there is a UserAuthentication record between the user and the provider account.

arjanfrans avatar Oct 25 '14 10:10 arjanfrans

Should the second line say User.where(id: authentication.user_id).first instead of User.where(authentication.id).first ?

xuanwu avatar Oct 29 '14 22:10 xuanwu

Yes you are right, I changed it.

arjanfrans avatar Nov 02 '14 11:11 arjanfrans

Cool thanks!

xuanwu avatar Nov 02 '14 21:11 xuanwu