search

gujjwal00 / avnc

VNC Client for Android
GNU General Public License v3.0
632 stars 58 forks source link

Fingerprint unlock #51

Closed user8446 closed 1 year ago

user8446 commented 2 years ago

Hi @gujjwal00 ,

Question: Since fingerprint auth is already a part of the app for server export, is it on the roadmap to use fingerprint auth for opening the app?

gujjwal00 commented 2 years ago

I haven't thought about it, but it can be implemented. From my personal experience, app-locks can be really frustrating, especially on Android. Most apps will prompt whenever they comes to foreground, because "app-opening" is not a clear-cut event on Android (e.g. you can directly launch a vnc:// URL with AVNC, entirely bypassing Home page/activity). This frustration may discourage users from using such feature. IMO, full app-locking is best handled by system-level apps or Android itself.

So we should consider our threat-model before implementing it.

  1. Lets assume a malicious user has access to your device.
  2. AVNC never exposes any secret in plaintext, so they cannot copy it.
  3. Export to plaintext JSON requires user-authentication as you mentioned.
  4. They can connect to a server if password is saved in AVNC.

I think last one is the only attack you may want to protect against. AVNC could require authentication before connecting to a saved server (and before changing the associated setting.)

user8446 commented 2 years ago

Hi,

Number 4 is one I'm concerned with. I use private keys which are saved so one click and a whole system is completely exposed.

Personally I haven't had any issues with other apps fingerprint auth such as Aegis, KeePass2Android, and commercial banking and money apps such as PayPal & Venmo.

gujjwal00 commented 2 years ago

Hi @user8446 , due to some personal issues, I may not get much free time this month. So this feature will have to wait a bit.

user8446 commented 2 years ago

No problem at all. I hope all is well.

gujjwal00 commented 2 years ago

Thanks for understanding. Yes, all is well healthwise, just some other issues need immediate attention.

gujjwal00 commented 2 years ago

Hi @user8446 I have started working on it. Current plan to implement a per-server checkbox titled 'Biometric lock' in Advanced server options.

Any feedback is welcome.

user8446 commented 2 years ago

Hi I think that implementation would work perfect and increase security for the users!

gujjwal00 commented 2 years ago

Here is the initial implementation: app-debug.zip

You can enable "Lock this server" in advanced server options. Please test out different scenarios, like wrong finger etc.

Some thoughts:

user8446 commented 2 years ago

I have used is as my daily driver for several days now and it works as expected.

The only thing I have a question on is a few times when the fingerprint auth appears if I delayed before authorizing I had a few times when it wouldn't connect. The issue could have been anything though such as signal since I was on mobile.

When the fingerprint auth appears it shows "connecting" in the background. Are you doing anything on the network before the auth?

gujjwal00 commented 2 years ago

The only thing I have a question on is a few times when the fingerprint auth appears if I delayed before authorizing I had a few times when it wouldn't connect. The issue could have been anything though such as signal since I was on mobile.

This should not happen. If you encounter this again, please send the logs to me.

When the fingerprint auth appears it shows "connecting" in the background. Are you doing anything on the network before the auth?

No, that's just the initial state of VncActivity. Thre are three main states: Connecting, Connected, Disconnected. Before introduction of locking, app was really connecting. But now, unlock is the first step, and rest of the stuff happen after successful unlock.

I am thinking about moving this option to app settings (in Settings => Server => Login). It would apply to all saved servers. And 'Login Auto-completion' option will now depend on new locking option being disabled.

user8446 commented 2 years ago

You are probably right, auth for all or none should work for everyone.

gujjwal00 commented 1 year ago

Finally finished it: https://github.com/gujjwal00/avnc/commit/cb84cb271284c483a71c3ae82450a0a30de6ca67 It will be available in next version.

user8446 commented 1 year ago

Looking forward to it, thank you!