RUM-5750 Use NO_EXPORT_FLAG for BroadcastReceiver on API above 26 #2170
Conversation
mariusc83
force-pushed
the
mconstantin/rum-5750/handle-security-issue-on-broadcast-receivers
branch
from
1f74d01 to
1bac553
Compare
| fun registerReceiver( | ||
| context: Context, | ||
| filter: IntentFilter | ||
| ): Intent? { | ||
| val intent = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| context.registerReceiver(this, filter, Context.RECEIVER_NOT_EXPORTED) | ||
| } else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { |
There was a problem hiding this comment.
Normally we should use ContextCompat here:
https://developer.android.com/reference/androidx/core/content/ContextCompat#RECEIVER_NOT_EXPORTED()
but not sure if we should bring the ContextCompat dependency only for this reason. The drawback on this approach would be in case that Flag value will be changed in the future (quite unlikely) . he worse thing it could happen is that this flag to change later but I checked and the SDK is not crashing it is just ignoring it. On the other side if the flag value (logic will change under the hood) we might end up with a different access rights for this receiver exposing again a security issue. I think this is highly unlikely but reviewers please have this in mind.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2170 +/- ##
========================================
Coverage 70.02% 70.02%
========================================
Files 726 726
Lines 27001 26992 -9
Branches 4525 4524 -1
========================================
- Hits 18906 18900 -6
- Misses 6819 6828 +9
+ Partials 1276 1264 -12
|
| @@ -18,13 +18,15 @@ internal abstract class ThreadSafeReceiver : BroadcastReceiver() { | |||
|
|
|||
| val isRegistered = AtomicBoolean(false) | |||
|
|
|||
| @SuppressLint("UnspecifiedRegisterReceiverFlag") | |||
| @SuppressLint("WrongConstant", "UnspecifiedRegisterReceiverFlag") | |||
There was a problem hiding this comment.
Following on from the dev chat, if we have to suppress perhaps we should document the reason in a comment
| testedReceiver = TestableThreadSafeReceiver() | ||
| } | ||
|
|
||
| // r |
There was a problem hiding this comment.
this comment looks like it got redacted
mariusc83
force-pushed
the
mconstantin/rum-5750/handle-security-issue-on-broadcast-receivers
branch
from
1bac553 to
6232b7d
Compare
| mockIntentFilter, | ||
| ThreadSafeReceiver.RECEIVER_NOT_EXPORTED_COMPAT | ||
| ) | ||
| assertThat(this.testedReceiver.isRegistered.get()).isTrue() |
There was a problem hiding this comment.
minor: this can be removed. Same for other similar lines in this file.
| assertThat(this.testedReceiver.isRegistered.get()).isTrue() | |
| assertThat(testedReceiver.isRegistered.get()).isTrue() |
mariusc83
deleted the
mconstantin/rum-5750/handle-security-issue-on-broadcast-receivers
branch
What does this PR do?
This issue was reported by a customer. Their security check tool detected this issue in our SDK where we do not register a receiver with NO_EXPORT_FLAG in API 33 to 26. This method actually exists also below API 33 up to API 26 but the flag Context.NO_EXPORT_FLAG was not available so that's the reason we skipped it.
I chose not to use the
ContextCompat.NO_EXPORT_FLAGjust to not have to introduce this new dependency just for this case and added a0x4local flag myself (same value as the one in Context and ContextCompat). The worse thing it could happen is that this flag to change later but I checked and the SDK is not crashing it is just ignoring it. On the other side if the flag value (logic will change under the hood) we might end up with a different access rights for this receiver exposing again a security issue. I think this is highly unlikely but reviewers please have this in mind.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)