binder: Optionally pre-authorize servers using their PackageManager identity before binding.

Author: jdcormie

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

@@ -17,7 +17,9 @@
 package io.grpc.binder;
 
 import android.content.Intent;
+import android.content.pm.ServiceInfo;
 import android.os.UserHandle;
+import io.grpc.Attributes;
 import io.grpc.ExperimentalApi;
 import io.grpc.NameResolver;
 
@@ -43,4 +45,22 @@ private ApiConstants() {}
    */
   public static final NameResolver.Args.Key<UserHandle> TARGET_ANDROID_USER =
       NameResolver.Args.Key.create("target-android-user");
+
+  /**
+   * Marks an {@link io.grpc.EquivalentAddressGroup} as needing pre-authorization.
+   *
+   * <p>Clients should authorize servers before connecting to them, but older versions of the binder
+   * transport didn't do so. While this important extra security check is now possible (see {@link
+   * BinderChannelBuilder#preAuthorizeServers(boolean)}, it remains optional, because it's a slight
+   * behavior change and has a small performance cost and we don't want to break existing apps.
+   */
+  public static final Attributes.Key<Void> PRE_AUTH_REQUIRED =
+      Attributes.Key.create("pre-auth-required");
+
+  /**
+   * The authentic ServiceInfo for an {@link io.grpc.EquivalentAddressGroup} of {@link
+   * AndroidComponentAddress}es, in case a {@link NameResolver} has already looked it up.
+   */
+  public static final Attributes.Key<ServiceInfo> TARGET_SERVICE_INFO =
+      Attributes.Key.create("target-service-info");
 }

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

@@ -158,7 +158,7 @@ public static BinderChannelBuilder forTarget(String target) {
   }
 
   private final ManagedChannelImplBuilder managedChannelImplBuilder;
-  private final BinderClientTransportFactory.Builder transportFactoryBuilder;
+  final BinderClientTransportFactory.Builder transportFactoryBuilder;
 
   private boolean strictLifecycleManagement;
 
@@ -179,6 +179,8 @@ private BinderChannelBuilder(
     } else {
       managedChannelImplBuilder =
           new ManagedChannelImplBuilder(target, transportFactoryBuilder, null);
+
+      preAuthorizeServers(true); // Pre-authorize resolved addresses by default.
     }
     idleTimeout(60, TimeUnit.SECONDS);
   }
@@ -279,6 +281,29 @@ public BinderChannelBuilder strictLifecycleManagement() {
     return this;
   }
 
+  /**
+   * Checks server addresses against this channel's {@link SecurityPolicy} *before* connecting.
+   *
+   * <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 connecting to a malicious server can enable
+   * "keep-alive" and "background activity launch" attacks, even if that server is ultimately
+   * disallowed by the channel's security policy. Pre-authorization is even more 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>Pre-authorization is strongly recommended but it remains optional for now because of the
+   * small performance cost and because it precludes servers that proxy their "endpoint binder"
+   * through another (unauthorized) app (not recommended, but technically a behavior change).
+   *
+   * <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.
+   */
+  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,6 +16,7 @@
 
 package io.grpc.binder.internal;
 
+import android.content.pm.ServiceInfo;
 import android.os.IBinder;
 import androidx.annotation.AnyThread;
 import androidx.annotation.MainThread;
@@ -45,6 +46,10 @@ interface Observer {
     void onUnbound(Status reason);
   }
 
+  /** Fetches details about the remote service from PackageManager *before* binding to it. */
+  @AnyThread
+  ServiceInfo resolve();
+
   /**
    * 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;
 
   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,16 @@ 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;
+    }
+
+    /** Whether to check server addresses against the SecurityPolicy before binding to them. */
+    public boolean getPreAuthorizeServers() {
+      return this.preAuthorizeServers;
+    }
   }
 }