freeotp / freeotp-android

Apache License 2.0
1.41k stars 299 forks source link

Restore overrides password and causes missing data in new backups #323

Open Jakeler opened 1 year ago

Jakeler commented 1 year ago

Hi, I did some experiments with v2 to check if the backup function is reliable. Noticed following behavior, steps to reproduce:

  1. Fresh FreeOTP setup, set password1
  2. Add some entries manually
  3. Restore backup from other phone, which was encrypted with password2
  4. Entries are merged, both the ones added before and from the restore are usable
  5. Create new backup file
  6. Try to restore somewhere with password1 = invalid password

Instead I found the restore (6.) works only with password2. So even when switching phones it is impossible to re-encrypt with a new password. It was silently overridden from the old backup.

But even more concerning is the new backup (5.) is missing all entries from step 2, only the stuff from the restore (3.) or created afterwards is included. This is especially surprising since the app still shows everything. There is also no warning or explanation that not all is backed up. Is this a bug or should restore be only used in the initial setup?

Luckily I still keep root backups from v1, otherwise it would be a disaster.

The whole Android keystore + encrypted backup approach seems very similar to Signal (messenger), but their implementation seems more refined. Main features that improve usability:

I looked for example at andOTP and Aegis, they also have most of these features, it is possible because they save a encrypted DB and put only the master key in the keystore.

Now I did not find technical docs about FreeOTP v2 and did not have the time to look into the code in depth, but as afaik it puts the actual OTP secrets in the keystore? Could be even better for security, but the usability tradeoff might be too big.

justin-stephenson commented 1 year ago

Hi, thank you for your comments and explanation. In general, the main issues raised here are due to an (incorrect) assumption that users would set the same backup password on difference devices. I will respond in-line below.

Hi, I did some experiments with v2 to check if the backup function is reliable. Noticed following behavior, steps to reproduce:

1. Fresh FreeOTP setup, set `password1`

2. Add some entries manually

3. Restore backup from other phone, which was encrypted with `password2`

4. Entries are merged, both the ones added before and from the restore are usable

5. Create new backup file

6. Try to restore somewhere with `password1` = invalid password

Instead I found the restore (6.) works only with password2. So even when switching phones it is impossible to re-encrypt with a new password. It was silently overridden from the old backup.

Here we retrieve the decrypted master key from the restore data, and store it back into the keystore overwriting the existing master key. I need to double-check if this is required for Auto backup functionality, if not needed we should just remove this operation as part of restore.

But even more concerning is the new backup (5.) is missing all entries from step 2, only the stuff from the restore (3.) or created afterwards is included. This is especially surprising since the app still shows everything. There is also no warning or explanation that not all is backed up. Is this a bug or should restore be only used in the initial setup?

At Step 4, you now have tokens encrypted with 'password1' and 'password2' stored in the keystore. This is the state which causes subsequent restore operations to only include the newer subset of token entries, only the newer password2 tokens can be decrypted.

When tokens are restored, they get re-added into the keystore and re-encrypted with the current (non-restored) backup password. If we remove the master password/key overwrite at restore time as described above then I hope it will address this issue also, as we should then have all existing + restored tokens using the existing, non-restored master password.

Luckily I still keep root backups from v1, otherwise it would be a disaster.

The whole Android keystore + encrypted backup approach seems very similar to Signal (messenger), but their implementation seems more refined. Main features that improve usability:

* Option to verify password in the settings

* Option to set a new password for new backups

* Prompts regularly to type in the password to check

* Automatic scheduling of backups

* Restore only possible in the initial setup

I looked for example at andOTP and Aegis, they also have most of these features, it is possible because they save a encrypted DB and put only the master key in the keystore.

As we are storing token secrets securely inside the Android keystore, that causes limitations related to backup functionality. FreeOTP only has access to the token secret before it gets added into the keystore, when adding a new token or restoring from backup - this is the only time to save backup, once it goes into the keystore it does not come back out.

We can add an option to verify the password, but setting a new password would require FreeOTP removing and re-adding all existing keystore-stored tokens to encrypt these tokens with the new password. I cannot see a way to implement this as long as token secrets exist in the keystore.

Now I did not find technical docs about FreeOTP v2 and did not have the time to look into the code in depth, but as afaik it puts the actual OTP secrets in the keystore? Could be even better for security, but the usability tradeoff might be too big.

Yes definitely there is some usability to trade-off. At this point I don't think it is feasible to undo the keystore approach we are using however.

Jakeler commented 1 year ago

Thanks for your in depth answer and work!

Here we retrieve the decrypted master key from the restore data, and store it back into the keystore overwriting the existing master key. I need to double-check if this is required for Auto backup functionality, if not needed we should just remove this operation as part of restore.

Makes sense now why it behaves like it does. Would be great if you can fix/remove this. That would also allow at least a manual process to reset the app and get the backup re-encrypted with the new password on a restore.

Yes definitely there is some usability to trade-off. At this point I don't think it is feasible to undo the keystore approach we are using however.

I agree, looking at the landscape of open OTP apps it is actually very good to have one with this approach for best security.

Other apps I mentioned above use a db (shared prefs or json) with the token secrets, which is then symmetrically (AES) encrypted with a user defined key. Only optionally they use the keystore, so the user doesn't need to type the master key everytime or can use the fingerprint to unlock. When app process launches it decrypts the db and has all secrets in memory. Makes it of course much easier to export them, both for the normal user and also for an potential attacker.

On FreeOTP it is basically impossible to extract the token secrets, even if the app is running. Only tokens can be generated, which could be further protected with user auth. I did not see a option in the app, #89 was closed though, is this already implemented?

justin-stephenson commented 1 year ago

Thanks for your in depth answer and work!

Here we retrieve the decrypted master key from the restore data, and store it back into the keystore overwriting the existing master key. I need to double-check if this is required for Auto backup functionality, if not needed we should just remove this operation as part of restore.

Makes sense now why it behaves like it does. Would be great if you can fix/remove this. That would also allow at least a manual process to reset the app and get the backup re-encrypted with the new password on a restore.

I created a PR here, it would be great if you can test it. I still need to do more testing on my end before merging however.

Yes definitely there is some usability to trade-off. At this point I don't think it is feasible to undo the keystore approach we are using however.

I agree, looking at the landscape of open OTP apps it is actually very good to have one with this approach for best security.

Other apps I mentioned above use a db (shared prefs or json) with the token secrets, which is then symmetrically (AES) encrypted with a user defined key. Only optionally they use the keystore, so the user doesn't need to type the master key everytime or can use the fingerprint to unlock. When app process launches it decrypts the db and has all secrets in memory. Makes it of course much easier to export them, both for the normal user and also for an potential attacker.

On FreeOTP it is basically impossible to extract the token secrets, even if the app is running. Only tokens can be generated, which could be further protected with user auth. I did not see a option in the app, #89 was closed though, is this already implemented?

Authentication support was added but it is not currently working as expected therefore it is not yet advertised as a working feature. I did not have time to address this in the FreeOTP 2.0 release but I hope to add it as fully supported soon. It can be tested with adding lock=true to the OTP token URI fields, as is done with the Lock checkbox at https://freeotp.github.io/qrcode.html