Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 61274fc6 authored by Matías Hernández's avatar Matías Hernández
Browse files

Fix allowlist token issues 

1) Don't accept enqueued notifications with an unexpected token.
2) Ensure allowlist token matches for all parceled and unparceled notifications (by only using the "root notification" one).
3) Simplify cookie usage in allowlist token serialization.
4) Ensure group summary (and any notifications added directly by NMS) have the correct token.

Bug: 328254922
Bug: 305695605
Test: atest NotificationManagerServiceTest ParcelTest CloseSystemDialogsTest + manually
Change-Id: I232e9b74eece745560ed2e762071b48984b3f176
Merged-In: I232e9b74eece745560ed2e762071b48984b3f176
parent dc993735

core/java/android/app/Notification.java

+37 −11
Original line number Diff line number Diff line
@@ -2565,8 +2565,11 @@ public class Notification implements Parcelable 
        if (mAllowlistToken == null) {

            mAllowlistToken = processAllowlistToken;

        }

        // Propagate this token to all pending intents that are unmarshalled from the parcel.

        // Propagate this token to all pending intents that are unmarshalled from the parcel,

        // or keep the one we're already propagating, if that's the case.

        if (!parcel.hasClassCookie(PendingIntent.class)) {

            parcel.setClassCookie(PendingIntent.class, mAllowlistToken);

        }















        when = parcel.readLong();















        creationTime = parcel.readLong();










@@ -3026,10 +3029,25 @@ public class Notification implements Parcelable













                }















            });















        }













        try {













            boolean mustClearCookie = false;













            if (!parcel.hasClassCookie(Notification.class)) {













                // This is the "root" notification, and not an "inner" notification (including













                // publicVersion or anything else that might be embedded in extras). So we want













                // to use its token for every inner notification (might be null).













                parcel.setClassCookie(Notification.class, mAllowlistToken);













                mustClearCookie = true;













            }















            try {















                // IMPORTANT: Add marshaling code in writeToParcelImpl as we















                // want to intercept all pending events written to the parcel.















                writeToParcelImpl(parcel, flags);













            } finally {













                if (mustClearCookie) {













                    parcel.removeClassCookie(Notification.class, mAllowlistToken);













                }













            }



























            synchronized (this) {















                // Must be written last!















                parcel.writeArraySet(allPendingIntents);









@@ -3044,7 +3062,10 @@ public class Notification implements Parcelable













    private void writeToParcelImpl(Parcel parcel, int flags) {















        parcel.writeInt(1);



























        parcel.writeStrongBinder(mAllowlistToken);













        // Always use the same token as the root notification (might be null).













        IBinder rootNotificationToken = (IBinder) parcel.getClassCookie(Notification.class);













        parcel.writeStrongBinder(rootNotificationToken);



























        parcel.writeLong(when);















        parcel.writeLong(creationTime);















        if (mSmallIcon == null && icon != 0) {










@@ -3400,16 +3421,21 @@ public class Notification implements Parcelable













     * Sets the token used for background operations for the pending intents associated with this















     * notification.















     *













     * This token is automatically set during deserialization for you, you usually won't need to













     * call this unless you want to change the existing token, if any.













     * Note: Should <em>only</em> be invoked by NotificationManagerService, since this is normally













     * populated by unparceling (and also used there). Any other usage is suspect.















     *















     * @hide















     */













    public void clearAllowlistToken() {













        mAllowlistToken = null;













    public void overrideAllowlistToken(IBinder token) {













        mAllowlistToken = token;















        if (publicVersion != null) {













            publicVersion.clearAllowlistToken();













            publicVersion.overrideAllowlistToken(token);













        }















    }

























    /** @hide */













    public IBinder getAllowlistToken() {













        return mAllowlistToken;















    }





























    /**

































core/java/android/os/Parcel.java



+22
−0




























Original line number
Diff line number
Diff line








@@ -782,6 +782,28 @@ public final class Parcel {













        return mClassCookies != null ? mClassCookies.get(clz) : null;















    }





























    /** @hide */













    public void removeClassCookie(Class clz, Object expectedCookie) {













        if (mClassCookies != null) {













            Object removedCookie = mClassCookies.remove(clz);













            if (removedCookie != expectedCookie) {













                Log.wtf(TAG, "Expected to remove " + expectedCookie + " (with key=" + clz













                        + ") but instead removed " + removedCookie);













            }













        } else {













            Log.wtf(TAG, "Expected to remove " + expectedCookie + " (with key=" + clz













                    + ") but no cookies were present");













        }













    }



























    /**













     * Whether {@link #setClassCookie} has been called with the specified {@code clz}.













     * @hide













     */













    public boolean hasClassCookie(Class clz) {













        return mClassCookies != null && mClassCookies.containsKey(clz);













    }





























    /** @hide */















    public final void adoptClassCookies(Parcel from) {















        mClassCookies = from.mClassCookies;

































core/tests/coretests/src/android/os/ParcelTest.java



+49
−0




























Original line number
Diff line number
Diff line








@@ -16,18 +16,23 @@





























package android.os;





























import static com.google.common.truth.Truth.assertThat;





























import static org.junit.Assert.assertEquals;















import static org.junit.Assert.assertFalse;















import static org.junit.Assert.assertThrows;















import static org.junit.Assert.assertTrue;































import android.platform.test.annotations.Presubmit;













import android.util.Log;































import androidx.test.runner.AndroidJUnit4;































import org.junit.Test;















import org.junit.runner.RunWith;





























import java.util.ArrayList;





























@Presubmit















@RunWith(AndroidJUnit4.class)















public class ParcelTest {










@@ -239,4 +244,48 @@ public class ParcelTest {













        assertThrows(IllegalArgumentException.class, () -> Parcel.compareData(pA, -1, pB, iB, 0));















        assertThrows(IllegalArgumentException.class, () -> Parcel.compareData(pA, 0, pB, -1, 0));















    }



























    @Test













    public void testClassCookies() {













        Parcel p = Parcel.obtain();













        assertThat(p.hasClassCookie(ParcelTest.class)).isFalse();



























        p.setClassCookie(ParcelTest.class, "string_cookie");













        assertThat(p.hasClassCookie(ParcelTest.class)).isTrue();













        assertThat(p.getClassCookie(ParcelTest.class)).isEqualTo("string_cookie");



























        p.removeClassCookie(ParcelTest.class, "string_cookie");













        assertThat(p.hasClassCookie(ParcelTest.class)).isFalse();













        assertThat(p.getClassCookie(ParcelTest.class)).isEqualTo(null);



























        p.setClassCookie(ParcelTest.class, "to_be_discarded_cookie");













        p.recycle();













        assertThat(p.getClassCookie(ParcelTest.class)).isNull();













    }



























    @Test













    public void testClassCookies_removeUnexpected() {













        Parcel p = Parcel.obtain();



























        assertLogsWtf(() -> p.removeClassCookie(ParcelTest.class, "not_present"));



























        p.setClassCookie(ParcelTest.class, "value");



























        assertLogsWtf(() -> p.removeClassCookie(ParcelTest.class, "different"));













        assertThat(p.getClassCookie(ParcelTest.class)).isNull(); // still removed



























        p.recycle();













    }



























    private static void assertLogsWtf(Runnable test) {













        ArrayList<Log.TerribleFailure> wtfs = new ArrayList<>();













        Log.TerribleFailureHandler oldHandler = Log.setWtfHandler(













                (tag, what, system) -> wtfs.add(what));













        try {













            test.run();













        } finally {













            Log.setWtfHandler(oldHandler);













        }













        assertThat(wtfs).hasSize(1);













    }















}
























services/core/java/com/android/server/notification/NotificationManagerService.java
100755 → 100644



+16
−2




























Original line number
Diff line number
Diff line








@@ -640,7 +640,7 @@ public class NotificationManagerService extends SystemService {



























    private static final int MY_UID = Process.myUid();















    private static final int MY_PID = Process.myPid();













    private static final IBinder ALLOWLIST_TOKEN = new Binder();













    static final IBinder ALLOWLIST_TOKEN = new Binder();















    protected RankingHandler mRankingHandler;















    private long mLastOverRateLogTime;















    private float mMaxPackageEnqueueRate = DEFAULT_MAX_NOTIFICATION_ENQUEUE_RATE;










@@ -4369,7 +4369,7 @@ public class NotificationManagerService extends SystemService {













                    // Remove background token before returning notification to untrusted app, this















                    // ensures the app isn't able to perform background operations that are















                    // associated with notification interactions.













                    notification.clearAllowlistToken();













                    notification.overrideAllowlistToken(null);















                    return new StatusBarNotification(















                            sbn.getPackageName(),















                            sbn.getOpPkg(),










@@ -6498,6 +6498,15 @@ public class NotificationManagerService extends SystemService {













                    + " trying to post for invalid pkg " + pkg + " in user " + incomingUserId);















        }



























        IBinder allowlistToken = notification.getAllowlistToken();













        if (allowlistToken != null && allowlistToken != ALLOWLIST_TOKEN) {













            throw new SecurityException(













                    "Unexpected allowlist token received from " + callingUid);













        }













        // allowlistToken is populated by unparceling, so it can be null if the notification was













        // posted from inside system_server. Ensure it's the expected value.













        notification.overrideAllowlistToken(ALLOWLIST_TOKEN);



























        checkRestrictedCategories(notification);





























        // Fix the notification as best we can.










@@ -7349,6 +7358,11 @@ public class NotificationManagerService extends SystemService {













        @Override















        public void run() {















            synchronized (mNotificationLock) {













                // allowlistToken is populated by unparceling, so it will be absent if the













                // EnqueueNotificationRunnable is created directly by NMS (as we do for group













                // summaries) instead of via notify(). Fix that.













                r.getNotification().overrideAllowlistToken(ALLOWLIST_TOKEN);



























                final Long snoozeAt =















                        mSnoozeHelper.getSnoozeTimeForUnpostedNotification(















                                r.getUser().getIdentifier(),

































services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java



+350
−4


























File changed.
Preview size limit exceeded, changes collapsed.