Proposal for HWKeyCrypter and Biometric Authentication #1121
Conversation
|
Thanks for investigating into biometrics. For now, I only quickly skimmed over your PR, but I've got the following conceptual questions:
|
It's not a replacement in the sense that both features co-exist but you're right that they are mutually exclusive. Having both active at the same time isn't supported.
The key in the secure element/TEE is not extractable which is precisely why it's secure. Here it is important to understand the Bitcoin master key is not imported into the secure module as Android KeyStore doesn't support the secp256k1 elliptical curve. However, an AES key is generated in the secure environment and used to encrypt the wallet file the same way the spending PIN derived AES key did. The key acts as a key encryption key (KEK).
Initially when HW-encryption is first activated, a key is generated in the secure environment. When importing a different wallet and activating HW-encryption, that same key will be used for encryption because the key is bound to the application and always uses the same key reference defined in Constants.java.
There is no way to recover the biometrics-protected wallet. However, this is only relevant if trying to recover the currently in-use wallet file under /data/data/de.schildbach.wallet/files/wallet-protobuf. A backup file will never be HW-encrypted. |
Co-authored-by: Sebastian Nicol <sebastian.r.nicol@gmail.com> Co-authored-by: Oliver Aemmer <oliver.aemmer@protonmail.ch>
I wonder if we should take that part of this PR and extend it to always remove any kind of wallet encryption (currently: spending PIN) for the back up. I've been pondering with this for a long time. The issue is people tend to forget (despite warnings in the app) that they still might need to remember their (possibly outdated) spending PIN along with their backup password. That seems too much of an ask for normal users. As a positive side effect, merging such a change in advance will reduce the size of this PR. Would you be willing to spearhead such a PR? |
| @@ -24,6 +24,7 @@ dependencies { | |||
| implementation "androidx.room:room-runtime:2.6.1" | |||
| annotationProcessor "androidx.room:room-compiler:2.6.1" | |||
| implementation 'com.google.guava:guava:31.1-android' | |||
| implementation 'androidx.biometric:biometric:1.2.0-alpha04' | |||
There was a problem hiding this comment.
Can you say if the native Android API would be an option here?
For one, I'm always reluctant to add new dependencies due to supply chain attacks being on the rise. And this dep comes from a not-really-trusted Google repo only.
And second, it's an alpha.
| @@ -274,6 +281,9 @@ public Dialog onCreateDialog(final Bundle savedInstanceState) { | |||
| if (showEncryptedMessage) { | |||
| message.append("\n\n"); | |||
| message.append(getString(R.string.restore_wallet_dialog_success_encrypted)); | |||
| } else { | |||
| message.append("\n\n"); | |||
There was a problem hiding this comment.
Instead of adding a message here, we could rework how tutorial hints work and add something like a "security checklist" to the main flow.
…le attack vector. Co-authored-by: Sebastian Nicol <sebastian.r.nicol@gmail.com> Co-authored-by: Oliver Aemmer <oliver.aemmer@protonmail.ch>
Currently the keys in the wallet can be encrypted using a spending pin. With this PR we propose a new encryption type which leverages the Android KeyStore and Biometric Authentication to encrypt the keys. It is not a replacement of the spending pin but an addition.
This change requires additional changes in bitcoinj. The files in wallet/libs/ are built from the proposal_keycrypterfactory_new_encryptiontype branch.
This PR is not meant to be merged in the current state. If the proposed changes are accepted, the mentioned additional changes in bitcoinj would need to be merged and published first.
Change log of 31c78f7:
Change log of 894a9bf:
With Android Studio 2022.3.1 Patch 2 this current settings should build without additional changes
Authors: Sebastian Nicol and Oliver Aemmer