From 3927ddec3e79a16e3e0b3ced1f2d26a709cf2e91 Mon Sep 17 00:00:00 2001
From: Alexander Bakker <ab@alexbakker.me>
Date: Sun, 9 Oct 2022 11:27:20 +0200
Subject: [PATCH] Make vault lock intent used in notifications more explicit

Aegis can display a notification to the user as a reminder that the vault is
unlocked. If the user taps the notification, the vault is locked. CodeQL
reported that Aegis may be vulnerable to CWE-927, because of the use of an
implicit intent wrapped by a PendingIntent in that notification.

This does not appear to be exploitable in our case, because we use
``PendingIntent.getBroadcast`` and explicitly set the action of the wrapped
intent. Aegis also does not read or act on any information from the received
intent. This means that a malicious app cannot launch activities or send a
broadcast with a different action, as is common with these type of weakness. The
worst an app with notification access can do, is lock the vault.

Either way, it's good to make the intent explicit, so this patch addresses that.
Additionally, for API level 23 and up, we've made the wrapped intent immutable a
while back.

We'd like to thank John Rune, who ran a CodeQL scan on the Aegis codebase and
privately disclosed this finding to us.
---
 app/src/main/AndroidManifest.xml              |  7 ++++
 .../aegis/AegisApplicationBase.java           | 20 -----------
 .../aegis/receivers/VaultLockReceiver.java    | 34 +++++++++++++++++++
 .../aegis/services/NotificationService.java   | 25 ++++++++------
 4 files changed, 56 insertions(+), 30 deletions(-)
 create mode 100644 app/src/main/java/com/beemdevelopment/aegis/receivers/VaultLockReceiver.java

diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml
index c5bcd524..c3c5a1e9 100644
--- a/app/src/main/AndroidManifest.xml
+++ b/app/src/main/AndroidManifest.xml
@@ -93,6 +93,13 @@
 
         <service android:name=".services.NotificationService" />
 
+        <receiver android:name=".receivers.VaultLockReceiver" android:exported="false">
+            <intent-filter>
+                <action android:name="${applicationId}.LOCK_VAULT" />
+                <action android:name="android.intent.action.SCREEN_OFF" />
+            </intent-filter>
+        </receiver>
+
         <provider
             android:name="androidx.core.content.FileProvider"
             android:authorities="${fileProviderAuthority}"
diff --git a/app/src/main/java/com/beemdevelopment/aegis/AegisApplicationBase.java b/app/src/main/java/com/beemdevelopment/aegis/AegisApplicationBase.java
index bb8a5ec4..0f22257b 100644
--- a/app/src/main/java/com/beemdevelopment/aegis/AegisApplicationBase.java
+++ b/app/src/main/java/com/beemdevelopment/aegis/AegisApplicationBase.java
@@ -3,10 +3,7 @@ package com.beemdevelopment.aegis;
 import android.app.Application;
 import android.app.NotificationChannel;
 import android.app.NotificationManager;
-import android.content.BroadcastReceiver;
-import android.content.Context;
 import android.content.Intent;
-import android.content.IntentFilter;
 import android.content.pm.ShortcutInfo;
 import android.content.pm.ShortcutManager;
 import android.graphics.drawable.Icon;
@@ -35,7 +32,6 @@ import dagger.hilt.components.SingletonComponent;
 
 public abstract class AegisApplicationBase extends Application {
     private static final String CODE_LOCK_STATUS_ID = "lock_status_channel";
-    private static final String CODE_LOCK_VAULT_ACTION = "lock_vault";
 
     private VaultManager _vaultManager;
 
@@ -52,13 +48,6 @@ public abstract class AegisApplicationBase extends Application {
         Iconics.init(this);
         Iconics.registerFont(new MaterialDesignIconic());
 
-        // listen for SCREEN_OFF events
-        ScreenOffReceiver receiver = new ScreenOffReceiver();
-        IntentFilter intentFilter = new IntentFilter();
-        intentFilter.addAction(Intent.ACTION_SCREEN_OFF);
-        intentFilter.addAction(CODE_LOCK_VAULT_ACTION);
-        registerReceiver(receiver, intentFilter);
-
         // lock the app if the user moves the application to the background
         ProcessLifecycleOwner.get().getLifecycle().addObserver(new AppLifecycleObserver());
 
@@ -121,15 +110,6 @@ public abstract class AegisApplicationBase extends Application {
         }
     }
 
-    private class ScreenOffReceiver extends BroadcastReceiver {
-        @Override
-        public void onReceive(Context context, Intent intent) {
-            if (_vaultManager.isAutoLockEnabled(Preferences.AUTO_LOCK_ON_DEVICE_LOCK)) {
-                _vaultManager.lock(false);
-            }
-        }
-    }
-
     @EarlyEntryPoint
     @InstallIn(SingletonComponent.class)
     interface EntryPoint {
diff --git a/app/src/main/java/com/beemdevelopment/aegis/receivers/VaultLockReceiver.java b/app/src/main/java/com/beemdevelopment/aegis/receivers/VaultLockReceiver.java
new file mode 100644
index 00000000..75079d1a
--- /dev/null
+++ b/app/src/main/java/com/beemdevelopment/aegis/receivers/VaultLockReceiver.java
@@ -0,0 +1,34 @@
+package com.beemdevelopment.aegis.receivers;
+
+import android.content.BroadcastReceiver;
+import android.content.Context;
+import android.content.Intent;
+
+import com.beemdevelopment.aegis.BuildConfig;
+import com.beemdevelopment.aegis.Preferences;
+import com.beemdevelopment.aegis.vault.VaultManager;
+
+import javax.inject.Inject;
+
+import dagger.hilt.android.AndroidEntryPoint;
+
+@AndroidEntryPoint
+public class VaultLockReceiver extends BroadcastReceiver {
+    public static final String ACTION_LOCK_VAULT
+            = String.format("%s.LOCK_VAULT", BuildConfig.APPLICATION_ID);
+
+    @Inject
+    protected VaultManager _vaultManager;
+
+    @Override
+    public void onReceive(Context context, Intent intent) {
+        if (!intent.getAction().equals(ACTION_LOCK_VAULT)
+                && !intent.getAction().equals(Intent.ACTION_SCREEN_OFF)) {
+            return;
+        }
+
+        if (_vaultManager.isAutoLockEnabled(Preferences.AUTO_LOCK_ON_DEVICE_LOCK)) {
+            _vaultManager.lock(false);
+        }
+    }
+}
diff --git a/app/src/main/java/com/beemdevelopment/aegis/services/NotificationService.java b/app/src/main/java/com/beemdevelopment/aegis/services/NotificationService.java
index fa22d0af..65b7a0ca 100644
--- a/app/src/main/java/com/beemdevelopment/aegis/services/NotificationService.java
+++ b/app/src/main/java/com/beemdevelopment/aegis/services/NotificationService.java
@@ -11,13 +11,14 @@ import androidx.annotation.Nullable;
 import androidx.core.app.NotificationCompat;
 import androidx.core.app.NotificationManagerCompat;
 
+import com.beemdevelopment.aegis.BuildConfig;
 import com.beemdevelopment.aegis.R;
+import com.beemdevelopment.aegis.receivers.VaultLockReceiver;
 
 public class NotificationService extends Service {
-    public static final int VAULT_UNLOCKED_ID = 1;
+    private static final int NOTIFICATION_VAULT_UNLOCKED = 1;
 
-    private static final String CODE_LOCK_STATUS_ID = "lock_status_channel";
-    private static final String CODE_LOCK_VAULT_ACTION = "lock_vault";
+    private static final String CHANNEL_ID = "lock_status_channel";
 
     @Override
     public int onStartCommand(Intent intent,int flags, int startId){
@@ -28,28 +29,32 @@ public class NotificationService extends Service {
 
     @SuppressLint("LaunchActivityFromNotification")
     public void serviceMethod() {
-        int flags = 0;
+        Intent intent = new Intent(this, VaultLockReceiver.class);
+        intent.setAction(VaultLockReceiver.ACTION_LOCK_VAULT);
+        intent.setPackage(BuildConfig.APPLICATION_ID);
+
+        int flags = PendingIntent.FLAG_ONE_SHOT;
         if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
             flags |= PendingIntent.FLAG_IMMUTABLE;
         }
-        Intent intentAction = new Intent(CODE_LOCK_VAULT_ACTION);
-        PendingIntent lockDatabaseIntent = PendingIntent.getBroadcast(this, 1, intentAction, flags);
-        NotificationCompat.Builder builder = new NotificationCompat.Builder(this, CODE_LOCK_STATUS_ID)
+        PendingIntent pendingIntent = PendingIntent.getBroadcast(this, 1, intent, flags);
+
+        NotificationCompat.Builder builder = new NotificationCompat.Builder(this, CHANNEL_ID)
                 .setSmallIcon(R.drawable.ic_aegis_notification)
                 .setContentTitle(getString(R.string.app_name_full))
                 .setContentText(getString(R.string.vault_unlocked_state))
                 .setPriority(NotificationCompat.PRIORITY_DEFAULT)
                 .setOngoing(true)
-                .setContentIntent(lockDatabaseIntent);
+                .setContentIntent(pendingIntent);
 
         NotificationManagerCompat notificationManager = NotificationManagerCompat.from(this);
-        notificationManager.notify(VAULT_UNLOCKED_ID, builder.build());
+        notificationManager.notify(NOTIFICATION_VAULT_UNLOCKED, builder.build());
     }
 
     @Override
     public void onDestroy() {
         NotificationManagerCompat notificationManager = NotificationManagerCompat.from(this);
-        notificationManager.cancel(VAULT_UNLOCKED_ID);
+        notificationManager.cancel(NOTIFICATION_VAULT_UNLOCKED);
         super.onDestroy();
     }