search

capacitor-community / background-geolocation

A Capacitor plugin that sends you geolocation updates, even while the app is in the background.
MIT License
180 stars 56 forks source link

Potential security best practice deviation for broadcasts (Snyk Code) #50

Closed bt-nia closed 2 years ago

bt-nia commented 2 years ago

Hi!

As part of our internal security review, we perform security checks with Snyk. It found the following issue in your code, which I'm unable to completely verify: image I thought I might share, since due to the potential type of data (geolocation), this might be worth mitigating.

diachedelic commented 2 years ago

Am I to understand that if an app calls sendBroadcast it is sent to every app installed on the device?

There are a few ideas on how to use the broadcastPermission parameter here. The docs are not very helpful though.

bt-nia commented 2 years ago

hey @diachedelic I'm absolutely not an Android dev - just relaying this info here. It seems though that the broadcast is sent everywhere (but just within the application?): https://developer.android.com/reference/androidx/localbroadcastmanager/content/LocalBroadcastManager#sendBroadcast(android.content.Intent) Apparently the class is deprecated, but the explanation is not very helpful to me:

This class is deprecated. LocalBroadcastManager is an application-wide event bus and embraces layer violations in your app: any component may listen events from any other. You can replace usage of LocalBroadcastManager with other implementation of observable pattern, depending on your usecase suitable options may be LiveData or reactive streams.

The way the documentation reads, it seems the risk here is maybe just within the app? That would leave SDKs as potential attack vectors.

diachedelic commented 2 years ago

Thanks, it does seems to be local to the app:

You know that the data you are broadcasting won't leave your app, so don't need to worry about leaking private data.

It is not possible for other applications to send these broadcasts to your app, so you don't need to worry about having security holes they can exploit.

Another plugin within the app could potentially spoof and intercept these broadcasts, but that does not seem like a big deal to me.

It looks like the tool above wrongly assumed that these methods were called on a Context (device-wide), not a LocalBroadcastManager (app-wide). But the methods of the latter do not accept a "permission" argument.