Improve lock screen implementation with extra security measures

[jmartinesp]

Type of change

• Feature
• Bugfix
• Technical
• Other :

Content

See #6522 .

Motivation and context

We want to add additional security measures to the biometric authentication of the lock screen feature:

1. An extra check to make sure the Cipher used is unlocked after authentication.
2. Enabling a setting to only be able to use this Cipher while the device is unlocked.

Tests

• With your app in develop or any other branch, go to Settings -> Security -> Protect access, and create a PIN code and enable biometric authentication. This can be done on the emulator too, but you need to make sure you're on API >= 30).
• Build and launch this branch, make sure you can still use biometric authentication.

Tested devices

• Physical
• Emulator
• OS version(s): 12.

Checklist

• Changes has been tested on an Android device or Android emulator with API 21
• UI change has been tested on both light and dark themes
• Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
• Pull request is based on the develop branch
• Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
• Pull request includes screenshots or videos if containing UI changes
• Pull request includes a sign off
• You've made a self review of your PR
• If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

[artkoenig]

I have some remarks about this.

1. if (cipher != null && !isCipherValid(cipher)) should be IMO if (cipher == null || !isCipherValid(cipher))
2. isCipherValid should IMO check, if it can decrypt the encrypted data

[jmartinesp]

I have some remarks about this.

1. if (cipher != null && !isCipherValid(cipher)) should be IMO if (cipher == null || !isCipherValid(cipher))
2. isCipherValid should IMO check, if it can decrypt the encrypted data

About these:

1. I wrote it that way so the authentication can also be successful if no CryptoObject is used, although it is by default now. We could enforce using a CryptoObject as you suggest here.
2. That function actually checks if the Cipher inside the CryptoObject is unlocked by the biometric authentication, in this case, by using it to encode some value. If we tried to bypass it, the Cipher would throw an exception when it's used. Isn't this enough to avoid the issues around the current implementation?

If we wanted to both encode and decode using the Cipher inside the CryptoObject we'd need to:

1. Create a new credential when the biometric authentication is enabled since its Cipher with ENCRYPT_MODE would need to be unlocked by a successful authentication.
2. Store it.
3. Decrypt that credential using another Cipher with DECRYPT_MODE and verify it matches the original.

The problem here would be the migration for users who are already using biometric authentication: to create this encrypted credential we first need to have the user unlock a Cipher with encrypt mode enabled. This means either silently trying to do this migration by trying to create the credential the first time biometric unlock is used and not checking the Cipher if there is no credential, or force every user already using biometric unlock to unlock twice if the credential is not present (one to create them, another one to verify those credentials). Also, in the first case the Cipher validation could be bypassed by just removing that credential from its storage.

[artkoenig]

I think it should be enough to enforce a non-null CryptoObject (to prevent the bypassing with a Frida script) and leave the isCipherValid method as it is.

[jmartinesp]

I've been having issues all morning with the same emulator on API 31 I've been using so far where there is an extremely weird crash:

1. We need to pass a CryptoObject when we call BiometricHelper.authenticateIntenal.
2. For this CryptoObject we need an initialized Cipher instance.
3. When the Cipher is initialized it throws an UserNotAuthenticatedException, which is unexpected and uncaught.
4. To authenticate the user we need to call BiometricHelper.authenticate. We're back on point 1.

This is actually working fine on another API 30 emulator and 2 real devices with APIs 30 and 31, and I still haven't found what could be causing this weird behaviour, not even creating a new API 31 emulator fixed it. I did upgrade my emulator version from the SDK not long ago, so it might be a newly introduced bug in emulators in that API.

[artkoenig]

Just one more remark: PKCS1Padding used here has some security issues. It's better to use OAEPPadding here.

[jmartinesp]

Just one more remark: PKCS1Padding used here has some security issues. It's better to use OAEPPadding here.

RSA with PKCS1Padding is used for Android 5 and 5.1. We can try to change the padding used to a better one, but SecretStoringUtils is used through the app in quite a few places, and if any of the data encrypted on those places is stored to disk and reused between app sessions we'd have to migrate those too along with the keys, so this work would probably need its own issue and planning first.

Either that, or split the cryptography in 2 separate components, one for the rest of the app and another one specially for this. Also, we were already considering bumping minSdk to 23 (see #6067), and in that case we could just drop support for this RSA encryption after a migration to AES if I'm not mistaken.

[artkoenig]

Switching to minSdk 23 sounds good to me.

[yostyle]

LGTM

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

75.0% Coverage
0.0% Duplication