codeigniter4 / shield

Authentication and Authorization for CodeIgniter 4
https://shield.codeigniter.com
MIT License
350 stars 123 forks source link

Security: informative error messages #264

Closed ThibautPV closed 8 months ago

ThibautPV commented 2 years ago

Hello,

I think that the translations should be modified to improve the security of Shield.

Indeed, the current translations can give information to malicious people and also be compromising for personal data.

For example, with this translation: "Unable to log you in. Please check your password", we know that the email address exists in the database.

This is also the case with the sentence "Unable to verify the email address matches the email on record."

For example, I suggest replacing with the valid sentence "Check your email! We just sent you an email with a Login link inside. It is only valid for 60 minutes."

What do you think?

InstantT

kenjis commented 2 years ago

This is a usability or security issue.

It may be better to change the default message to something less informative, since more secure is preferred as the default.

lonnieezell commented 2 years ago

I’m on my phone currently and can’t find the right doc, but the latest advice from NOST, IIRC, is that you should focus on usability and help people with the messages instead of secure it. Combined with encouraging better passwords they found the obscure messaging wasn’t needed and tended to hurt the experience with no benefit to security.

kenjis commented 2 years ago

Authentication and Error Messages https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#authentication-and-error-messages

Incorrectly implemented error messages in the case of authentication functionality can be used for the purposes of user ID and password enumeration. An application should respond (both HTTP and HTML) in a generic manner.

The problem with returning a generic error message for the user is a User Experience (UX) matter. A legitimate user might feel confused with the generic messages, thus making it hard for them to use the application, and might after several retries, leave the application because of its complexity. The decision to return a generic error message can be determined based on the criticality of the application and its data. For example, for critical applications, the team can decide that under the failure scenario, a user will always be redirected to the support page and a generic error message will be returned.

kenjis commented 2 years ago

Probably Lonnie said about NIST Guidelines https://pages.nist.gov/800-63-3/ But I didn't immediately know where exactly to read.

lonnieezell commented 2 years ago

I was just reading that part from OWASP about usability. I’ll try to find the NIST recommends later tonight. However, I’m tempted to say we leave it as is, and let people that need more security change their lang file. This is similar to how we treat remember me - since more secure sites are recommended not to have that functionality anyway.

MGatner commented 2 years ago

Maybe we need a docs section on "hardening security" with recommendations on how to set this package to be maximally secure.

ThibautPV commented 2 years ago

I think you should still modify the translations or indicate it in the documentation as suggested by @MGatner

If we look on the internet, comparing Laravel vs CodeIgniter, many websites highlight security as a strong point of CI

So I think this is an important point on which you should continue to differentiate CI.

This is of course only my opinion and I let you decide ;-)

sammyskills commented 9 months ago

I've seen some websites use something like this after the initial email validation:

If the email you entered exists in our database, you will receive a link to reset your password.

Can we adopt this? @kenjis

kenjis commented 9 months ago

@sammyskills Probably no. We have the following messages on sending Magic Link. And if we change the messages to that, the UX will be worse.

    'invalidEmail'          => 'Unable to verify the email address matches the email on record.',
    'unableSendEmailToUser' => 'Sorry, there was a problem sending the email. We could not send an email to "{0}".',

    'checkYourEmail'     => 'Check your email!',
    'magicLinkDetails'   => 'We just sent you an email with a Login link inside. It is only valid for {0} minutes.',

If we change only magicLinkDetails, it doesn't make much sense. Because invalidEmail shows the email address is not registered.

sammyskills commented 9 months ago

If we are to adopt it, definitely, other messages will be updated, not just magicLinkDetails. For this, we can update the magicLinkDetails and invalidEmail to use the same message, or better still, use just one lang string:

'magicLinkDetails' => 'If your email address exists on record, you will receive a Login link inside which is valid for {0} only.'

With this, we can remove the invalidEmail string. The unableSendEmailToUser will still be used when sending the email fails.

kenjis commented 9 months ago

Okay, that makes sense. But we have no consensus to change to generic messages.

If we use the message like If your email address exists on record, you will receive a Login link inside which is valid for {0} only., the user won't know the email address is registered or not. If the user forgot which email address is registered, the UX will be worse.

Of course, you can customize the error message as you like. So, adding docs to customize the message for security is no problem.

MGatner commented 9 months ago

The decision to return a generic error message can be determined based on the criticality of the application and its data.

I think this is still the guiding principle, and providing helpful messages with docs on how to tighten security covers all bases. But thank you @sammyskills as ever for your attentiveness