always use explicit Intents in PendingIntents

@grote pointed out that all the PendingIntentts need to be explicit: https://gitlab.com/fdroid/fdroidclient/merge_requests/468/diffs#note_27062813

This should be an easy fix, something like Intent.setClass() or Intent.setComponent() before every PendingIntent. Since these are always created by F-Droid to be sent to F-Droid, it should be simple.

mentioned in merge request !468 (merged)

assigned to @pserwylo

@grote: I'm taking a look at this now, and was wondering if you could provide some feedback.

I understand why what we are doing now is a bad idea, because it lets anybody send an intent to our app asking it to do something (e.g. install an update) which is bad.

Am I correct in assuming that we can no longer use context.registerReceiver( ... anonymous receiver class ...) like this. Instead, we would need to have a BroadcastReceiver specified in our AndroidManifest.xml?

If so, then I can understand how this helps with security, because the BroadcastReceiver will have exported="false", preventing other apps from sending us intents to initiate updates. It is slightly more inconvenient, but should not be too difficult to use a receiver statically registered in the manifest, and then access the notification helper singleton from that receiver.

removed assignee

how to properly implement this is not entirely clear for me. I guess the Intent that is then sent by clicking the Notification comes from "external", so the receiver needs to be exported. That's one thing to test. I thought that PendingIntents had their own kind of auth mechanism whereby the only the intended receiver can actually generate them. I.e. only F-Droid can generate the PendingIntent that is then sent to F-Droid as if it was from F-Droid. So maybe in that case, these Intents look like internal Intents after all.

I understand why what we are doing now is a bad idea, because it lets anybody send an intent to our app asking it to do something (e.g. install an update) which is bad.

Here's some more information on that: https://www.securecoding.cert.org/confluence/display/android/DRD21-J.+Always+pass+explicit+intents+to+a+PendingIntent

Am I correct in assuming that we can no longer use context.registerReceiver( ... anonymous receiver class ...) like this. Instead, we would need to have a BroadcastReceiver specified in our AndroidManifest.xml?

I don't think you need to specify the BroadcastReceiver in your manifest. What is important is that you do either Intent.setClass() or Intent.setPackage() on the intent that you pass to the PendingIntent.

Haven't tried it myself yet, but word is that if another application manages to hijack your pending intent, they can do stuff with the same privileges as F-Droid and even appear as if they are F-Droid. I also haven't reviewed all pending intents of the client, but there are some given to the notifications at least. Don't know how likely this hijacking scenario really is though.

If so, then I can understand how this helps with security, because the BroadcastReceiver will have exported="false", preventing other apps from sending us intents to initiate updates

Not exporting the receiver might be another appropriate solution indeed.

As far as I remember, BroadcastReceiver can be instantiated and exported at runtime, so even if its not declared in AndroidManifest.xml, it could be exported. Then that opens up the risk of rogue apps sending Intents to it.

If we can just be sure that the BroadcastReceiver is not being exported, then I think it doesn't matter whether it is created dynamically or in AndroidManifest.xml

assigned to @pserwylo

mentioned in merge request !485 (merged)

closed