Gedsh / InviZible

Android application for online privacy and security
https://invizible.net
GNU General Public License v3.0
1.43k stars 99 forks source link

Question on requested permissions #243

Closed IzzySoft closed 7 months ago

IzzySoft commented 7 months ago

My scanner just got a few additional checks implemented, which found the following in today's release:

! repo/pan.alexander.tordnscrypt_2203.apk declares risky permission(s): android.permission.READ_EXTERNAL_STORAGE android.permission.REQUEST_INSTALL_PACKAGES android.permission.QUERY_ALL_PACKAGES

Could you please clarify what InviZible needs those for? (Might be also a good topic for your your excellent Wiki for all permissions?)

Thanks in advance!

Gedsh commented 7 months ago

android.permission.READ_EXTERNAL_STORAGE

This is required to import DNSCrypt blocklists and restore the app configuration from a backup without using the system file picker. This permission is only used for SDKs below 33. Android 13 (API level 33) and above only uses the system file picker.

<uses-permission
        android:name="android.permission.READ_EXTERNAL_STORAGE"
        android:maxSdkVersion="32" />

android.permission.REQUEST_INSTALL_PACKAGES

This is necessary for updating InviZible. User consent is required to install the update.

android.permission.QUERY_ALL_PACKAGES

This is necessary to retrieve a list of device apps, including system ones. It is used in the firewall, exclude/include apps from Tor, and many other places needed for the app to work properly.

Might be also a good topic for your your excellent Wiki for all permissions?

Thanks. It is a good idea. I just need to find some free time for this.

IzzySoft commented 7 months ago

Bottom up:

I just need to find some free time for this.

Sure, at your convenience. I just saw the excellent and extensive Wiki and was puzzled that exactly the one thing I was looking for wasn't present :see_no_evil:

It is used in the firewall

Ah dang, I should have thought about that! Of course, absolutely clear – and added to the allow-list.

This is necessary for updating InviZible. User consent is required to install the update.

Hm… Self-updaters are a bit against the inclusion rules – as they'd bypass security checks here (like this one). User consent (aka opt-in) of course makes that a bit less heavy. Now please tell me the consent is also "informed" (like, not only a "Update (y/n)", but explaining where the update comes from and that additional checks (like with my repo) are not performed when installed directly from Github, and I see that I can add it to the allow-list. You know that your app in my repo updates rather fast (within 24h of your making the APK available here) – so maybe you could also disable the update check unless the installer property shows it was installed via the package updater (initial install) or by itself (self-update already performed at least once)? Then there'd be no long explanation in a popup, but just along the toggle in settings?

READ_EXTERNAL_STORAGE

Looks like I missed "MinVer: 4.4" – clear then of course. Yes, I've heard with 13 it becomes a NOP (permission without rights, not implying anything, granting nothing). Up to 9 it's still needed for media access. But I can never remember all the exceptions for this one.

So for now the allow-list reads:

      android.permission.QUERY_ALL_PACKAGES: needed to obtain a list of all apps for app-specific firewall rules
      android.permission.READ_EXTERNAL_STORAGE: required on older Android versions to import DNSCrypt blocklists and restore the app configuration from a backup

For REQUEST_INSTALL_PACKAGES I'll wait for your answer on my above "wall-of-text" :wink:

Thanks a lot!

Gedsh commented 7 months ago

the consent is also "informed" (like, not only a "Update (y/n)", but explaining where the update comes from

In the Fast settings, you can find the option to update the app automatically. It is enabled by default. When a new update is detected, the user is prompted to download and install it. After downloading, the app asks you to install it. There is no explanation as to where this update came from.

You know that your app in my repo updates rather fast (within 24h of your making the APK available here)

Because you have a beta version. This helps me test new features and make the stable version reliable.

so maybe you could also disable the update check unless the installer property shows it was installed via the package updater (initial install) or by itself (self-update already performed at least once)?

This doesn't make much sense since a user can easily download the apk from your repo and install it. It is impossible to understand where the app was downloaded from. And creating a separate beta version for your repo each time takes too much time.

Then there'd be no long explanation in a popup, but just along the toggle in settings?

What explanation would be fine?

You can use the stable version from the main F-droid repository. It doesn't have mentioned permission. But I’m not interested in this, because it doesn’t help the project development.

IzzySoft commented 7 months ago

It is enabled by default.

Hm, that doesn't meet the inclusion criteria. Could you please make that "disabled by default", with an explanation what enabling it would imply (as outlined above – and now also below) and where those updates are downloaded from?

When a new update is detected, the user is prompted to download and install it. After downloading, the app asks you to install it.

That would be OK with the above opt-in being established. Else you cannot call it informed consent – as the implications are not clear (especially with the download source not even mentioned). Please don't get me wrong: I'm not implying any malicious things here. But even the best of us can miss a thing here and there, so additional checks (as performed by my scanner) help catch things.

Because you have a beta version. This helps me test new features and make the stable version reliable.

That was the idea behind it of course. But that does not mean the inclusion criteria do not apply :wink: I'm not saying you have to remove the self-updater. With both F-Droid's repo and mine they are acceptable as long as they are opt-in with the implications of enabling them made clear.

This doesn't make much sense since a user can easily download the apk from your repo and install it. It is impossible to understand where the app was downloaded from.

I'm not talking about manual downloads (those would show the package installer as installer) – but about people using an F-Droid client to install apps and keep them updated. They expect and trust updates coming from the very same repo. If they manually downloaded the APK and installed it, they're unlikely to use an F-Droid client for keeping it updated, so in those cases your installer totally makes sense.

And creating a separate beta version for your repo each time takes too much time.

That's also not what I meant. Run adb shell pm help and watch out for the install command and its options:

-i: specify package name of installer owning the app

Each F-Droid client sets that to its own package name. Your app will set it to pan.alexander.tordnscrypt. Using adb install will set it to Android's package installer (com.google.android.packageinstaller IIRC). So you just need to look at that property, and if it's set to one of the latter two (or is null) have the installer enabled by default, else disabled. No extra build/flavor needed.

What explanation would be fine?

Going by the assumption that downloads happen from your Github releases, something along the lines:

This will enable the built-in self-updater, which checks for new versions becoming available at the Github repo's releases page {add frequency of checks here}, and offers them to you as updates. Note that when you originally installed the app from IzzyOnDroid using your favorite F-Droid client, updates received via this self-updater would bypass the additional APK checks implemented there. So in that case it's recommended|preferable {pick your term} to not enable this; updates at IzzyOnDroid usually become available the same day, and would thus just mean a short delay but extra trust.

Just a "draft phrasing" with some {placeholders} holding the needed facts, feel free to adjust of course.

Gedsh commented 7 months ago

Could you please make that "disabled by default"

This is not suitable for beta testing as there is no point in submitting bug reports for outdated beta versions.

updates received via this self-updater would bypass the additional APK checks implemented there. So in that case it's recommended|preferable {pick your term} to not enable this

There is no point in an option with such a description. No one who can read ever turns it on. It becomes the most dangerous option in the entire app :)))

But that does not mean the inclusion criteria do not apply

Unfortunately, the new criteria are not suitable for beta testing. So you can use the stable version from the main f-droid repository or simply remove the app from your repository.

Thank you for providing a cozy place for my app and helping the project all these years.

IzzySoft commented 7 months ago

OK, if you don't want to make it opt-in, I will simply remove that warning from the reports. This then means the permission will show up highlighted as warning in the permission section:

image

I understand that you want "fast updates" with beta testing. And beta-testers should understand that as well. But the few hours delay when fetching them via the F-Droid client from my repo should be worth the additional checks, wouldn't you agree?

That said: neither the app description nor its version are making clear this is a beta. So first-time installers will see the newer version in my repo and think: "Hm, why should I take an older version from F-Droid if there's a newer version available here?" – without being aware of this fact. Do you think I could/should at least "override" your title.txt by a local one, making the title InviZible Pro **BETA**? I think then I could consider adding a "reason" like "this BETA release contains a self-updater pulling directly from Github releases, which can be turned off in the app's settings". That'd remove the chocolate color and show the way e.g. QUERY_ALL_PACKAGES already does.

It becomes the most dangerous option in the entire app :)))

Becomes – or is? :speak_no_evil: :dash:

Gedsh commented 7 months ago

This then means the permission will show up highlighted as warning in the permission section

If you can add that this permission is only used for self-updating, that would be great. Some might think that invizible can install a bunch of scary apps without being noticed. But without the user's consent it is impossible in any case.

I understand that you want "fast updates" with beta testing.

In fact, the reason is not fast updates. I need reliable updates. InviZible only checks for updates when the user launches the app and only once a day. I don't know how reliably f-droid clients can do this job, and I don't want to rely on it.

Do you think I could/should at least "override" your title.txt by a local one, making the title InviZible Pro BETA?

Yes. This is a good idea. I want the user to know that this is a beta version and may contain bugs. The InviZible beta icon has the capital letters BETA. But unfortunately, not all users know what a beta version is. Therefore, I need to periodically answer this question.

"Hm, why should I take an older version from F-Droid if there's a newer version available here?"

That's not how it usually works. Stable versions have higher numbers, for example the current stable version is 6.0.4 and the beta version is 2.0.3. So the user simply selects a higher number :))

I think then I could consider adding a "reason" like "this BETA release contains a self-updater pulling directly from Github releases, which can be turned off in the app's settings". That'd remove the chocolate color and show the way e.g. QUERY_ALL_PACKAGES already does.

It would be great. Or you can simply add a description of why it is used. It's not a secret at all that the app can update itself.

Becomes – or is?

It depends on your threat model ;)

IzzySoft commented 7 months ago

If you can add that this permission is only used for self-updating

That would raise questions on why the app is in my repo at all, as it doesn't meet the inclusion criteria – which would escalate to the question how reliable even the other listed criteria are then. So I have to make clear "BETA" – and that it can be turned of. Plus of course the purpose; which then results in the suggested string, which I now applied.

I don't know how reliably f-droid clients can do this job

Err… well… Most are reliable with that. It's just the "official client" which sometimes forgets to pull its index (and then, not just once). So OK, you have a point there.

(title) Thanks, adjusted that then (and excluded to pull this file from your Fastlane).

Stable versions have higher numbers, for example the current stable version is 6.0.4 and the beta version is 2.0.3.

Ah. But those are strings. I'd bet your beta's versionCode is higher than that of the latest release? So it would be listed on top, as the most recent, and recommended for install. Maybe a better idea would be to give the beta a dedicated package name (e.g. pan.alexander.tordnscrypt.beta) – so even versionNames could be kept closer – and, as a side-effect, both could be installed at the same time (which of course only makes sense if the app can be "switched off", as both can/should not be active at the same time)?

It would be great.

Done – should become visible within the next half hour. And the "chocolate" will become invizible then, hehe… eaten.

It depends on your threat model ;)

LOL – right :rofl:

Thanks! All done then, so I close the door, ahem, issue. Thanks a lot!

Gedsh commented 7 months ago

I'd bet your beta's versionCode is higher than that of the latest release?

Nope :)

Maybe a better idea would be to give the beta a dedicated package name (e.g. pan.alexander.tordnscrypt.beta) – so even versionNames could be kept closer – and, as a side-effect, both could be installed at the same time

I did almost exactly as you said. The difference is that the beta version has the pan.alexander.tordnscrypt package. The stable version is pan.alexander.tordnscrypt.stable and they can be installed side by side.

Done – should become visible within the next half hour.

Thank you so much!

IzzySoft commented 7 months ago

I did almost exactly as you said. The difference is that the beta version has the pan.alexander.tordnscrypt package. The stable version is pan.alexander.tordnscrypt.stable and they can be installed side by side.

Ah! Then there is no danger one picks the wrong version, now the beta is clearly stating "BETA" in its name! And all looks fine now :smiley:

Thank you so much!

Gladly – and I thank you!