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

Add ambiguous error messages option to Accounts config #8520

Merged
merged 3 commits into from Mar 29, 2017

Conversation

Projects
None yet
4 participants
@dhrubins
Contributor

dhrubins commented Mar 25, 2017

User account enumeration is a security concern for some people. In our bug bounty program @LegalRobot, we've had 17 different reports on user enumeration. It got annoying, so we started using this code in local forks of accounts-core and accounts-password but are now happy to share this with the community. This PR addresses some of the simpler attack vectors: different error messages returned from the server upon various login failures or pw reset failures.

By default, we leave the existing behavior alone but provide a simple mechanism to enable this by setting the ambiguousErrorMessages key to true in Accounts.config on the server, like this:

Accounts.config({
  ambiguousErrorMessages: true
})

Enabling this option sends the same, rather ambiguous, error message to the client upon various login and reset failures. The specific text isn't really important, just that it can be identical across different failures. Perhaps as an extension of this, we could allow people to configure the message?

As @glasser and @lorensr discussed a few years ago in #2139, user enumeration is still possible via inference upon registration failure - the only way to prevent this is allowing duplicate accounts for the same email, but that opens up a whole mess of UX issues; and that can still be mitigated through rate limiting on those methods. So, this PR does not tackle the whole user enumeration problem, but at least we’re not being as explicit about every failure.

Unfortunately, we're not really able to write tests for this change since it seems that server-side testing is not isolated (PR anyone?). We can either run the tests with this new option disabled (which is more common and the default) or enabled, but not both.

Looping in @guide for documentation changes, and @DaTa since we're messing with some pretty important stuff in Meteor accounts.

Add ambiguous error messages option to Accounts
Add boolean option 'ambiguousErrorMessages' to Accounts config that sends ambiguous error messages to the client in order to mitigate user enumeration. User enumeration still possible via inference upon registration failure, but at least we’re not being as explicit about the failures.

@hwillson hwillson self-requested a review Mar 26, 2017

@hwillson

Hi @dhrubins - thanks for taking the time to submit this PR! At a quick glance, I think these changes make sense, but I'll let others comment as well to confirm. Normally we recommend first creating an issue to track feature requests like this, to promote discussion and help hash out of the best design approach (see Proposing your change in the Contributing guide). That being said, I've done a quick review of your code, and have a few suggestions, but I'm sure others will chime in with more comments. Thanks!

@@ -66,7 +66,11 @@ Accounts._checkPassword = function (user, password) {
password = getPasswordString(password);
if (! bcryptCompare(password, user.services.password.bcrypt)) {
result.error = new Meteor.Error(403, "Incorrect password");
if(Accounts._options.ambiguousErrorMessages) {

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

We should be able to reduce a lot of the error boilerplate code. Creating a re-usable error handling function like the following should help:

const handleError = (msg, throwError = true) => {
  const error = new Meteor.Error(
    403, 
    Accounts._options.ambiguousErrorMessages
      ? "Login failure. Please check your login credentials."
      : msg
  );
  if (throwError) {
    throw error;
  }
  return error;
};

So in this case, the call would become:

result.error = handleError("Incorrect password", false);

This comment has been minimized.

@dhrubins

dhrubins Mar 26, 2017

Contributor

Agreed - that makes for much cleaner code. Thanks!

throw new Meteor.Error(403, "Login failure. Please check your username and password.");
} else {
throw new Meteor.Error(403, displayName + " already exists.");
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

handleError(displayName + " already exists.");
throw new Meteor.Error(403, "Login failure. Please check your username and password.");
} else {
throw new Meteor.Error(403, "User not found");
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

handleError("User not found");
throw new Meteor.Error(403, "Login failure. Please check your username and password.");
} else {
throw new Meteor.Error(403, "User has no password set");
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

handleError("User has no password set");
userId: user._id,
error: new Meteor.Error(403, "Incorrect password")
};
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

return {
  userId: Accounts._options.ambiguousErrorMessages ? null : user._id,
  error: handleError("Incorrect password", false)
}
throw new Meteor.Error(403, "Login failure. Please check your username and password.");
} else {
throw new Meteor.Error(403, "User not found");
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

handleError("User not found");
throw new Meteor.Error(403, "Login failure. Please check your username and password.");
} else {
throw new Meteor.Error(403, "User not found");
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

handleError("User not found");
throw new Meteor.Error(403, "Login failure. Please check your username and password.");
} else {
throw new Meteor.Error(403, "User has no password set");
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

handleError("User has no password set");
return
} else {
throw new Meteor.Error(403, "User not found");
}

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Using handleError, this call would become:

handleError("User not found");
@@ -66,7 +66,11 @@ Accounts._checkPassword = function (user, password) {
password = getPasswordString(password);
if (! bcryptCompare(password, user.services.password.bcrypt)) {
result.error = new Meteor.Error(403, "Incorrect password");
if(Accounts._options.ambiguousErrorMessages) {
result.error = new Meteor.Error(403, "Login failure. Please check your username and password.");

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Since users don't have to login with a username (e.g. can use an email address), how about changing this to something like "Login failure. Please check your login credentials.".

This comment has been minimized.

@hwillson

hwillson Mar 26, 2017

Member

Just to add - I updated this in the common/sample handleError function I mentioned above.

Consolidate error handling
Per @hwillson’s comments
Maintain braces on `if` conditionals + other style changes.
This file leaves a lot to be desired, but strong preference to not
remove existing curly-braces on if-statements.

Also removed trailing whitespaces and slight indentation changes
following the changes in #8520.
@abernix

abernix approved these changes Mar 29, 2017 edited

Applied a style-fix commit, but otherwise LGTM. CI will now run.

@abernix abernix merged commit 3a9bd40 into meteor:devel Mar 29, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

abernix added a commit that referenced this pull request Mar 29, 2017

Merge pull request #8520 from dhrubins/accounts-password-ambiguous-er…
…rors

Add ambiguous error messages option to Accounts config

@dhrubins dhrubins deleted the dhrubins:accounts-password-ambiguous-errors branch Mar 30, 2017

@runlevelsix

This comment has been minimized.

runlevelsix commented Jun 1, 2017

I appreciate the work put into making this feature a reality in Meteor 1.5. However, I think it may be worth noting that it does not really prevent username enumeration.

When an non-existent email address is entered in the "forgot password" form, a websocket error still indicates that the password reset failed with the error: "Login failure. Please check your login credentials".

To be completely sure that username enumeration cannot happen, the error must not be returned to the client.

@abernix

This comment has been minimized.

Member

abernix commented Jun 2, 2017

@runlevelsix Thanks for bringing this up. You're right that the fix applied here does not truly avoid user enumeration (and that's stated in the OPs post). Though, I think it's worth pointing out that even Google and Facebook tell you immediately if an account exists via "Forgot Password" or not. There's a user experience issue to blocking it entirely, though it would be nice if it were fully configurable though. Would you be willing to expand upon this PR?

image
image

@dhrubins dhrubins referenced this pull request Jun 8, 2017

Open

Isolate server tests #58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment