jaredhanson / passport

Simple, unobtrusive authentication for Node.js.
https://www.passportjs.org?utm_source=github&utm_medium=referral&utm_campaign=passport&utm_content=about
MIT License
22.99k stars 1.24k forks source link

Send error message when verify function fails due to invalid credentials #483

Open fvgs opened 8 years ago

fvgs commented 8 years ago

Currently, when invalid credentials are passed, the verify function merely responds with a 401 status code. I realize flash messages and redirects are available, however I am authenticating via an AJAX request. Therefore, neither of those is useful to me.

The 401 status code is enough to tell me the credentials were invalid. However, it does not provide enough feedback for me to tell the user whether it was the username, password, or both that were incorrect.

Is there currently a way to achieve this of which I'm unaware? If not, it seems like this is a feature that should be implemented at the passport level so that strategies can use it to provide meaningful error feedback.

fvgs commented 8 years ago

@jaredhanson any feedback on this?

JamieSharpe commented 8 years ago

The 401 status code is enough to tell me the credentials were invalid. However, it does not provide enough feedback for me to tell the user whether it was the username, password, or both that were incorrect.

This is considered bad practice in security. I'd advise you move away from this.

You can find more information on why here but also be aware of this too.

fvgs commented 8 years ago

It is a security concern to the extent that leaking the existence of a specific user record will benefit an attacker. In the case of a brute force attack, rate-limiting goes a long way in preventing the attacker from succeeding even with the knowledge that a particular login id exists.

Likewise, as the second link mentions, in order for this obfuscation to have a factorable benefit, the registration process must be just as opaque. In other words, attempting to register with an already registered login id cannot let the registrant know the login id is already in use. Any service allowing users to login with an internal login id such as a username will fail this test. The login id must have external significance, such as an email address, so ownership can be externally verified without giving away information to arbitrary individuals. As shown in the article, many applications that obfuscate feedback in the login form still fail this test elsewhere. Thus, they provide an unfavorable UX experience with no factorable benefit to security or privacy.

In terms of security, we already established that mitigating a brute-force attack can be done by rate-limiting, which is standard practice. What I didn't see mentioned in either of those links, which I believe is a far more serious attack vector, is the potential for a social engineering attack. If an attacker can collect and verify a set of email addresses registered with a service, they can easily target those users through a social engineering attack.

Similarly, as a matter of privacy, an individual should not be allowed to check if a certain email address is registered with a service.

Thus, with respect to login forms and passport, I agree a service should not be leaking information verifying the existence of any login id. However, this policy has to be implemented across the entire service in order to be effective. If the information can be leaked via the registration form, then degrading the UX experience of the login form adds almost zero benefit to security or privacy. I say almost zero because on the privacy front, a non-technical user may be more likely to exploit the information leak through the login form than the registration form.

With that in mind, I agree this is a practice to be avoided for reasons of privacy and security. But it is also up to the developer to evaluate if their application will actually become more secure as a result. Otherwise, it may make sense to provide this convenience to the user.

Personally, I hadn't fully considered the implications of telling the user which credential is incorrect. From now on, I'll be sticking with "The email/password you entered is incorrect", as well as enforcing this policy throughout my application.

JamieSharpe commented 8 years ago

I agree that the linked websites fail to mention other vectors of attack, including but limited to these as well. But that's likely to do with other vectors being a topic/sub-topic of their own; as security and counter measures can branch out into many categories.

I disagree with your opinion that it's up to the developer to evaluate the level of security in the application. That's what we have security analysts, and pen-testers for. Of course the developer should not be ignorant of security practices, and should thrive to produce secure software to the best of their ability, with collaboration with those more qualified.

Coming back to this topic, I think adding in a feature to be able to tell which authentication stage failed would be a step backwards in security. If this was to be added in, other developers who aren't aware of this vector of attack, may be inclined to use it for the initial use you posted; informing the end user. For a lost potential feature, we really have a net gain in security by completely disallowing it. Which I see as a good thing.

fvgs commented 8 years ago

I think it's up to the team creating the service and their advisors to evaluate the tradeoff between providing this convenience to the user and the potential loss in security.

For instance, suppose you have a service that has a username as the login id and is robust in its rate-limiting. Under these parameters, the threat posed by exposing the existence of a login id through the login form is virtually non-existent. If the login ids are public (such as on GitHub), there is zero benefit to obfuscating the feedback and it is trivial for an attacker to compile a list of valid login ids to target. Even if the login ids are not officially public, rate-limiting mitigates all but the most large-scale of brute-force attacks. Therefore, verifying the existence of a login id is of little help to an attacker. Furthermore, if the service has strong rules about what passwords are allowed, a bot trying the passwords "123456" or "password" on every login id on the service will not compromise a single account.

Also recall from my previous comment that it is generally not practical to have a truly private login id. Most services (who use a username as the login id) allow the user to select their login id and must therefore provide feedback during registration when a login id is not available. An attacker can always use this vector to produce, at the minimum, a small list of valid login ids to target. One could solve this by having the service assign an arbitrary string to be the user's login id, thereby making it possible to have a private login id. But, for the same reasons given above, this is rarely necessary and would be a big hit to UX. That is, arbitrarily assigning a login id of "8sha^2jHmW" to a user for the sake of negligibly improving security and privacy is not necessary or recommended unless your service deals with highly sensitive data such as bank information.

I have personally come to the conclusion that it is best for a login id to be fully private in such a way that the service will never reveal whether or not some login id (such as an email) is registered with it. Thus, I will be sticking to the ambiguous authentication feedback.

However, bringing this back to passport, I disagree with the notion that adding the option for specific authentication feedback would negatively impact the security or privacy of users' services. Many, if not most, services have a user-selected username as the login id. More often than not, that login id (username) is public. So an attacker does not need the login form to figure out what login ids are valid. Again, look at GitHub for an example. For those services where the login id (such as an email) is meant to be private, the vast majority allow for the login id to be exposed in some way or another. That is not something to be encouraged, but there would be no loss of security. For the small percentage of services that properly implement this sort of security policy already, well, they know what they're doing.

I think it's clear how this feature would be a UX benefit. But it should also be clear that providing specific authentication feedback has no security impact on the majority of services. Thus, I think it makes sense to introduce the non-default option to provide specific authentication feedback to passport.

faustlifes commented 7 years ago

Guys, I think if user request auth like login at server, and some auth data is the wrong, server should return error 400, not 401, and the server is not required to specify what was wrong but should divide this two errors, 401 should return when client request secure Rest API, does it make sense?