binder: Only accept post-setup transactions from the server UID we actually authorize. (#12359)

Author: jdcormie

binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java

@@ -340,6 +340,7 @@ protected void handleSetupTransport(Parcel parcel) {
         shutdownInternal(
             Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true);
       } else {
+        restrictIncomingBinderToCallsFrom(remoteUid);
         attributes = setSecurityAttrs(attributes, remoteUid);
         authResultFuture = checkServerAuthorizationAsync(remoteUid);
         Futures.addCallback(

binder/src/main/java/io/grpc/binder/internal/BinderTransport.java

@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.util.concurrent.Futures.immediateFuture;
+import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler;
 
 import android.os.DeadObjectException;
 import android.os.IBinder;
@@ -39,6 +40,7 @@
 import io.grpc.Status;
 import io.grpc.StatusException;
 import io.grpc.binder.InboundParcelablePolicy;
+import io.grpc.binder.internal.LeakSafeOneWayBinder.TransactionHandler;
 import io.grpc.internal.ObjectPool;
 import java.util.ArrayList;
 import java.util.Iterator;
@@ -155,6 +157,7 @@ protected enum TransportState {
   private final ObjectPool<ScheduledExecutorService> executorServicePool;
   private final ScheduledExecutorService scheduledExecutorService;
   private final InternalLogId logId;
+  @GuardedBy("this")
   private final LeakSafeOneWayBinder incomingBinder;
 
   protected final ConcurrentHashMap<Integer, Inbound<?>> ongoingCalls;
@@ -476,6 +479,15 @@ private boolean handleTransactionInternal(int code, Parcel parcel) {
     }
   }
 
+  @BinderThread
+  @GuardedBy("this")
+  protected void restrictIncomingBinderToCallsFrom(int allowedCallingUid) {
+    TransactionHandler currentHandler = incomingBinder.getHandler();
+    if (currentHandler != null) {
+      incomingBinder.setHandler(newCallerFilteringHandler(allowedCallingUid, currentHandler));
+    }
+  }
+
   @Nullable
   @GuardedBy("this")
   protected Inbound<?> createInbound(int callId) {
@@ -561,6 +573,11 @@ Map<Integer, Inbound<?>> getOngoingCalls() {
     return ongoingCalls;
   }
 
+  @VisibleForTesting
+  LeakSafeOneWayBinder getIncomingBinderForTesting() {
+    return this.incomingBinder;
+  }
+
   private static Status statusFromRemoteException(RemoteException e) {
     if (e instanceof DeadObjectException || e instanceof TransactionTooLargeException) {
       // These are to be expected from time to time and can simply be retried.

binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java

@@ -73,7 +73,21 @@ public void detach() {
     setHandler(null);
   }
 
-  /** Replaces the current {@link TransactionHandler} with `handler`. */
+  /** Returns the current {@link TransactionHandler} or null if already detached. */
+  public @Nullable TransactionHandler getHandler() {
+    return handler;
+  }
+
+  /**
+   * Replaces the current {@link TransactionHandler} with `handler`.
+   *
+   * <p>{@link TransactionHandler} mutations race against incoming transactions except in the
+   * special case where the caller is already handling an incoming transaction on this same {@link
+   * LeakSafeOneWayBinder} instance. In that case, mutations are safe and the provided 'handler' is
+   * guaranteed to be used for the very next transaction. This follows from the one-at-a-time
+   * property of one-way Binder transactions as explained by {@link
+   * TransactionHandler#handleTransaction}.
+   */
   public void setHandler(@Nullable TransactionHandler handler) {
     this.handler = handler;
   }

binder/src/main/java/io/grpc/binder/internal/TransactionUtils.java

@@ -16,9 +16,13 @@
 
 package io.grpc.binder.internal;
 
+import android.os.Binder;
 import android.os.Parcel;
 import io.grpc.MethodDescriptor.MethodType;
 import io.grpc.Status;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import io.grpc.binder.internal.LeakSafeOneWayBinder.TransactionHandler;
 import javax.annotation.Nullable;
 
 /** Constants and helpers for managing inbound / outbound transactions. */
@@ -99,4 +103,24 @@ static void fillInFlags(Parcel parcel, int flags) {
     parcel.writeInt(flags);
     parcel.setDataPosition(pos);
   }
+
+  /**
+   * Decorates the given {@link TransactionHandler} with a wrapper that only forwards transactions
+   * from the given `allowedCallingUid`.
+   */
+  static TransactionHandler newCallerFilteringHandler(
+      int allowedCallingUid, TransactionHandler wrapped) {
+    final Logger logger = Logger.getLogger(TransactionUtils.class.getName());
+    return new TransactionHandler() {
+      @Override
+      public boolean handleTransaction(int code, Parcel data) {
+        int callingUid = Binder.getCallingUid();
+        if (callingUid != allowedCallingUid) {
+          logger.log(Level.WARNING, "dropped txn from " + callingUid + " !=" + allowedCallingUid);
+          return false;
+        }
+        return wrapped.handleTransaction(code, data);
+      }
+    };
+  }
 }

binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java

@@ -16,12 +16,17 @@
 
 package io.grpc.binder.internal;
 
+import static android.os.IBinder.FLAG_ONEWAY;
 import static android.os.Process.myUid;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
 import static io.grpc.binder.internal.BinderTransport.REMOTE_UID;
 import static io.grpc.binder.internal.BinderTransport.SETUP_TRANSPORT;
+import static io.grpc.binder.internal.BinderTransport.SHUTDOWN_TRANSPORT;
 import static io.grpc.binder.internal.BinderTransport.WIRE_FORMAT_VERSION;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.verify;
@@ -48,6 +53,7 @@
 import io.grpc.binder.SecurityPolicies;
 import io.grpc.binder.internal.SettableAsyncSecurityPolicy.AuthRequest;
 import io.grpc.internal.AbstractTransportTest;
+import io.grpc.internal.ClientTransport;
 import io.grpc.internal.ClientTransportFactory.ClientTransportOptions;
 import io.grpc.internal.ConnectionClientTransport;
 import io.grpc.internal.GrpcUtil;
@@ -64,6 +70,8 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
@@ -102,6 +110,9 @@ public final class RobolectricBinderTransportTest extends AbstractTransportTest
 
   @Mock AsyncSecurityPolicy mockClientSecurityPolicy;
 
+  @Captor
+  ArgumentCaptor<Status> statusCaptor;
+
   ApplicationInfo serverAppInfo;
   PackageInfo serverPkgInfo;
   ServiceInfo serviceInfo;
@@ -306,6 +317,42 @@ public void clientIgnoresDuplicateSetupTransaction() throws Exception {
         .isEqualTo(myUid());
   }
 
+  @Test
+  public void clientIgnoresTransactionFromNonServerUids() throws Exception {
+    server.start(serverListener);
+    client = newClientTransport(server);
+    startTransport(client, mockClientTransportListener);
+
+    int serverUid = ((ConnectionClientTransport) client).getAttributes().get(REMOTE_UID);
+    int someOtherUid = 1 + serverUid;
+    sendShutdownTransportTransactionAsUid(client, someOtherUid);
+
+    // Demonstrate that the transport is still working and that shutdown transaction was ignored.
+    ClientTransport.PingCallback mockPingCallback = mock(ClientTransport.PingCallback.class);
+    client.ping(mockPingCallback, directExecutor());
+    verify(mockPingCallback, timeout(TIMEOUT_MS)).onSuccess(anyLong());
+
+    // Try again as the expected uid to demonstrate that this wasn't ignored for some other reason.
+    sendShutdownTransportTransactionAsUid(client, serverUid);
+
+    verify(mockClientTransportListener, timeout(TIMEOUT_MS))
+        .transportShutdown(statusCaptor.capture());
+    assertThat(statusCaptor.getValue().getCode()).isEqualTo(Status.Code.UNAVAILABLE);
+    assertThat(statusCaptor.getValue().getDescription()).contains("shutdown");
+  }
+
+  static void sendShutdownTransportTransactionAsUid(ClientTransport client, int sendingUid) {
+    int originalUid = Binder.getCallingUid();
+    try {
+      ShadowBinder.setCallingUid(sendingUid);
+      ((BinderClientTransport) client)
+          .getIncomingBinderForTesting()
+          .onTransact(SHUTDOWN_TRANSPORT, null, null, FLAG_ONEWAY);
+    } finally {
+      ShadowBinder.setCallingUid(originalUid);
+    }
+  }
+
   @Test
   @Override
   // We don't quite pass the official/abstract version of this test yet because

binder/src/test/java/io/grpc/binder/internal/TransactionUtilsTest.java

@@ -0,0 +1,70 @@
+/*
+ * Copyright 2025 The gRPC Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.grpc.binder.internal;
+
+import static com.google.common.truth.Truth.assertThat;
+import static io.grpc.binder.internal.TransactionUtils.newCallerFilteringHandler;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.same;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.os.Binder;
+import android.os.Parcel;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.robolectric.RobolectricTestRunner;
+import org.robolectric.shadows.ShadowBinder;
+
+@RunWith(RobolectricTestRunner.class)
+public final class TransactionUtilsTest {
+
+  @Rule public MockitoRule mocks = MockitoJUnit.rule();
+
+  @Mock LeakSafeOneWayBinder.TransactionHandler mockHandler;
+
+  @Test
+  public void shouldIgnoreTransactionFromWrongUid() {
+    Parcel p = Parcel.obtain();
+    int originalUid = Binder.getCallingUid();
+    try {
+      when(mockHandler.handleTransaction(eq(1234), same(p))).thenReturn(true);
+      LeakSafeOneWayBinder.TransactionHandler uid100OnlyHandler =
+          newCallerFilteringHandler(1000, mockHandler);
+
+      ShadowBinder.setCallingUid(9999);
+      boolean result = uid100OnlyHandler.handleTransaction(1234, p);
+      assertThat(result).isFalse();
+      verify(mockHandler, never()).handleTransaction(anyInt(), any());
+
+      ShadowBinder.setCallingUid(1000);
+      result = uid100OnlyHandler.handleTransaction(1234, p);
+      assertThat(result).isTrue();
+      verify(mockHandler).handleTransaction(1234, p);
+    } finally {
+      ShadowBinder.setCallingUid(originalUid);
+      p.recycle();
+    }
+  }
+}