meteor / meteor-feature-requests

A tracker for Meteor issues that are requests for new functionality, not bugs.
Other
89 stars 3 forks source link

Accounts Password Setting Server Hooks #374

Open chasemaier opened 4 years ago

chasemaier commented 4 years ago

We have password complexity policy requirements that we wish to enforce for our users and we're using the Meteor Accounts package.

There are "client-only" methods for changePassword, resetPassword and server methods setPassword etc.. that take a new password value and save it for the user.

The problem is there is no ability (that I can tell) to allow the server to authorize such changes. For example, I'd like to add a hook where -- given a new password -- it would return true/false if it was okay to continue with the password setting operation and that could be incorporated into all methods that allow setting or saving a password for a given user.

We already have front-end validation and explanation of what the rules are -- but there seems to be no way to enforce those password complexity rules on the server before they hit mongo.

E.g. A user executing Accounts.changePassword(...) directly in their browser console.

mitar commented 4 years ago

You cannot do that because passwords are already hashed before they hit the server. You could try to break the hash and see if they are weak in that sense. That might even be better than a policy.

evolross commented 4 years ago

Could you write your own Meteor Method to receive the password plain-text, check it, then call Accounts.changePassword() on the server?

chasemaier commented 4 years ago

That's the current solution, yeah -- it's just a bit cumbersome to handle each of the set/reset/change variants and (as you say) requires the password hit the server which is not ideal. I suspect there is some crypto to prove conformance without knowledge of the password, but perhaps that's overkill.

I guess having the built-in client-only methods invoke some isPasswordAcceptable hooks on the client side would be one step forward (even though you could still call the Meteor method directly -- at least it's a little more obscure). We could accomplish this in the interim by monkey patching in our replacement implementation/wrapper for each method, but seems like a universally useful sanity check (if not security enforcement).

evolross commented 4 years ago

As a slightly better work-around for now, it might be worthwhile to experiment with the Meteor.users collection and the collection hooks package to see if there's some server-side checking/reversing you could do in a before.update or after.update hook. Could save on the monkey-patching.

evolross commented 3 years ago

A security researcher emailed me showing how extremely long passwords (e.g. 12K+ characters) can be passed to Accounts.setPassword (which takes a plain-text password) and then hashes it on the server in this code.

This can open a vulnerability to DOS attacks by overloading the server CPU with hashes of excessively long passwords. It's likely not a high-priority vulnerability as I assume:

  1. There's rate-limiting for a single Meteor account calling this method repeatedly
  2. A DDOS would require multiple logged in accounts to actually call setPassword which isn't easily scaled
  3. Most apps probably use Accounts.changePassword which does hash on the client. A malicious actor would have to know about Accounts.setPassword.

But, it's vulnerability nonetheless which is corrected by adding a simple configurable password length limit.

So this could be added to this feature request.