zo0r / react-native-push-notification

React Native Local and Remote Notifications
MIT License
6.77k stars 2.05k forks source link

Remediation for Implicit PendingIntent Vulnerability #2064

Open git-user-1337 opened 3 years ago

git-user-1337 commented 3 years ago

Hello! We are happily using react-native-push-notification in our app. At our latest update, Google contacted us because a potential vulnerability due to implicit pending-intents within our app. A complete article that describes this security breach can be found here. After searching through our code I found these lines of code, which look similar to the implicit intent pattern described in the article:

// RNPushNotificationHelper.java
Intent notificationIntent = new Intent(context, RNPushNotificationPublisher.class);
...
return PendingIntent.getBroadcast(context, notificationID, notificationIntent, PendingIntent.FLAG_UPDATE_CURRENT);

Has anyone using this package received the same warning from google? Do you think the above method could be related to that? Thanks for your help!

raynormw commented 3 years ago

Hi, we also have the same issue. Is there any workaround for solving this issue? or we have to wait the next version release?

Thank you so much.

DDani commented 3 years ago

We are facing the same issue here since a couple of days ago: we can't release new versions of our app due this. If there is not immediate solution, we can perhaps open a PR for this.

Dallas62 commented 3 years ago

Hi, I'm not available this month for a fix, a PR is welcome. I may have few hours to release it if you have a PR this month. Or you will have to wait for August. Regards

mussegam commented 3 years ago

Hi,

Would it be enough to change the previous code to something like this? My android knowledge is a bit lacking hehe

Intent notificationIntent = new Intent(context, RNPushNotificationPublisher.class);
notificationIntent.setPackage(packageName);
notificationIntent.setAction(packageName + ".ACTION");
bjacog commented 3 years ago

This might help https://github.com/react-native-share/react-native-share/issues/1043 and https://github.com/react-native-share/react-native-share/pull/1046/files

bjacog commented 3 years ago

So I manually patched the version I had been using, it was really old so you might need to adjust for your version. Once I have updated my apps to the latest version of this lib I can make a PR to resolve this if it is still a problem with latest.

The app that passed review is using react-native-push-notification@3.1.9. Here is my patch that passed review:

--- a/node_modules/react-native-push-notification/android/src/main/java/com/dieam/reactnativepushnotification/modules/RNPushNotificationHelper.java
+++ b/node_modules/react-native-push-notification/android/src/main/java/com/dieam/reactnativepushnotification/modules/RNPushNotificationHelper.java
@@ -23,6 +23,7 @@ import androidx.core.app.NotificationCompat;
 import android.util.Log;

 import com.facebook.react.bridge.ReadableMap;
+import com.facebook.react.bridge.ReactContext;

 import org.json.JSONArray;
 import org.json.JSONException;
@@ -73,6 +74,7 @@ public class RNPushNotificationHelper {
         int notificationID = Integer.parseInt(bundle.getString("id"));

         Intent notificationIntent = new Intent(context, RNPushNotificationPublisher.class);
+        notificationIntent.setPackage(context.getPackageName());
         notificationIntent.putExtra(RNPushNotificationPublisher.NOTIFICATION_ID, notificationID);
         notificationIntent.putExtras(bundle);

@@ -276,6 +278,7 @@ public class RNPushNotificationHelper {
             notification.setStyle(new NotificationCompat.BigTextStyle().bigText(bigText));

             Intent intent = new Intent(context, intentClass);
+            intent.setPackage(context.getPackageName());
             intent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP);
             bundle.putBoolean("userInteraction", true);
             intent.putExtra("notification", bundle);
@@ -359,6 +362,7 @@ public class RNPushNotificationHelper {
                     }

                     Intent actionIntent = new Intent(context, intentClass);
+                    actionIntent.setPackage(context.getPackageName());
                     actionIntent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP);
                     actionIntent.setAction(context.getPackageName() + "." + action);
git-user-1337 commented 3 years ago

Hey @bjacog, thanks for your suggestion, with the adjustments it seems to work just fine. But I identified the following issue: If users update from a previous version of the app with some local notifications scheduled, we can't seem to cancel / edit those old notifications in the new app. E.g. in my case I am calling cancelAllLocalNotifications sometimes to clear all local notifications, but that won't clear any notifications scheduled before the update. I guess it's related to the scheduledNotificationsPersistence instance which might not contain the old notifications anymore, but I don't really understand why. Do you have any clue what is happening there? Thanks again for the help :)

bjacog commented 3 years ago

@git-user-1337 I'm no export on Android or push notifications but the only change is setting package context so I imagine the cancelAllLocalNotifications perhaps use package context to filter the notifications. So before update: notifications do not have the package context, then after updating the call to cancelAllLocalNotifications now looks for notifications with context?

It is a shot in the dark, sorry I don't have time atm to investigate it a bit more.

aotaduy commented 2 years ago

Im having this issue even with 8.1.1. Can we create a pull request based on @bjacog changes?

github-actions[bot] commented 2 weeks ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.