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

Example code is susceptible to username hijacking #87

Closed
kgiszewski opened this Issue Apr 30, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@kgiszewski
Copy link

kgiszewski commented Apr 30, 2018

While I know the code you are providing in the views/controller is sample, the code is susceptible to allowing for a user to create an Umbraco account for an email/username which is not apart of the claims.

I realize it's up to the developer to look out for these sorts of situations but Identity is also a "not easy to learn" topic. I recently worked on a project where a malicious user could associate a different email by manipulating the hidden fields.

My suggestion is to discourage "picking your own username" and to discourage hidden fields for the username". Most developers are just happy it works, but I think this leaves a site open to misuse.

Scenario:

  • User authenticates against 3rd-Party
  • User is taken to the ExternalLoginConfirmation where the user can choose his/her username
  • I can then choose some_account@not_mine.com or a hidden field is used for the user/email (which I can manipulate client side)
  • Identity will then associate my login with an account that is not mine with potential malicious outcome

Suggestions:

  • Change the sample code to not encourage the developer to ask for a username
  • Don't rely on a hidden field for the email/username either
  • Use information from the claims to create the username
  • Change the following code inside ExternalLoginConfirmation to only take information from the claims and not the model:

image

https://github.com/Shazwazza/UmbracoIdentity/blob/master/src/UmbracoIdentity.Web/App_Code/Controllers/UmbracoIdentityAccountController.cs#L138-L143

To sum up, thanks for writing this, just wanted to pass on my 2 cents.

I changed a project that I was working on to only use information provided via claims and disallowed using any sort of hidden fields on the form.

@Shazwazza

This comment has been minimized.

Copy link
Owner

Shazwazza commented Apr 30, 2018

Hi @kgiszewski , It's been a while since I've had my head in this project and most of this code is taken directly from the code installed via the MVC templates with Visual Studio. I can't remember if I've changed it to allow such a thing to occur.

Would you by chance have time to submit a PR for review as this would be very helpful to more quickly get a better idea of exactly what you are referring to?

@kgiszewski

This comment has been minimized.

Copy link
Author

kgiszewski commented Apr 30, 2018

Sure thing, I'll try to get you something 👍

@Shazwazza Shazwazza added this to the 6.0.1 milestone Jan 15, 2019

@Shazwazza

This comment has been minimized.

Copy link
Owner

Shazwazza commented Jan 15, 2019

I've investigated this now and figured out what you are referring to. IIRC what happened was that a lot of this code is borrowed from the ASP.Net original identity 2.x snippets but since our members generally use an email address as their username, that snippet was updated whereas i think the original identity snippet actually uses a true username instead of an email.

In any case, i've simplified this whole thing, there will be no screen to confirm your username or email, it will just create your account with the email supplied from the claims and if any of that fails will redirect to an error page with the details. I'll get this released asap.

@Shazwazza Shazwazza closed this in fcc0025 Jan 15, 2019

@kgiszewski

This comment has been minimized.

Copy link
Author

kgiszewski commented Jan 15, 2019

Cool. I forgot that I was supposed to PR this. Glad I made some sense 😊

Thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.