vendure-ecommerce / vendure

The commerce platform with customization in its DNA.
https://www.vendure.io
Other
5.63k stars 997 forks source link

Customer Accounts #33

Closed michaelbromley closed 5 years ago

michaelbromley commented 5 years ago

A Customer can be a guest (has no associated User) or registered (has an associated User).

Account creation

An account can be created in one of two ways:

  1. Ad-hoc account creation by the customer outside the order process
  2. Conversion of a guest account into an authenticated account after the placing of an order.

customer-account-order-activity-diagram

As the diagram shows, a customer does not need to create an account after placing an order. Indeed, the customer is free to only every do guest checkouts. For the checkout, however, the email address would be used to create a guest Customer, and then on subsequent checkouts with the same email address, the same guest Customer would be assumed.

At any time, the guest Customer can be converted into an authenticated Customer by registering in either of the two ways outlined above.

Registration process

There are 2 questions to resolve regarding registration:

  1. Do we support 3rd party authentication methods in the future (social login, Google account etc)? If so, what (if any) changes to the current User entity would be needed?
  2. Do we require email account verification? If so, what is the purpose? Do unverified users have a restricted set of possibilities compared to verified ones?

Account Deletion (GDPR)

A user should have the ability to delete their account, as should an administrator. In this case, we want to keep the physical row in the DB for the purposes of data consistency and reporting, but we should completely remove any personally identifiable information (PII):

“[A]n identifiable natural person is one who can be identified, directly or indirectly, in particular by reference to an identifier such as a name, an identification number, location data, an online identifier or to one or more factors specific to the physical, physiological, genetic, mental, economic, cultural or social identity of that natural person.” source

michaelbromley commented 5 years ago

Issue: Account enumeration attacks

Since Customer's emailAddress is constrained to be unique, and we allow the customer to change their emailAddress, this opens the door to an enumeration attack. Consider:

  1. Attacker creates an account with the email address "hacker@leet.net"
  2. Attacker goes to own account page and attempts to update own account with new address "joe.smith@gmail.com"
  3. If a customer with the email "joe.smith@gmail.com" already exists, we cannot allow this change as it would violate the unique constraint. We return an error of some kind.
  4. Now the attacker knows that Joe Smith has an account with us.
  5. Attacker repeats this process (likely in an automated fashion directly against the GraphQL API) to build up a list of all of our registered users' email addresses.

The same process could also be taken against the "create account" endpoint, but would be a little less efficient since it would create a lot of new accounts for non-matching email addresses.

Severity

In the case outlined above, the disclosure of the existing email address is necessary and unavoidable. However, is it so bad?

Many well-known sites also allow such enumeration: https://markitzeroday.com/enumeration/privacy/2017/06/20/to-be-enumerated-or-not-to-be.html

This answer basically says it is a non-issue: https://security.stackexchange.com/a/42874

I tend to think it is of low severity since email addresses are public information in general.

Mitigation

The most practical mitigation I can think of would be some kind of throttling on the frequency that a given Customer may attempt to change their emailAddress.

michaelbromley commented 5 years ago

Email address verification

Email address verification is quite a common pattern. It usually works like this:

  1. User signs up with email address and a password.
  2. An email is sent to that address with a verification link. The link contains a unique token which is usually time-limited.
  3. Clicking the link tells the site that the email link has been clicked, so we can safely assume that the person who registered under this email address does indeed control the account.

The purpose of this process is:

How to handle

Some companies may want to implement email verification and others not. We should make it optional, i.e. a Customer entity would have a verified property, and the developer can choose to use this or not when deciding whether certain functions are permissible.

In general, much of the implementation will be contained in however we handle emails in general (issue to be created).

michaelbromley commented 5 years ago

Review this discussion on the Sylius repo for another argument for verification: https://github.com/Sylius/Sylius/issues/6883

michaelbromley commented 5 years ago

Customer Registration Flow

customer-registration-activity-diagram

In the diagram above, the password is only set after the verification link has been clicked. This works in the same way as a typical the "forgotten password" flow.

The reasoning is that there is no additional security to setting & then verifying a password on registration, since the user could also register with a password and then after that invoke the forgotten password flow, which we must provide, otherwise a forgotten initial registration password would render that email account void and unusable.

Therefore there is no additional security to be gained by requiring a password up-front.

Token expiry

The verification token expires after a configured time period (7 days in the current default config). After expiry a new token can be issued to the email address. Why then bother with an expiry if a new token can be immediately issued? The reason is that this prevents the scenario where the initial token is leaked somehow (email address compromised, email traffic intercepted) but now then real owner once again has control over the email account and the attacker does not.

marwand commented 3 years ago

Is there a reason why creating an account with an email already registered doesn't return an error?

Update - more information

Running the following mutation multiple times with the same credentials always returns success.

mutation Register($input: RegisterCustomerInput!) {
  registerCustomerAccount(input: $input) {
    __typename
    ... on Success {
      success
    }
    ...ErrorResult
  }
}

fragment ErrorResult on ErrorResult {
  errorCode
  message
}

Here are the two cases I could think of: 1) If the user registered, but has not yet verified his account. Any subsequent registration attempt with the same credentials returns success. Also, multiple verification emails are sent. If different passwords were specified during the registration attempts, the first one is the one recorded in the database. You could see how this might confuse customers.

2) If the account is verified, any subsequent registration attempt returns success.

Looking at the error codes currently used, it seems like it should return:

I'm now aware of commit b1ffa1e that makes the registration mutation silently fail to protect user data. You're trading off extra security for customer experience. As a user, I often forget if I have an account on a particular website - especially if I don't use it often. I'm curious to hear what you think.

alibeknow commented 3 years ago

We have same problem with this case