Unsecure biometric/fingerprint authentication
darekdeo opened this issue · 1 comments
Hi, I've found 2 issues regarding biometric/fingerprint security used in app. First of all, sorry if I misunderstood something from reading the code, I took only fast glimpse at it without downloading/checking out project.
- App doesn't ask for current app PIN while enabling biometric authentication (both for login and transaction auth)
1a. Is this because PIN is stored as string in SharedPreferences?
https://github.dev/breadwallet/breadwallet-android/blob/master/app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java
Here it looks a little bit better, but it's hard to tell without checking out project: https://github.dev/breadwallet/breadwallet-android/blob/master/app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java
If that's the case, then on rooted device PIN can be easly stolen.
1b. I would suggest to use PBKDF2/SHA1 and AES to encrypt and decrypt application sensitive data (that would be wallet keys, etc?). So upon login user would enter PIN, if it would successfully decrypt wallet then user would be able to proceed. Same upon executing transactions, etc. So if that's not yet implemented it would be great if application sensitive data would be stored on storage encrypted and decrypted upon only when retrieved to RAM memory.
This first problem is mostly problematic for biometric/fingerprint because I can only enable biometric for logging into app and leave it disabled for transactions. Imagine now someone else has registered his biometric fingerprint on my device too, he doesn't know my BRD app PIN but if he could enter the app, he can easly just enable biometric for transactions and voila. Separate switch for biometric login and transactions doesn't serve any purpose for user security. This goes into the 2nd issue...
- Normally when registering new fingerprint/biometric on the device, the system and keystore detects it and applications can react to disable fingerprint authentication. Unfortunately this is not the case for BRD wallet: https://github.dev/breadwallet/breadwallet-android/blob/master/app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java In this file setUserAuthenticationRequired is not forced, but left under boolean flag. On my device (Samsung Galaxy S10e) I know this feature works, but I was able to add new fingerprint to the device, login into app using newly added fingerprint and then enable biometric for transactions because there is no prompt for PIN as stated in point 1.
2a. App should detect that fingerprint was added/removed on the device and should disable biometric whatsoever, allowing to access the app using only PIN.
2b. But boolean flag can be left in the code, as setUserAuthenticationRequired feature is only available from newer Android devices (I think it's if i remember correctly >= P). Although not all producers did respect that and on some devices which are >= P setting this flag to true will throw exception. In this is the case it's best to catch the exception and then generate biometric key with boolean false as workaround.
The 2nd problem is related to first 1. Because if someone malicious can gain access to user's device (for example weak device PIN), he can probably easly add new fingerprints too. If in BRD biometric/fingerprint is enabled it will not react to adding new fingerprints and will allow malicious user to access the app, change the settings for transactions and steal wallet resources. Even if user will set strong PIN for BRD, he is not safe if he has weak device PIN and enabled biometric/fingerprint in BRD.
This issue doesn't mean to attack the app, it's great open source project. Thank you all for working on this. I'm only pointing few issues I've found while testing the app and taking the glimpse at the code, hope it will be helpful.
Hi,
I also came to a similar conclusion after looking through the code.
Specifically, the following getPhrase-function looks like BRKeyStore has been downgraded to a legacy-migration:
In other words, it seems to me that BRKeyStore is only present for migration-purposes and all new app-users are using some kind of encrypted shared preferences.
The function migrateKeystoreData indicates this migration: