Merge remote-tracking branch 'upstream/master' into rlseagerinit

Author: kannanjgithub

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/AndroidComponentAddress.java

@@ -58,7 +58,7 @@ public final class AndroidComponentAddress extends SocketAddress {
   @Nullable
   private final UserHandle targetUser; // null means the same user that hosts this process.
 
-  protected AndroidComponentAddress(Intent bindIntent, @Nullable UserHandle targetUser) {
+  private AndroidComponentAddress(Intent bindIntent, @Nullable UserHandle targetUser) {
     checkArgument(
         bindIntent.getComponent() != null || bindIntent.getPackage() != null,
         "'bindIntent' must be explicit. Specify either a package or ComponentName.");
@@ -250,7 +250,22 @@ public Builder setBindIntentFromComponent(ComponentName component) {
       return this;
     }
 
-    /** See {@link AndroidComponentAddress#getTargetUser()}. */
+    /**
+     * Specifies the Android user in which the built Address' bind Intent will be evaluated.
+     *
+     * <p>Connecting to a server in a different Android user is uncommon and requires the client app
+     * have runtime visibility of &#064;SystemApi's and hold certain &#064;SystemApi permissions.
+     * The device must also be running Android SDK version 30 or higher.
+     *
+     * <p>See https://developer.android.com/guide/app-compatibility/restrictions-non-sdk-interfaces
+     * for details on which apps can call the underlying &#064;SystemApi's needed to make this type
+     * of connection.
+     *
+     * <p>One of the "android.permission.INTERACT_ACROSS_XXX" permissions is required. The exact one
+     * depends on the calling user's relationship to the target user, whether client and server are
+     * in the same or different apps, and the version of Android in use. See {@link
+     * Context#bindServiceAsUser}, the essential underlying Android API, for details.
+     */
     @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10173")
     public Builder setTargetUser(@Nullable UserHandle targetUser) {
       this.targetUser = targetUser;

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;
 
@@ -35,12 +37,27 @@ private ApiConstants() {}
   /**
    * Specifies the Android user in which target URIs should be resolved.
    *
-   * <p>{@link UserHandle} can't reasonably be encoded in a target URI string. Instead, all
-   * {@link io.grpc.NameResolverProvider}s producing {@link AndroidComponentAddress}es should let
-   * clients address servers in another Android user using this argument.
+   * <p>{@link UserHandle} can't reasonably be encoded in a target URI string. Instead, all {@link
+   * io.grpc.NameResolverProvider}s producing {@link AndroidComponentAddress}es should let clients
+   * address servers in another Android user using this argument.
    *
-   * <p>See also {@link AndroidComponentAddress#getTargetUser()}.
+   * <p>Connecting to a server in a different Android user is uncommon and can only be done by a
+   * "system app" client with special permissions. See {@link
+   * AndroidComponentAddress.Builder#setTargetUser(UserHandle)} for details.
    */
+  @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

@@ -242,9 +242,9 @@ public BinderChannelBuilder securityPolicy(SecurityPolicy securityPolicy) {
    * specify a {@link UserHandle}. If neither the Channel nor the {@link AndroidComponentAddress}
    * specifies a target user, the {@link UserHandle} of the current process will be used.
    *
-   * <p>Targeting a Service in a different Android user is uncommon and requires special permissions
-   * normally reserved for system apps. See {@link android.content.Context#bindServiceAsUser} for
-   * details.
+   * <p>Connecting to a server in a different Android user is uncommon and can only be done by a
+   * "system app" client with special permissions. See {@link
+   * AndroidComponentAddress.Builder#setTargetUser(UserHandle)} for details.
    *
    * @deprecated This method's name is misleading because it implies an impersonated client identity
    *     when it's actually specifying part of the server's location. It's also no longer necessary
@@ -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;
+    }
   }
 }