Signal-android: Bug: Signal is over-requesting the storage permission

Created on 3 Jun 2018 · 7Comments · Source: signalapp/Signal-Android

In light of the recent HTML parsing bugs for the Desktop app and associated controversies relating to Signal's security, I was thinking about the runtime permissions in the Android app and whether or not they may be over-requested. I'm thinking particularly about the storage write permission.

A best security practice would be to request only the minimal set of permissions needed and no more. The problem with asking for unlimited access to write anywhere in storage is that it is simply not needed in many situations.

To paraphrase, well, myself:

For reading and/or writing a file, there is actually a way to completely avoid having to ask for and then check for the permission and thus bypass the whole request. The solution, which I believe works in API 19+, eliminates the need for the WRITE_EXTERNAL_STORAGE permission by having a DocumentsProvider act as a "middleman" to basically ask the user on your app's behalf "please select the specific file to write to" (ie, a "file picker") and then once the user chooses a file (or types in a new file name), the app is now magically granted permission to do so for that Uri, since the user has specifically granted it.

No "official" WRITE_EXTERNAL_STORAGE permission needed.

This way of kind of borrowing permissions is part of the Storage Access Framework, and a discussion about this was made at the Big Android BBQ by Ian Lake. Here's a video called Forget the Storage Permission: Alternatives for sharing and collaborating that goes over the basics and how you specifically use it to bypass the WRITE_EXTERNAL_STORAGE permission requirement entirely.

In other words, the user can grant a temporary permission for a single file (or in API 21, a folder) and never have to be explicitly asked to grant the full app-level storage permission. When Signal sends a file, this would be done automatically (since you already select the file to send). When receiving a file and wanting to save it to a particular directory, the target directory & filename can similarly be selected by the user and the one-use permission would be granted to save the file without ever needing to request/grant the full app permission. These limited-time permission-grants can optionally be persisted across system restarts or abandoned, say, once the file is saved and now the permission is no longer needed.

The advantage here is to stop Signal from being able to access any file or folder across all storage devices (local+cloud) in case there is some future vulnerability that might allow exposure to all accessible files (similar to what happened w/ the Desktop app). Why ask for a broad permission you don't really need? If some kind of bug ever let an attacker remotely access the Signal device's storage systems, the damage could be contained considerably.

Incidentally, Signal also requests the storage write permission just to access images-- but the SAF allows you to select images from any DocumentsProvider (such as the gallery or other image providers) without needing the storage permission-- just filter to narrow the request by MIME type.

I'm just throwing this out there for discussion. Obviously the old mechanism would still need to be available for APIs prior to the Storage Access Framework becoming available. But for running Signal on Android and more and more on Chrome OS, a user probably shouldn't give the app access beyond the narrowest scope in which it's needed.

Cheers!

Source

fat-tire

👍2

Most helpful comment

Hi.. I'm a little confused-- Perhaps this issue could be better titled as "Bug: Signal is over-requesting the storage permission" but to be clear, this _is_ potentially a security issue I'm reporting. I could submit a PR that might address it, but I'd like to receive some guidance first to understand if I'm missing anything or if anyone else has any thoughts on the matter.

I'm not proposing a new feature- this relates to what I see as a problem in the current code as-is.

Update: Changed issue title per clarity.

fat-tire on 4 Jun 2018

👍3

All 7 comments

Hi there,

thank you for the nice post. We use this repository just for bugs. Can you move this to the community board please and Close the issue here.

https://whispersystems.discoursehosting.net/

Kamkata on 4 Jun 2018

I'm not proposing a new feature- this relates to what I see as a problem in the current code as-is.

Update: Changed issue title per clarity.

fat-tire on 4 Jun 2018

👍3

(That said, if you still feel it should be moved I can move it... Thanks to the reported MSFT takeover I'll probably be bailing from github soon so would probably be more interested in following up there anyway.)

fat-tire on 4 Jun 2018

👍1

Hi,

the reason i said you should move it is, because i'm sure some mod will close it anyway. I agree. It is a bug. If you wan't just open it there too. So we have it ready if this should be closed.

Moxie will ask not to delete the issue template for example. Anyway thanks for the Report.

Kamkata on 4 Jun 2018

Great to read a well researched topic with detailed references. In case you missed it after the spring clean the preference here has been to use the tracker just for bugs and move other discussion to the forums. The line is not always clear but the issue you raised is definitely a higher level topic/bug than just a regular bug which has simple, actionable next steps. The design choices with permissions are probably not the best as the code predates Android run time permissions but I think the best way is to continue this discussion and the solution you proposed in the forums.