feat!: use internal storage only due to Android 11 introducing insecure external storage for apps

[sc0ttdav3y]

Platforms affected

Android

Motivation and Context

• Fixes compatibility with Android 11, specifically File Size coming as 0 in android 11 #314
• Removes the default use of external storage, as it is no longer secure since Android 11

Description

Android 11 no longer supports the Environment.getExternalStorageDirectory() method currently in use, plus external storage is no longer secured to the app, so is a bad choice as a default storage location for app data.

Background

In earlier Android OSs external app storage was protected. See https://developer.android.com/training/data-storage which still lists the older behaviour — I've dropped a screenshot for convenience.

Since Android 11 this has changed. See https://developer.android.com/training/data-storage/app-specific:

Because of these changes as at Android 11, this PR updates the plugin to use internal (secure) file storage.

Testing

I've deployed this onto a physical Android 11 device and a physical Android 8.1.0 device, then started an audio recording and completed the recording to ensure the file is accessible and contains data.

See my comments in the linked issue for the error I was solving, along with the backtrace.

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[erisu]

Out of curiosity, have you tried using theContext#getExternalFilesDir(String), MediaStore, or Intent#ACTION_OPEN_DOCUMENT?

The three options above were mentioned on the Android Docs as alternatives to getExternalStorageDirectory, as it was deprecated, and offer better performance.

From Android Docs, regarding to Context#getExternalFilesDir(String)

This is like getFilesDir() in that these files will be deleted when the application is uninstalled, however there are some important differences:

• Shared storage may not always be available, since removable media can be ejected by the user. Media state can be checked using Environment#getExternalStorageState(File).
• There is no security enforced with these files. For example, any application holding Manifest.permission.WRITE_EXTERNAL_STORAGE can write to these files.

[sc0ttdav3y]

I read about that, but for my use-case it’s important that I have security enforced on these files. From what I read about the external filesystem any app can read these files in Android 11, and that doesn’t seem like a good default choice to me. I expect having file security would be the default expectation of other developers too, plus it’s in line with what you get on other platforms such as iOS so it makes sense to me for a Cordova plug-in to be consistent. Also, using an external filesystem also seems to be conditional on the presence of a mounted volume which may not always be present. I have two devices here and none have external storage. I’m not an Android expert though so I may be wrong on this. Perhaps the filesystem could be an option for the plug-in that could be configured through JS? Ultimately, it doesn’t sit right to me to not have the files secured by default.

…

On 5 Oct 2021, at 10:03 pm, エリス ***@***.***> wrote:  Out of curiosity, have you tried using theContext#getExternalFilesDir(String), MediaStore, or Intent#ACTION_OPEN_DOCUMENT? The three options above were mentioned on the Android Docs as alternatives to getExternalStorageDirectory and offer better performance. From Android Docs, regarding to Context#getExternalFilesDir(String) This is like getFilesDir() in that these files will be deleted when the application is uninstalled, however there are some important differences: Shared storage may not always be available, since removable media can be ejected by the user. Media state can be checked using Environment#getExternalStorageState(File). There is no security enforced with these files. For example, any application holding Manifest.permission.WRITE_EXTERNAL_STORAGE can write to these files. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[erisu]

For more information, see the table in https://developer.android.com/training/data-storage, which will describe that the following methods are accessable to the app-sepcific storage and other apps do not have access:

• getFilesDir() = internal (/data/data/[packagename]/files)
• getCacheDir() = internal
• getExternalFilesDir() = external (/Android/data/[packagename]/files)
• getExternalCacheDir() = external

And also see:

• the item listed above the table App-specific storage. It will mention to use internal for sensitive information.
• the use cases section: "The solution you choose depends on your specific needs:" which will point out limited space for internal storage, but not into details of what is your limit.

And lastly, just pointing out that this appears to me as a major breaking change and not just a fix. Its going from external stroage, which was the original behaviour, to the limited internal storage. As it might be major, we need to also update the documentation in this repo to explain this and provide a solution to people who have large data..

[breautek]

Context#getExternalFilesDir(String)

This would be the closest equivalent to what is currently in the repository. It's path translates to something like /storage/emulated/0/<app_id>/files

But like I said in the bug ticket:

I'm not sure on the original rationale. We should never write to an external medium unless explicitly requested.

Writing to external storage is not guaranteed to be secure (private), so why are we implicitly choosing that location without user/developer consent?

[erisu]

Anyways, regarding the PR changes, I believe they are OK for the next major release. We still need to test though.

But, I think the title and description should be updated.

Even though the changes may have fixed Android 11 because you removed the deprecated method, I do not believe it is an actual fix for the current major release (5.x) with Android 11.

I believe this PR is a feature change and that the title should be: feat!: use internal storage only.
And for the motivation section of the description, I would change:

Fixes compatibility with Android 11, specifically #314

to

I expect files to be stored internally for better security by default.

Anyways, the above changes are how I see it.

If we wanted to fix 5.x for Android 11, I think the changes should look like this: master...erisu:fix/get-external-dir

I have not tested my changes yet, but if it resolved the Android 11 issue for 5.x major, do we want to release a fix so that External continues to work with Android 11 and then introduce the breaking change?

[sc0ttdav3y]

@erisu Your approach is closer to the way the plugin works now and I understand your motivation to keep things as-is. And hey, you're a maintainer and know more than me so I'll bow to your better judgement ;-) 👍

However, please also consider that by maintaining the use of external storage like you propose, you'll be effectively introducing a security weakness to existing apps when their developers upgrade to API 31 that they may not anticipate (which is essentially all of us). At least I got an error and then got the chance to decide on which filesystem to use.

@breautek pointed out it's unclear why that rationale was ever in the plugin, but I presume it was "all things being equal", to favour a larger disk over a smaller one. Now, all things are no longer equal.

Anyway, I'm happy to rename my PR as requested.

[erisu]

@sc0ttdav3y anyways, I am not against your decision on dropping the external directory support. I also think it is a good idea, as the thread says external is less secure.

I am just thinking if we should make two PRs and two releases. Both releases will support Android 11 but one will be with external support, the other will be internal only.

Maybe the ideal roadmap:

• Step 1: Merge in my PR. This would be the actual Android 11 fix for the issue on the current major release 5.x. It will maintain the current pattern of using external dir first and then fall back to internal if not mounted).
• Step 2: Release the media plugin 5.0.4 patch.
• Step 3: Merge in your PR which will introduce the breaking feature change. Internal directory only support.
  • Step 3.1: MAYBE we could add in a flag that allows users to keep the old flow plan, external first and fallback to internal, but we default to internal only as you suggest and for better security.
• Step 4: Release media plugin 6.0.0.

This is kind of what I was thinking...

Do you think this is a good idea?

If we do not do "Step 3.1", then anyone who wants to keep using an external directory would remain on 5.0.3.

[sc0ttdav3y]

Sure, sounds like a good plan @erisu 👍🏻

Perhaps the README should also indicate the security implications in Android 11. I'm just a guest here and it's not really my place to write these things.

If doing a step 3.1, perhaps we should look at what cordova-plugin-file or cordova-plugin-camera does and align their behaviour if possible. I know the file plugin has constants for the paths exposed in JS, and I was able to use them to work around this issue. I'm not sure where the camera plugin is at, because I don't use it at present due to the ongoing exif issue.

I'm happy to help further, but I'm time poor right now so I don't want to gold-plate things ;-)

[breautek]

Ok it sounds like we will treat this PR as a breaking change, I'm going to add it to our 6.0 milestone.

cordova-plugin-file is suppose to be a more general use filesystem API. I believe it's README already states what is considered public vs private. I'm not really sure what the camera plugin does at this time, it's not something I use heavily in my own work...

Personally I agree with step 3.1. External storage is there and it does have its use cases. It just should be something you opt in to.

I may have over exaggerated the external issue a bit as well. The scoped storage changed introduced in API 29 / enforced in API 30 has made this not as much as an issue, but devices with actual removable storage, you still cannot guarantee that the data will remain private to the app. So my overall point I think still stands.

I'm just a guest here and it's not really my place to write these things.

We are all really guests here, afterall Apache projects are pretty much entirely volunteer driven! I know we've been struggling on getting plugins supported, especially with these filesystem changes.

[sc0ttdav3y]

Hey guys, I've done some more reading on scoped storage it looks like I may be wrong about security.

https://developer.android.com/about/versions/11/privacy/storage#other-apps-data indicates that apps cannot access other apps files on external storage, which seems to contradict the other articles I shared.

It seems that external storage may be private to the app by default, but its files can be accessed by other privileged apps that have android.permission.MANAGE_EXTERNAL_STORAGE. This is intended for utilities such as backup apps, virus scanners, etc. I found some insight from this stack overflow post.

If this is correct, it's okay after all to use external storage. If that's the case, @erisu's PR will be better. I'll do more research to confirm this as best I can.