Skip to content
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

Rebuild the login process #1130

Closed

Conversation

@bytehead
Copy link
Member

bytehead commented Dec 23, 2019

As discussed in Slack on 12th of December 2019, we want to rebuild the login process in Contao 4.9 so both the username/password login and the 2FA login use the delay scheme as described in the RFC:

Another option would be to implement a delay scheme to avoid a brute
force attack. After each failed attempt A, the authentication server
would wait for an increased TA number of seconds, e.g., say T = 5,
then after 1 attempt, the server waits for 5 seconds, at the second
failed attempt, it waits for 5 * 2 = 10 seconds, etc.

This implies that $user->loginCount is set to 0 by default and is incremented with each failed login attempt. Then $user->locked is set to $user->loginCount * 5 seconds, so the lock period increases with each failed login attempt.

There are several advantages compared to the current logic, e.g. we do not have to inform the administrator about locked accounts and we do not have to configure the lock time and number of login attempts anymore – which was one of the bigger issues with this PR.

@bytehead bytehead changed the title [POC] Rebuild login process [RFC] Rebuild login process Dec 23, 2019
@bytehead bytehead marked this pull request as ready for review Dec 23, 2019
@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Dec 23, 2019

This is just a first attempt, maybe there is more to cover or refactor. What do you think?

@bytehead bytehead force-pushed the bytehead:feature/rebuild-login-process branch from e5bd103 to 8ef5fc2 Dec 23, 2019
@m-vo

This comment has been minimized.

Copy link
Contributor

m-vo commented Dec 23, 2019

I like!

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Dec 23, 2019

Instead of a migration that turns all the loginCount values into 0 we could just drop that column and name it loginAttempts with a default value of 0. It's more descriptive and migration is not needed :)

@Toflar Toflar added the feature label Dec 23, 2019
@bytehead bytehead added this to the 4.9 milestone Dec 23, 2019
@bytehead bytehead force-pushed the bytehead:feature/rebuild-login-process branch 2 times, most recently from 3812768 to 79b9b4e Dec 23, 2019
@DeDan69

This comment has been minimized.

Copy link

DeDan69 commented Dec 29, 2019

Since this is my first post, I apologize in advance if I am doing something wrong. So first, my greetings to all Contao developers out there. You are doing a great job! I am deeply impressed.

Let's come to my point for this RFC discussion:
Could you please also consider a point that I would sketch out as "Security by Design"?
Today, the Contao4 login is available via root-url/contao. Since knowledge about the target is an important key for any "brute force attack", I wonder if it would not be an advantage if the Contao admin could define the backend directory himself, via configuration file in /app/config/?
Merci

@xchs

This comment has been minimized.

Copy link
Contributor

xchs commented Dec 29, 2019

So you're actually suggesting security through obscurity?

@DeDan69

This comment has been minimized.

Copy link

DeDan69 commented Dec 29, 2019

So you're actually suggesting security through obscurity?

No, and I'm not here to argue. I wish to construtivly contribute. Clarity or ambiguity is for me only a question of documentation. By default it certainly could remain as it is. But neglecting safety aspects to achieve an apparent clarity could be a disservice in the end. Why not leave the decision to the admin of the site? Autonomy is ultimately the big brother of open source. Or am I wrong? Cheers

@fritzmg

This comment has been minimized.

Copy link
Collaborator

fritzmg commented Dec 30, 2019

Obfuscation of the back end login URL is a common feature of other systems as well.

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Dec 30, 2019

if the Contao admin could define the backend directory himself, via configuration file in /app/config/?

This should already be possible if you add the following lines to your routing.yml file I think:

contao_backend:
    path: /admin
contao_backend_login:
    path: /admin/login
@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Dec 30, 2019

This should already be possible if you add the following lines to your routing.yml file I think:

contao_backend:
    path: /admin
contao_backend_login:
    path: /admin/login

Don't forget to adapt the firewall configuration (access_control) in security.yml to the new paths:

access_control:
    - { path: ^/contao/login$, roles: IS_AUTHENTICATED_ANONYMOUSLY }
    - { path: ^/contao/logout$, roles: IS_AUTHENTICATED_ANONYMOUSLY }
    - { path: ^/contao(/|$), roles: ROLE_USER }
    - { path: ^/_contao/two-factor$, roles: [IS_AUTHENTICATED_2FA_IN_PROGRESS, ROLE_MEMBER] }
    - { path: ^/, roles: [IS_AUTHENTICATED_2FA_IN_PROGRESS, IS_AUTHENTICATED_ANONYMOUSLY] }
@Mynyx

This comment has been minimized.

Copy link
Contributor

Mynyx commented Dec 31, 2019

For usability reasons the account should not be locked after the first failure. I would propose to accept three attempts before starting to lock the account.

This would not decrease the security level, because in opposite to the currenct implementation the login attempts counter ist not reset.

@DeDan69

This comment has been minimized.

Copy link

DeDan69 commented Jan 1, 2020

This should already be possible if you add the following lines to your routing.yml file I think:

contao_backend:
    path: /admin
contao_backend_login:
    path: /admin/login

Don't forget to adapt the firewall configuration (access_control) in security.yml to the new paths:

access_control:
    - { path: ^/contao/login$, roles: IS_AUTHENTICATED_ANONYMOUSLY }
    - { path: ^/contao/logout$, roles: IS_AUTHENTICATED_ANONYMOUSLY }
    - { path: ^/contao(/|$), roles: ROLE_USER }
    - { path: ^/_contao/two-factor$, roles: [IS_AUTHENTICATED_2FA_IN_PROGRESS, ROLE_MEMBER] }
    - { path: ^/, roles: [IS_AUTHENTICATED_2FA_IN_PROGRESS, IS_AUTHENTICATED_ANONYMOUSLY] }

Happy new Year!
Thanks a lot for your efforts. All what Ausi and you replied seemed to me very logical and clear, but in the end I am not successful. It seems I am missing an important point that would allow contao to find its sources at the newly defined path. So far 404 is returned. -Thanks.

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 1, 2020

I never tried by myself. Possibly there is a lot of resource paths hard wired (like css and javascript files).

Copy link
Member

leofeyer left a comment

This PR is almost complete IMHO. 👍

There are a few inconsistencies though:

  1. When an account is locked for 5 seconds, I can submit more username/password combinations without increasing the lock period. However, when I do the same with the TOTP tokens, the lock period is increased upon every submit.

  2. When a user enters a wrong username/password combination, we log an "Invalid password submitted" message. We never log an "Invalid verification code" message though. Why not?

@leofeyer leofeyer changed the title [RFC] Rebuild login process Rebuild the login process Jan 2, 2020
@bytehead bytehead force-pushed the bytehead:feature/rebuild-login-process branch from 48d5c1d to 5b6525e Jan 6, 2020
Copy link
Contributor

aschempp left a comment

Looks pretty good so far! I wonder why we do/have to split the logic across AuthenticationProvider, event listeners and success/failure handlers. Why not "just" have everything in success and failure handlers in the first place? But maybe we should merge this first and then (try to) optimize.

@@ -360,10 +360,10 @@
'eval' => array('rgxp'=>'datim', 'doNotCopy'=>true),
'sql' => "int(10) unsigned NOT NULL default 0"
),
'loginCount' => array
'loginAttempts' => array

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 7, 2020

Contributor

I'm pretty sure the DBAL Schema Migration will try to rename this field… It's not really a problem though, as the delay seconds is only applied if the password is wrong, so it would be increased 15 secs instead of 5 secs on the first login. I'm really fine with that.

return $this->httpUtils->createRedirectResponse($request, $this->determineTargetUrl($request));
}

$this->user = $user;
$this->user->lastLogin = $this->user->currentLogin;

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 7, 2020

Contributor

I wonder if we should change this too, only store the time a 2FA was successful (if enabled for the user)?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jan 10, 2020

Member

I think so, too.

--$user->loginCount;

if ($user->loginCount > 0) {
if ($user->loginAttempts < 1) {

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 7, 2020

Contributor

I'm not sure this process is correct. According to the docblock, the user should be locked for 5 secs after the first attempt. But this is not the case here. Maybe this block is more a leftover of a not-locked login attempt, which does not exist anymore?

On the other hand, it does not really make sense to lock after the first attempt and show a "your locked for 5secs" ?

@@ -61,12 +61,12 @@ public function testThrowsAnExceptionIfTheAccountIsLocked(): void
/** @var BackendUser&MockObject $user */
$user = $this->mockClassWithProperties(BackendUser::class);
$user->username = 'foo';
$user->locked = time() + 300;
$user->locked = time() + 5;

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 7, 2020

Contributor

@ausi won't this result in time issues similar to the image tests?

This comment has been minimized.

Copy link
@bytehead

bytehead Jan 7, 2020

Author Member

Should not. It uses the ClockMock from Symfony/phpunit-bridge.

@@ -22,14 +22,13 @@ public function testReturnsTheLockedSecondsAndMinutes(): void
$exception = new LockedException(300);

$this->assertSame(300, $exception->getLockedSeconds());
$this->assertSame(5, $exception->getLockedMinutes());

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 7, 2020

Contributor

The getLockedMinutes method is deprecated, but it's still there. Still makes sense to test it works correctly.

This comment has been minimized.

Copy link
@bytehead

bytehead Jan 7, 2020

Author Member

Valid point :)

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 7, 2020

Why not "just" have everything in success and failure handlers in the first place?

I've asked this myself. I'll check if it's somehow easy to refactor.

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 8, 2020

  1. When an account is locked for 5 seconds, I can submit more username/password combinations without increasing the lock period. However, when I do the same with the TOTP tokens, the lock period is increased upon every submit.

I have to check.

  1. When a user enters a wrong username/password combination, we log an "Invalid password submitted" message. We never log an "Invalid verification code" message though. Why not?

I don't know. Should it?

@bytehead bytehead force-pushed the bytehead:feature/rebuild-login-process branch from 5b6525e to ecfcc97 Jan 8, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 9, 2020

I don't know. Should it?

I don't know, either. 😄 But it should be consistent, I think, so either log both or none. WDYT?

bytehead added 2 commits Dec 13, 2019
bytehead and others added 7 commits Dec 23, 2019
@bytehead bytehead force-pushed the bytehead:feature/rebuild-login-process branch from ecfcc97 to 4cdbbf0 Jan 9, 2020
@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 9, 2020

I think, so either log both or none. WDYT?

Definitely.
But I don't have a preference, as I don't care about this messages. 🤷‍♂
@contao/developers WDYT?

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 9, 2020

I wouldn't log it at all. Just floods the logs and what info does it provide? If someone cannot log in anymore they will get in touch with me, I'll try myself and notice myself that the password is wrong :D

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 10, 2020

Ok, then we will drop the "user entered a wrong username and password" log messages entirely.

@Mynyx

This comment has been minimized.

Copy link
Contributor

Mynyx commented Jan 10, 2020

I do not think it is a good practice to remove the log messages. They could be helpful to analyse unauthorised access or attacks.

It is also an OWASP recommendation: https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#logging-and-monitoring

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 10, 2020

You can still see the failed login requests in the server log, which is a better starting point to analyse attacks anyway IMHO.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2020

Closed in favor of #1178

@leofeyer leofeyer closed this Jan 14, 2020
@leofeyer leofeyer removed this from the 4.9 milestone Jan 14, 2020
@bytehead bytehead deleted the bytehead:feature/rebuild-login-process branch Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.