Two factor is not disabled/removed during user deletion

[skshetry]

When a user having activated TOTP is deleted, two-factor for that user is not disabled and secrets are not cleaned up.
So, if I:

1. Delete that user,
2. Recreate the user with the same username and,
3. Try to login with One-time password from the previous secret,
   Then, it can successfully be logged in.

Expected Output

The user is logged in without asking for two factor

P.S. This might not be a security issue, as the files are deleted during user cleanup.

@karakayasemi

[phil-davis]

This might not be a security issue, as the files are deleted during user cleanup

Yes, this seems more likely to be a nuisance issue for the user. If they got their account "reset" (deleted, recreated) by the admin for some reason, and during that process they deleted their client/saved knowledge of the OTP secret, then they will find that they are blocked from logging in - even though their newly-created account has just skeleton files.

Similarly if there was someone "Phil Davis" with user name pdavis deleted last week. Then a new staff member "Peter Davis" arrives, and a pdavis account is created. They will get blocked from logging in because they do not know the old OTP secret.

[karakayasemi]

As far as I see, we have a new generic event user.afterdelete which is coming with server 10.0.5: https://github.com/owncloud/core/pull/29939/files#diff-0a830b0354b26e84f08f705b5356a05bR317 . Also, we have legacy \OC\User:postDelete hook.
If I use the new event, the minimum version of the new release will be 10.0.5 instead of 10.0 . @PVince81 which one should I use?

[PVince81]

@karakayasemi use user.afterdelete. I think it's ok to depend on 10.0.5 or 10.0.10 in min-version. everything else is too old anyway

[davitol]

Tested with tarball 0.5.3.Rc2: #95 (comment) and keeps failing for me:

Steps to reproduce

1 - enable totp 0.5.2
2 - create a user /pwd
3 - log in with user /pwd
4 - enable totp using FreeOTP for the validation
5 - Log out and log in again using FreeOTP
6 - Upgrade TOTP app version to 0.5.3 using the tarball
7 - As admin, delete user
8 - As admin, create user /pwd2
9 - Log in using new password and old TOTP from FreeOTP

The user can log in.

Note

If you install 0.5.3 version (the one with the fix) and then you enable TOTP for the user etc it works fine. The bug persist only for the users that enabled it previosly

[PVince81]

@karakayasemi please investigate

[karakayasemi]

I could not reproduce the problem. I followed the below steps:
1- Checkout stable10
2- Enable totp 0.5.2
2- Create user test
3- Enable totp for test
4- Using test user, logout and login with password + totp
6- Remove totp v0.5.2, copy 0.5.3RC2 tarball
7- Delete user test with occ command
8- Check twofactor_totp_secrets table (There is no entry, working as expected)
9- Create test user again
10- Login with test user, it is not asking a second factor (Expected)

[davitol]

@karakayasemi Sorry wrong steps detailed above.

In the scenario detailed in the OP:

1 - Delete that user,
2 - Recreate the user with the same username and,
3 - Try to login with One-time password from the previous secret,
4 - Then, it can successfully be logged in.

The user that 'has been hacked' keeps been possible to log in. The PR fixes if the user deletion happens after the totp update (cleaning the users secrets). Is there a way to also get the users that were 'hacked' before? (deletion before twofactor_totp upgrade). Maybe it is not possible or it is out of the scope of the fix.

[karakayasemi]

@davitol When you delete user (step 1), this PR deletes all user related entries in totp table. When you create an user with same username, two factor totp will not be activated for the new user. So, step 3 should be not possible.

Actually, this PR's purpose is prevent possible locked down situation of new user from old user's second factor and clean up old redundant data.

[davitol]

@davitol When you delete user (step 1), this PR deletes all user related entries in totp table. When you create an user with same username, two factor totp will not be activated for the new user. So, step 3 should be not possible.

Yes, those steps are from the OP and twofactor_totp 0.5.2. (Already released). The scenario i comment is about fixing that users that were already deleted+created before the twofactor_totp upgrade. Due to the PR works fine for the detailed problem and prevents to happen again in the future, i think it is fine for me. Let's close it.

[karakayasemi]

Sorry, I understand your point now. I guess, it is not in the this PR's scope. To delete old deleted user's secrets, we should iterate all totp entries and check user exist or not.

IMHO, we may add an occ command for that.

[PVince81]

@karakayasemi can you raise a ticket for this ?

[karakayasemi]

Raised in here: #110