signalfx / splunk-otel-android

Android RUM library and instrumentation
https://docs.splunk.com/Observability/gdi/get-data-in/rum/android/get-android-data-in.html
Apache License 2.0
38 stars 27 forks source link

remove READ_PHONE_STATE #733

Open ber4444 opened 8 months ago

ber4444 commented 8 months ago

The SDK adds this permission here: https://github.com/signalfx/splunk-otel-android/blob/a3d3208d5bac9d2d0e895a0cc29a58f16265e6af/splunk-otel-android/src/main/AndroidManifest.xml#L6

It's a security finding with the following notes:

The listed permissions give access to sensitive user data, and could expose this data. This may also be viewed as a privacy violation or even malware.

Unless required by the App, remove permission requests that are unsafe, unnecessary, or used by known malware. This will ensure that the App is not accessing user private data.

android.permission.READ_PHONE_STATE: This permission allows read only access to phone state. Developers, usually use this permission to detect when the user receives a call. However, this permission also provides the application access to sensitive hardware identifiers such as the IMEI, SimSerialNumber, SubscriberID.

breedx-splk commented 8 months ago

Thanks @ber4444 , we will take this under advisement. I forget the exact specific thing, but I believe that the permission is included so that we can do thorough network detection, especially on older versions of Android. See the NetworkDetector in the upstream OpenTelemetry repository.

I wonder if this is something that we can turn off by default and require developers to opt-into this feature. That would be a breaking change that could come as a surprise to existing users (and would probably require a major version bump as a result).

ber4444 commented 8 months ago

Is it not an optional permission in Open telemetry? If not, it should be, and existing users won't notice a thing since they already added this permission, thinking it was mandatory.

breedx-splk commented 8 months ago

It's not currently optional in otel. If you look above your last comment you'll see that I opened an issue there to also track this.

I see your point, though, that if it's already brought it from upstream that it may not be needed here.