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

Clean up pin code key and biometrics key on logout #6906

Merged
merged 4 commits into from
Oct 4, 2022

Conversation

artkoenig
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Delete pin code key and the key used for biometrics authentication on logout.

Motivation and context

Pin code key and biometrics key should be deleted if the user logs out.

Signed-off-by: Artjom König artjom.koenig.ext@bwi.de

jmartinesp
jmartinesp previously approved these changes Aug 22, 2022
Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bmarty
Copy link
Member

bmarty commented Aug 22, 2022

Are we sure we want this change?

PIN code and biometrics belong to the user and protect access to the application, and are not specifically related to any session. For instance, if we support multi account, signing out from a session should preserve those security prompt for the other sessions.
Also if the user logs in again, even using another account, the protection level should stay the same (IMO).
I'm wondering if you could have users who get confused to set up PIN code and biometrics again after a re-login.

On the other hand, having extra security to access a login screen is probably wrong.

CC @dkasak for security advisor.
CC @daniellekirkwood for product POV.

@jmartinesp jmartinesp dismissed their stale review August 23, 2022 06:01

Removed approval until we've discussed if we want to include these changes.

@artkoenig
Copy link
Contributor Author

With the current implementation the user who did setup biometrics can unlock the app, regardless of who is actually logged in on the same device...

@jmartinesp
Copy link
Member

jmartinesp commented Sep 5, 2022

It makes sense to me to remove pin code protection on logout at the moment since we don't support multi-account.

On the other hand, having extra security to access a login screen is probably wrong.

Yes, it might make sense if you had several accounts and you still had access to your conversations and rooms on one of the other accounts, but it feels weird to have to pass authentication to even log in. Also, I don't think there's a way to remove or modify this authentication method until you've successfully logged in again.

@dkasak, @daniellekirkwood any input on this?

@dkasak
Copy link
Member

dkasak commented Sep 5, 2022

I agree it's not ideal to ask for PIN just to reach the login screen.

The PIN should conceptually be protecting logged in accounts -- so if we support multi-account, logging out an account should keep the PIN active for the remaining accounts, but allow you to create a new one without a PIN. But then further access of that account should again require you to enter your PIN, since the account is now logged in. This is a bit moot since we don't support multi-account at the moment, but it might be nice to leave a comment and/or create an issue about it, so that we remember this problem if/when we do get around to implementing multi-account.

One nagging point remains: What happens if the server logs you out, instead of it being user initiated? In other words, with this change, can a malicious HS force your device to drop its PIN/biometric auth?

@jmartinesp
Copy link
Member

One nagging point remains: What happens if the server logs you out, instead of it being user initiated? In other words, with this change, can a malicious HS force your device to drop its PIN/biometric auth?

As far as I can see, the pin code and biometric auth would be disabled in that case too. However, since you're logged out, is there anything worth protecting from an unauthorised user access? This would only remove the extra auth protection for the app, not the whole device.

@dkasak
Copy link
Member

dkasak commented Sep 5, 2022

I guess it doesn't, as long as we are non-multi-account and we can guarantee that a logout will make the existing data inaccessible. It still feels wrong to have this controllable by the HS though since the security story around that then becomes context-sensitive (e.g. whether we support multi-account or not).

Would it be possible to solve this problem in an alternative way, by excluding the login activities from the auth mechanism, but still requiring it for all others? (I'm unfamiliar with how this is implemented.)

@jmartinesp
Copy link
Member

Would it be possible to solve this problem in an alternative way, by excluding the login activities from the auth mechanism, but still requiring it for all others?

I think it's possible if we implement the UnlockedActivity interface for those screens.

@dkasak
Copy link
Member

dkasak commented Sep 5, 2022

Then I think that would be the better solution rather than accepting this PR, since I agree with @bmarty's reasoning here. A logout shouldn't delete the PIN, it should just allow for a new login to happen without having to know it.

@artkoenig
Copy link
Contributor Author

I think, you should at least delete the PIN and the corresponding key, if there are no more valid sessions on the device. This will work for a multi-account scenario, too. From the security point of view, there is no reason to keep the PIN and the key, in case if the users logs out of all his accounts. I would not assume, that it will be the same person, who logs in next time...

@artkoenig
Copy link
Contributor Author

I'm wondering if you could have users who get confused to set up PIN code and biometrics again after a re-login.

@bmarty Actually, no. As a user, I expect my settings to either be stored in my profile on a server or deleted when I log out of an app. I would even go so far as to expect the app to be in the state as if it were just freshly installed.

@jmartinesp
Copy link
Member

I'm wondering if you could have users who get confused to set up PIN code and biometrics again after a re-login.

@bmarty Actually, no. As a user, I expect my settings to either be stored in my profile on a server or deleted when I log out of an app. I would even go so far as to expect the app to be in the state as if it were just freshly installed.

I guess it's a bit subjective, but as a user I'd also expect this behaviour. Also, if we had multi-account support, I'd expect a different pin code to be configured for each user, so if you switch from one account to another you'd actually have to authenticate yourself with either pin code or biometric auth again.

@bmarty
Copy link
Member

bmarty commented Sep 7, 2022

Yes, it depends if you consider that the phone is shared by multiple people, each people having one account, and so a PIN per account makes sense, or if there is one single user with multiple accounts, and in this case one PIN code is enough.

I guess we will decide when we will think about multi-account.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this PR and iterate later if necessary. Thanks!

@bmarty bmarty merged commit ca6813b into element-hq:develop Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants