binder: Introduce server pre-authorization (#12127)

grpc-binder clients authorize servers by checking the UID of the sender of the SETUP_TRANSPORT Binder transaction against some SecurityPolicy. But merely binding to an unauthorized server to learn its UID can enable "keep-alive" and "background activity launch" abuse, even if security policy ultimately decides the connection is unauthorized. Pre-authorization mitigates this kind of abuse by looking up and authorizing a candidate server Application's UID before binding to it. Pre-auth is especially important when the server's address is not fixed in advance but discovered by PackageManager lookup.

Author: jdcormie

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

@@ -172,6 +172,12 @@ public BinderClientTransportBuilder setReadyTimeoutMillis(int timeoutMillis) {
       return this;
     }
 
+    @CanIgnoreReturnValue
+    public BinderClientTransportBuilder setPreAuthorizeServer(boolean preAuthorizeServer) {
+      factoryBuilder.setPreAuthorizeServers(preAuthorizeServer);
+      return this;
+    }
+
     public BinderTransport.BinderClientTransport build() {
       return factoryBuilder
           .buildClientTransportFactory()
@@ -372,11 +378,12 @@ public void testBlackHoleEndpointConnectTimeout() throws Exception {
   }
 
   @Test
-  public void testBlackHoleSecurityPolicyConnectTimeout() throws Exception {
+  public void testBlackHoleSecurityPolicyAuthTimeout() throws Exception {
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
     transport =
         new BinderClientTransportBuilder()
             .setSecurityPolicy(securityPolicy)
+            .setPreAuthorizeServer(false)
             .setReadyTimeoutMillis(1_234)
             .build();
     transport.start(transportListener).run();
@@ -387,15 +394,39 @@ public void testBlackHoleSecurityPolicyConnectTimeout() throws Exception {
     assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
     assertThat(transportStatus.getDescription()).contains("1234");
     transportListener.awaitTermination();
-
     // If the transport gave up waiting on auth, it should cancel its request.
     assertThat(authRequest.isCancelled()).isTrue();
   }
 
   @Test
-  public void testAsyncSecurityPolicyFailure() throws Exception {
+  public void testBlackHoleSecurityPolicyPreAuthTimeout() throws Exception {
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
-    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
+    transport =
+        new BinderClientTransportBuilder()
+            .setSecurityPolicy(securityPolicy)
+            .setPreAuthorizeServer(true)
+            .setReadyTimeoutMillis(1_234)
+            .build();
+    transport.start(transportListener).run();
+    // Take the next authRequest but don't respond to it, in order to trigger the ready timeout.
+    AuthRequest preAuthRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS);
+
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
+    assertThat(transportStatus.getDescription()).contains("1234");
+    transportListener.awaitTermination();
+    // If the transport gave up waiting on auth, it should cancel its request.
+    assertThat(preAuthRequest.isCancelled()).isTrue();
+  }
+
+  @Test
+  public void testAsyncSecurityPolicyAuthFailure() throws Exception {
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(false)
+            .setSecurityPolicy(securityPolicy)
+            .build();
     RuntimeException exception = new NullPointerException();
     transport.start(transportListener).run();
     securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS).setResult(exception);
@@ -406,15 +437,55 @@ public void testAsyncSecurityPolicyFailure() throws Exception {
   }
 
   @Test
-  public void testAsyncSecurityPolicySuccess() throws Exception {
+  public void testAsyncSecurityPolicyPreAuthFailure() throws Exception {
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
-    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(true)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    RuntimeException exception = new NullPointerException();
+    transport.start(transportListener).run();
+    securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS).setResult(exception);
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.INTERNAL);
+    assertThat(transportStatus.getCause()).isEqualTo(exception);
+    transportListener.awaitTermination();
+  }
+
+  @Test
+  public void testAsyncSecurityPolicyAuthSuccess() throws Exception {
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(false)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    transport.start(transportListener).run();
+    securityPolicy
+        .takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS)
+        .setResult(Status.PERMISSION_DENIED.withDescription("xyzzy"));
+    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();
     transport.start(transportListener).run();
     securityPolicy
         .takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS)
-        .setResult(Status.PERMISSION_DENIED);
+        .setResult(Status.PERMISSION_DENIED.withDescription("xyzzy"));
     Status transportStatus = transportListener.awaitShutdown();
     assertThat(transportStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED);
+    assertThat(transportStatus.getDescription()).contains("xyzzy");
     transportListener.awaitTermination();
   }
 

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;
 
@@ -46,4 +48,16 @@ private ApiConstants() {}
   @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10173")
   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
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/12191")
+  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,35 @@ 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 it's ultimately unauthorized.
+   * Pre-authorization mitigates these threats by performing a preliminary {@link SecurityPolicy}
+   * check against a server app's PackageManager-registered identity without actually creating an
+   * instance of it. This is especially important for security when the server's direct 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/12191")
+  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 or upgraded 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;
+    }
   }
 }