binder: Introduce server pre-authorization

binder/build.gradle

@@ -72,6 +72,7 @@ dependencies {
     androidTestImplementation testFixtures(project(':grpc-core'))
 
     testFixturesImplementation libraries.guava.testlib
+    testFixturesImplementation testFixtures(project(':grpc-core'))
 }
 
 import net.ltgt.gradle.errorprone.CheckSeverity

binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java

@@ -58,10 +58,8 @@
 import java.io.InputStream;
 import java.util.ArrayDeque;
 import java.util.Deque;
-import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
-import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
@@ -102,7 +100,7 @@ public final class BinderClientTransportTest {
 
   AndroidComponentAddress serverAddress;
   BinderTransport.BinderClientTransport transport;
-  BlockingSecurityPolicy blockingSecurityPolicy = new BlockingSecurityPolicy();
+  SettableAsyncSecurityPolicy blockingSecurityPolicy = new SettableAsyncSecurityPolicy();
 
   private final ObjectPool<ScheduledExecutorService> executorServicePool =
       new FixedObjectPool<>(Executors.newScheduledThreadPool(1));
@@ -170,6 +168,11 @@ public BinderClientTransportBuilder setReadyTimeoutMillis(int timeoutMillis) {
       return this;
     }
 
+    public BinderClientTransportBuilder setPreAuthorizeServer(boolean preAuthorizeServer) {
+      factoryBuilder.setPreAuthorizeServers(preAuthorizeServer);
+      return this;
+    }
+
     public BinderTransport.BinderClientTransport build() {
       return factoryBuilder
           .buildClientTransportFactory()
@@ -179,7 +182,7 @@ public BinderTransport.BinderClientTransport build() {
 
   @After
   public void tearDown() throws Exception {
-    blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.ABORTED);
+    blockingSecurityPolicy.setAuthorizationResult(Status.ABORTED);
     transport.shutdownNow(Status.OK);
     HostServices.awaitServiceShutdown();
     shutdownAndTerminate(executorServicePool.getObject());
@@ -285,16 +288,16 @@ public void testMessageProducerClosedAfterStream_b169313545() throws Exception {
   @Test
   public void testNewStreamBeforeTransportReadyFails() throws Exception {
     // Use a special SecurityPolicy that lets us act before the transport is setup/ready.
-    transport =
-        new BinderClientTransportBuilder().setSecurityPolicy(blockingSecurityPolicy).build();
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
     transport.start(transportListener).run();
     ClientStream stream =
         transport.newStream(streamingMethodDesc, new Metadata(), CallOptions.DEFAULT, tracers);
     stream.start(streamListener);
     assertThat(streamListener.awaitClose().getCode()).isEqualTo(Code.INTERNAL);
 
     // Unblock the SETUP_TRANSPORT handshake and make sure it becomes ready in the usual way.
-    blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.OK);
+    securityPolicy.setAuthorizationResult(Status.OK);
     transportListener.awaitReady();
   }
 
@@ -370,24 +373,60 @@ public void testBlackHoleEndpointConnectTimeout() throws Exception {
   }
 
   @Test
-  public void testBlackHoleSecurityPolicyConnectTimeout() throws Exception {
+  public void testBlackHoleSecurityPolicyAuthTimeout() throws Exception {
+    transport =
+        new BinderClientTransportBuilder()
+            .setSecurityPolicy(blockingSecurityPolicy)
+            .setPreAuthorizeServer(false)
+            .setReadyTimeoutMillis(1_234)
+            .build();
+    transport.start(transportListener).run();
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
+    assertThat(transportStatus.getDescription()).contains("1234");
+    transportListener.awaitTermination();
+  }
+
+  @Test
+  public void testBlackHoleSecurityPolicyPreAuthTimeout() throws Exception {
     transport =
         new BinderClientTransportBuilder()
             .setSecurityPolicy(blockingSecurityPolicy)
+            .setPreAuthorizeServer(true)
             .setReadyTimeoutMillis(1_234)
             .build();
     transport.start(transportListener).run();
     Status transportStatus = transportListener.awaitShutdown();
     assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
     assertThat(transportStatus.getDescription()).contains("1234");
     transportListener.awaitTermination();
-    blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.OK);
   }
 
   @Test
-  public void testAsyncSecurityPolicyFailure() throws Exception {
+  public void testAsyncSecurityPolicyAuthFailure() throws Exception {
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
-    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(false)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    RuntimeException exception = new NullPointerException();
+    securityPolicy.setAuthorizationException(exception);
+    transport.start(transportListener).run();
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.INTERNAL);
+    assertThat(transportStatus.getCause()).isEqualTo(exception);
+    transportListener.awaitTermination();
+  }
+
+  @Test
+  public void testAsyncSecurityPolicyPreAuthFailure() throws Exception {
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(true)
+            .setSecurityPolicy(securityPolicy)
+            .build();
     RuntimeException exception = new NullPointerException();
     securityPolicy.setAuthorizationException(exception);
     transport.start(transportListener).run();
@@ -400,11 +439,32 @@ public void testAsyncSecurityPolicyFailure() throws Exception {
   @Test
   public void testAsyncSecurityPolicySuccess() throws Exception {
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
-    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
-    securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED);
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(false)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED.withDescription("xyzzy"));
+    transport.start(transportListener).run();
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED);
+    assertThat(transportStatus.getDescription()).contains("xyzzy");
+    transportListener.awaitTermination();
+  }
+
+  @Test
+  public void testAsyncSecurityPolicyPreAuthSuccess() throws Exception {
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(true)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED.withDescription("xyzzy"));
     transport.start(transportListener).run();
     Status transportStatus = transportListener.awaitShutdown();
     assertThat(transportStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED);
+    assertThat(transportStatus.getDescription()).contains("xyzzy");
     transportListener.awaitTermination();
   }
 
@@ -548,26 +608,6 @@ public synchronized void closed(Status status, RpcProgress rpcProgress, Metadata
     }
   }
 
-  /**
-   * A SecurityPolicy that blocks the transport authorization check until a test sets the outcome.
-   */
-  static class BlockingSecurityPolicy extends SecurityPolicy {
-    private final BlockingQueue<Status> results = new LinkedBlockingQueue<>();
-
-    public void provideNextCheckAuthorizationResult(Status status) {
-      results.add(status);
-    }
-
-    @Override
-    public Status checkAuthorization(int uid) {
-      try {
-        return results.take();
-      } catch (InterruptedException e) {
-        return Status.fromThrowable(e);
-      }
-    }
-  }
-
   /** An AsyncSecurityPolicy that lets a test specify the outcome of checkAuthorizationAsync(). */
   static class SettableAsyncSecurityPolicy extends AsyncSecurityPolicy {
     private SettableFuture<Status> result = SettableFuture.create();

binder/src/main/java/io/grpc/binder/ApiConstants.java

@@ -18,6 +18,8 @@
 
 import android.content.Intent;
 import android.os.UserHandle;
+import io.grpc.Attributes;
+import io.grpc.EquivalentAddressGroup;
 import io.grpc.ExperimentalApi;
 import io.grpc.NameResolver;
 
@@ -43,4 +45,15 @@ private ApiConstants() {}
    */
   public static final NameResolver.Args.Key<UserHandle> TARGET_ANDROID_USER =
       NameResolver.Args.Key.create("target-android-user");
+
+  /**
+   * Lets you override a Channel's pre-auth configuration (see {@link
+   * BinderChannelBuilder#preAuthorizeServers(boolean)} for a given {@link EquivalentAddressGroup}.
+   *
+   * <p>A {@link NameResolver} that discovers servers from an untrusted source like PackageManager
+   * can use this to force server pre-auth and prevent abuse.
+   */
+  @EquivalentAddressGroup.Attr
+  public static final Attributes.Key<Boolean> PRE_AUTH_SERVER_OVERRIDE =
+      Attributes.Key.create("pre-auth-server-override");
 }

binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java

@@ -279,6 +279,34 @@ public BinderChannelBuilder strictLifecycleManagement() {
     return this;
   }
 
+  /**
+   * Checks servers against this Channel's {@link SecurityPolicy} *before* binding.
+   *
+   * <p>Android users can be tricked into installing a malicious app with the same package name as a
+   * legitimate server. That's why we don't send calls to a server until it has been authorized by
+   * an appropriate {@link SecurityPolicy}. But merely binding to a malicious server can enable
+   * "keep-alive" and "background activity launch" abuse, even if security policy ultimately causes
+   * the grpc connection to fail. Pre-authorization is especially important for security when the
+   * server's address isn't known in advance but rather resolved via target URI or discovered by
+   * other means.
+   *
+   * <p>Note that, unlike ordinary authorization, pre-authorization is performed against the server
+   * app's UID, not the UID of the process hosting the bound Service. These can be different, most
+   * commonly due to services that set `android:isolatedProcess=true`.
+   *
+   * <p>Pre-authorization is strongly recommended but it remains optional for now because of this
+   * behavior change and the small performance cost.
+   *
+   * <p>The default value of this property is false but it will become true in a future release.
+   * Clients that require a particular behavior should configure it explicitly using this method
+   * rather than relying on the default.
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/00000")
+  public BinderChannelBuilder preAuthorizeServers(boolean preAuthorize) {
+    transportFactoryBuilder.setPreAuthorizeServers(preAuthorize);
+    return this;
+  }
+
   @Override
   public BinderChannelBuilder idleTimeout(long value, TimeUnit unit) {
     checkState(

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

@@ -16,10 +16,12 @@
 
 package io.grpc.binder.internal;
 
+import android.content.pm.ServiceInfo;
 import android.os.IBinder;
 import androidx.annotation.AnyThread;
 import androidx.annotation.MainThread;
 import io.grpc.Status;
+import io.grpc.StatusException;
 
 /** An interface for managing a {@code Binder} connection. */
 interface Bindable {
@@ -45,6 +47,19 @@ interface Observer {
     void onUnbound(Status reason);
   }
 
+  /**
+   * Fetches details about the remote Service from PackageManager without binding to it.
+   *
+   * <p>Resolving an untrusted address before binding to it lets you screen out problematic servers
+   * before giving them a chance to run. However, note that the identity/existence of the resolved
+   * Service can change between the time this method returns and the time you actually bind/connect
+   * to it. For example, suppose the target package gets uninstalled right after this method
+   * returns. In {@link Observer#onBound}, you should verify that the server you resolved is the
+   * same one you connected to.
+   */
+  @AnyThread
+  ServiceInfo resolve() throws StatusException;
+
   /**
    * Attempt to bind with the remote service.
    *

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

@@ -55,6 +55,7 @@ public final class BinderClientTransportFactory implements ClientTransportFactor
   final InboundParcelablePolicy inboundParcelablePolicy;
   final OneWayBinderProxy.Decorator binderDecorator;
   final long readyTimeoutMillis;
+  final boolean preAuthorizeServers; // TODO(jdcormie): Default to true.
 
   ScheduledExecutorService executorService;
   Executor offloadExecutor;
@@ -75,6 +76,7 @@ private BinderClientTransportFactory(Builder builder) {
     inboundParcelablePolicy = checkNotNull(builder.inboundParcelablePolicy);
     binderDecorator = checkNotNull(builder.binderDecorator);
     readyTimeoutMillis = builder.readyTimeoutMillis;
+    preAuthorizeServers = builder.preAuthorizeServers;
 
     executorService = scheduledExecutorPool.getObject();
     offloadExecutor = offloadExecutorPool.getObject();
@@ -128,6 +130,7 @@ public static final class Builder implements ClientTransportFactoryBuilder {
     InboundParcelablePolicy inboundParcelablePolicy = InboundParcelablePolicy.DEFAULT;
     OneWayBinderProxy.Decorator binderDecorator = OneWayBinderProxy.IDENTITY_DECORATOR;
     long readyTimeoutMillis = 60_000;
+    boolean preAuthorizeServers;
 
     @Override
     public BinderClientTransportFactory buildClientTransportFactory() {
@@ -216,5 +219,11 @@ public Builder setReadyTimeoutMillis(long readyTimeoutMillis) {
       this.readyTimeoutMillis = readyTimeoutMillis;
       return this;
     }
+
+    /** Whether to check server addresses against the SecurityPolicy *before* binding to them. */
+    public Builder setPreAuthorizeServers(boolean preAuthorizeServers) {
+      this.preAuthorizeServers = preAuthorizeServers;
+      return this;
+    }
   }
 }