diff --git a/binder/build.gradle b/binder/build.gradle index 3390e02fce7..7ac23750a2a 100644 --- a/binder/build.gradle +++ b/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 diff --git a/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java b/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java index 33c127f97a7..31891c63fc6 100644 --- a/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java +++ b/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 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,8 +288,8 @@ 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); @@ -294,7 +297,7 @@ public void testNewStreamBeforeTransportReadyFails() throws Exception { 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,10 +373,26 @@ 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(); @@ -381,13 +400,33 @@ public void testBlackHoleSecurityPolicyConnectTimeout() throws Exception { 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 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 result = SettableFuture.create(); diff --git a/binder/src/main/java/io/grpc/binder/ApiConstants.java b/binder/src/main/java/io/grpc/binder/ApiConstants.java index 292c580c2b8..48aadbfdfd6 100644 --- a/binder/src/main/java/io/grpc/binder/ApiConstants.java +++ b/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 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}. + * + *

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 PRE_AUTH_SERVER_OVERRIDE = + Attributes.Key.create("pre-auth-server-override"); } diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index 0b8f0bb4b3f..f8559ffa117 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/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. + * + *

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. + * + *

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`. + * + *

Pre-authorization is strongly recommended but it remains optional for now because of this + * behavior change and the small performance cost. + * + *

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( diff --git a/binder/src/main/java/io/grpc/binder/internal/Bindable.java b/binder/src/main/java/io/grpc/binder/internal/Bindable.java index 8e1af64b63d..fdbeefddbd2 100644 --- a/binder/src/main/java/io/grpc/binder/internal/Bindable.java +++ b/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. + * + *

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. * diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java index 3852b21d5c3..ef00f70e35d 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java +++ b/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; + } } } diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index f61c455edd5..6aa11d27da4 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -19,9 +19,11 @@ 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.ApiConstants.PRE_AUTH_SERVER_OVERRIDE; import static java.util.concurrent.TimeUnit.MILLISECONDS; import android.content.Context; +import android.content.pm.ServiceInfo; import android.os.Binder; import android.os.DeadObjectException; import android.os.IBinder; @@ -569,6 +571,7 @@ public static final class BinderClientTransport extends BinderTransport private final long readyTimeoutMillis; private final PingTracker pingTracker; + private final boolean preAuthorizeServer; @Nullable private ManagedClientTransport.Listener clientTransportListener; @@ -578,6 +581,9 @@ public static final class BinderClientTransport extends BinderTransport @GuardedBy("this") private ScheduledFuture readyTimeoutFuture; // != null iff timeout scheduled. + @GuardedBy("this") + private ListenableFuture preAuthResultFuture; + /** * Constructs a new transport instance. * @@ -602,6 +608,9 @@ public BinderClientTransport( this.securityPolicy = factory.securityPolicy; this.offloadExecutor = offloadExecutorPool.getObject(); this.readyTimeoutMillis = factory.readyTimeoutMillis; + Boolean preAuthServerOverride = options.getEagAttributes().get(PRE_AUTH_SERVER_OVERRIDE); + this.preAuthorizeServer = + preAuthServerOverride != null ? preAuthServerOverride : factory.preAuthorizeServers; numInUseStreams = new AtomicInteger(); pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id)); @@ -643,7 +652,16 @@ public synchronized Runnable start(ManagedClientTransport.Listener clientTranspo synchronized (BinderClientTransport.this) { if (inState(TransportState.NOT_STARTED)) { setState(TransportState.SETUP); - serviceBinding.bind(); + try { + if (preAuthorizeServer) { + preAuthorize(serviceBinding.resolve()); + } else { + serviceBinding.bind(); + } + } catch (StatusException e) { + shutdownInternal(e.getStatus(), true); + return; + } if (readyTimeoutMillis >= 0) { readyTimeoutFuture = getScheduledExecutorService() @@ -657,6 +675,43 @@ public synchronized Runnable start(ManagedClientTransport.Listener clientTranspo }; } + @GuardedBy("this") + private void preAuthorize(ServiceInfo serviceInfo) { + // It's unlikely, but the identity/existence of this Service could change by the time we + // actually connect. It doesn't matter though, because: + // - If pre-auth fails (but would succeed against the server's new state), the grpc-core layer + // will eventually retry using a new transport instance that will see the Service's new state. + // - If pre-auth succeeds (but would fail against the server's new state), we might give an + // unauthorized server a chance to run, but the connection will still fail by SecurityPolicy + // check later in handshake. Pre-auth remains effective at mitigating abuse because malware + // can't typically control the exact timing of its installation. + preAuthResultFuture = checkServerAuthorizationAsync(serviceInfo.applicationInfo.uid); + Futures.addCallback( + preAuthResultFuture, + new FutureCallback() { + @Override + public void onSuccess(Status result) { + handlePreAuthResult(result); + } + + @Override + public void onFailure(Throwable t) { + handleAuthResult(t); + } + }, + offloadExecutor); + } + + private synchronized void handlePreAuthResult(Status authorization) { + if (inState(TransportState.SETUP)) { + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + } else { + serviceBinding.bind(); + } + } + } + private synchronized void onReadyTimeout() { if (inState(TransportState.SETUP)) { readyTimeoutFuture = null; @@ -751,6 +806,10 @@ void notifyTerminated() { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } + if (preAuthResultFuture != null) { + preAuthResultFuture.cancel(false); + preAuthResultFuture = null; + } serviceBinding.unbind(); clientTransportListener.transportTerminated(); } @@ -770,13 +829,8 @@ protected void handleSetupTransport(Parcel parcel) { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { - ListenableFuture authFuture = - (securityPolicy instanceof AsyncSecurityPolicy) - ? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid) - : Futures.submit( - () -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); Futures.addCallback( - authFuture, + checkServerAuthorizationAsync(remoteUid), new FutureCallback() { @Override public void onSuccess(Status result) { @@ -793,6 +847,12 @@ public void onFailure(Throwable t) { } } + private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { + return (securityPolicy instanceof AsyncSecurityPolicy) + ? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid) + : Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); + } + private synchronized void handleAuthResult(IBinder binder, Status authorization) { if (inState(TransportState.SETUP)) { if (!authorization.isOk()) { diff --git a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java index ee171140045..96f7d213395 100644 --- a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java +++ b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java @@ -23,13 +23,18 @@ import android.content.Context; import android.content.Intent; import android.content.ServiceConnection; +import android.content.pm.PackageManager; +import android.content.pm.ResolveInfo; +import android.content.pm.ServiceInfo; import android.os.IBinder; import android.os.UserHandle; import androidx.annotation.AnyThread; import androidx.annotation.MainThread; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.VerifyException; import com.google.errorprone.annotations.concurrent.GuardedBy; import io.grpc.Status; +import io.grpc.StatusException; import io.grpc.binder.BinderChannelCredentials; import java.util.concurrent.Executor; import java.util.logging.Level; @@ -247,6 +252,42 @@ void unbindInternal(Status reason) { } } + // Sadly we must call this system API reflectively since it isn't part of the Android SDK. + private static int getIdentifier(UserHandle userHandle) throws ReflectiveOperationException { + return (int) userHandle.getClass().getMethod("getIdentifier").invoke(userHandle); + } + + // Sadly we must call this system API reflectively since it isn't part of the Android SDK. + private static ResolveInfo resolveServiceAsUser( + PackageManager packageManager, Intent intent, int flags, UserHandle targetUserHandle) { + try { + return (ResolveInfo) + packageManager + .getClass() + .getMethod("resolveServiceAsUser", Intent.class, int.class, int.class) + .invoke(packageManager, intent, flags, getIdentifier(targetUserHandle)); + } catch (ReflectiveOperationException e) { + throw new VerifyException(e); + } + } + + @AnyThread + @Override + public ServiceInfo resolve() throws StatusException { + checkState(sourceContext != null); + PackageManager packageManager = sourceContext.getPackageManager(); + ResolveInfo resolveInfo = + targetUserHandle != null + ? resolveServiceAsUser(packageManager, bindIntent, 0, targetUserHandle) + : packageManager.resolveService(bindIntent, 0); + if (resolveInfo == null) { + throw Status.UNIMPLEMENTED // Same status code as when bindService() returns false. + .withDescription("resolveService(" + bindIntent + " / " + targetUserHandle + ") was null") + .asException(); + } + return resolveInfo.serviceInfo; + } + @MainThread private void clearReferences() { sourceContext = null; diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index 16f06ad81c9..ffd1d89e69c 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -22,7 +22,13 @@ import static org.robolectric.Shadows.shadowOf; import android.app.Application; +import android.content.pm.ApplicationInfo; +import android.content.pm.PackageInfo; +import android.content.pm.ServiceInfo; import androidx.test.core.app.ApplicationProvider; +import androidx.test.core.content.pm.ApplicationInfoBuilder; +import androidx.test.core.content.pm.PackageInfoBuilder; +import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; @@ -46,11 +52,13 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.robolectric.RobolectricTestRunner; +import org.robolectric.ParameterizedRobolectricTestRunner; +import org.robolectric.ParameterizedRobolectricTestRunner.Parameter; +import org.robolectric.ParameterizedRobolectricTestRunner.Parameters; import org.robolectric.annotation.LooperMode; import org.robolectric.annotation.LooperMode.Mode; -@RunWith(RobolectricTestRunner.class) +@RunWith(ParameterizedRobolectricTestRunner.class) @LooperMode(Mode.INSTRUMENTATION_TEST) public final class RobolectricBinderSecurityTest { @@ -62,10 +70,33 @@ public final class RobolectricBinderSecurityTest { private ManagedChannel channel; private Server server; + @Parameter public boolean preAuthServersParam; + + @Parameters(name = "preAuthServersParam={0}") + public static ImmutableList data() { + return ImmutableList.of(true, false); + } + @Before public void setUp() { + ApplicationInfo serverAppInfo = + ApplicationInfoBuilder.newBuilder().setPackageName(context.getPackageName()).build(); + serverAppInfo.uid = android.os.Process.myUid(); + PackageInfo serverPkgInfo = + PackageInfoBuilder.newBuilder() + .setPackageName(serverAppInfo.packageName) + .setApplicationInfo(serverAppInfo) + .build(); + shadowOf(context.getPackageManager()).installPackage(serverPkgInfo); + + ServiceInfo serviceInfo = new ServiceInfo(); + serviceInfo.name = "SomeService"; + serviceInfo.packageName = serverAppInfo.packageName; + serviceInfo.applicationInfo = serverAppInfo; + shadowOf(context.getPackageManager()).addOrUpdateService(serviceInfo); + AndroidComponentAddress listenAddress = - AndroidComponentAddress.forRemoteComponent(context.getPackageName(), "HostService"); + AndroidComponentAddress.forRemoteComponent(serviceInfo.packageName, serviceInfo.name); MethodDescriptor methodDesc = getMethodDescriptor(); ServerCallHandler callHandler = @@ -110,6 +141,7 @@ public ListenableFuture checkAuthorizationAsync(int uid) { checkNotNull(binderReceiver.get())); channel = BinderChannelBuilder.forAddress(listenAddress, context) + .preAuthorizeServers(preAuthServersParam) .build(); } diff --git a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java index 7d336067842..77a747addb5 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -16,13 +16,29 @@ package io.grpc.binder.internal; +import static com.google.common.truth.Truth.assertThat; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.verify; import static org.robolectric.Shadows.shadowOf; import android.app.Application; import android.content.Intent; +import android.content.pm.ApplicationInfo; +import android.content.pm.PackageInfo; +import android.content.pm.ServiceInfo; import androidx.test.core.app.ApplicationProvider; +import androidx.test.core.content.pm.ApplicationInfoBuilder; +import androidx.test.core.content.pm.PackageInfoBuilder; +import com.google.common.collect.ImmutableList; +import io.grpc.Attributes; import io.grpc.ServerStreamTracer; +import io.grpc.Status; import io.grpc.binder.AndroidComponentAddress; +import io.grpc.binder.ApiConstants; +import io.grpc.binder.AsyncSecurityPolicy; +import io.grpc.binder.internal.SettableAsyncSecurityPolicy.AuthRequest; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory.ClientTransportOptions; import io.grpc.internal.GrpcUtil; @@ -30,15 +46,24 @@ import io.grpc.internal.ManagedClientTransport; import io.grpc.internal.ObjectPool; import io.grpc.internal.SharedResourcePool; +import java.util.Collections; import java.util.List; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; +import org.junit.Before; import org.junit.Ignore; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; -import org.robolectric.RobolectricTestRunner; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; +import org.robolectric.ParameterizedRobolectricTestRunner; +import org.robolectric.ParameterizedRobolectricTestRunner.Parameter; +import org.robolectric.ParameterizedRobolectricTestRunner.Parameters; import org.robolectric.annotation.LooperMode; import org.robolectric.annotation.LooperMode.Mode; +import org.robolectric.shadows.ShadowBinder; /** * All of the AbstractTransportTest cases applied to {@link BinderTransport} running in a @@ -52,7 +77,7 @@ * meaning test cases don't run on the main thread. This supports the AbstractTransportTest approach * where the test thread frequently blocks waiting for transport state changes to take effect. */ -@RunWith(RobolectricTestRunner.class) +@RunWith(ParameterizedRobolectricTestRunner.class) @LooperMode(Mode.INSTRUMENTATION_TEST) public final class RobolectricBinderTransportTest extends AbstractTransportTest { @@ -64,14 +89,57 @@ public final class RobolectricBinderTransportTest extends AbstractTransportTest private final ObjectPool serverExecutorPool = SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR); + @Rule public MockitoRule mocks = MockitoJUnit.rule(); + + @Mock AsyncSecurityPolicy mockClientSecurityPolicy; + + ApplicationInfo serverAppInfo; + PackageInfo serverPkgInfo; + ServiceInfo serviceInfo; + private int nextServerAddress; + @Parameter public boolean preAuthServersParam; + + @Parameters(name = "preAuthServersParam={0}") + public static ImmutableList data() { + return ImmutableList.of(true, false); + } + + @Override + public void setUp() { + serverAppInfo = + ApplicationInfoBuilder.newBuilder().setPackageName("the.server.package").build(); + serverAppInfo.uid = android.os.Process.myUid(); + serverPkgInfo = + PackageInfoBuilder.newBuilder() + .setPackageName(serverAppInfo.packageName) + .setApplicationInfo(serverAppInfo) + .build(); + shadowOf(application.getPackageManager()).installPackage(serverPkgInfo); + + serviceInfo = new ServiceInfo(); + serviceInfo.name = "SomeService"; + serviceInfo.packageName = serverAppInfo.packageName; + serviceInfo.applicationInfo = serverAppInfo; + shadowOf(application.getPackageManager()).addOrUpdateService(serviceInfo); + + super.setUp(); + } + + @Before + public void requestRealisticBindServiceBehavior() { + shadowOf(application).setBindServiceCallsOnServiceConnectedDirectly(false); + shadowOf(application).setUnbindServiceCallsOnServiceDisconnected(false); + } + @Override protected InternalServer newServer(List streamTracerFactories) { - AndroidComponentAddress listenAddr = AndroidComponentAddress.forBindIntent( - new Intent() - .setClassName(application.getPackageName(), "HostService") - .setAction("io.grpc.action.BIND." + nextServerAddress++)); + AndroidComponentAddress listenAddr = + AndroidComponentAddress.forBindIntent( + new Intent() + .setClassName(serviceInfo.packageName, serviceInfo.name) + .setAction("io.grpc.action.BIND." + nextServerAddress++)); BinderServer binderServer = new BinderServer.Builder() @@ -81,6 +149,7 @@ protected InternalServer newServer(List streamTracer .setStreamTracerFactories(streamTracerFactories) .build(); + shadowOf(application.getPackageManager()).addServiceIfNotPresent(listenAddr.getComponent()); shadowOf(application) .setComponentNameAndServiceForBindServiceForIntent( listenAddr.asBindIntent(), listenAddr.getComponent(), binderServer.getHostBinder()); @@ -97,22 +166,30 @@ protected InternalServer newServer( return newServer(streamTracerFactories); } + BinderClientTransportFactory.Builder newClientTransportFactoryBuilder() { + return new BinderClientTransportFactory.Builder() + .setPreAuthorizeServers(preAuthServersParam) + .setSourceContext(application) + .setScheduledExecutorPool(executorServicePool) + .setOffloadExecutorPool(offloadExecutorPool); + } + + BinderClientTransportBuilder newClientTransportBuilder() { + return new BinderClientTransportBuilder() + .setClientTransportFactory(newClientTransportFactoryBuilder().buildClientTransportFactory()) + .setServerAddress(server.getListenSocketAddress()); + } + @Override protected ManagedClientTransport newClientTransport(InternalServer server) { - BinderClientTransportFactory.Builder builder = - new BinderClientTransportFactory.Builder() - .setSourceContext(application) - .setScheduledExecutorPool(executorServicePool) - .setOffloadExecutorPool(offloadExecutorPool); - ClientTransportOptions options = new ClientTransportOptions(); options.setEagAttributes(eagAttrs()); options.setChannelLogger(transportLogger()); - return new BinderTransport.BinderClientTransport( - builder.buildClientTransportFactory(), - (AndroidComponentAddress) server.getListenSocketAddress(), - options); + return newClientTransportBuilder() + .setServerAddress(server.getListenSocketAddress()) + .setClientTransportOptions(options) + .build(); } @Override @@ -120,6 +197,70 @@ protected String testAuthority(InternalServer server) { return ((AndroidComponentAddress) server.getListenSocketAddress()).getAuthority(); } + @Test + public void clientAuthorizesServerUidsInOrder() throws Exception { + // TODO(jdcormie): In real Android, Binder#getCallingUid is thread-local but Robolectric only + // lets us fake value this *globally*. So the ShadowBinder#setCallingUid() here unrealistically + // affects the server's view of the client's uid too. For now this doesn't matter because this + // test never exercises server SecurityPolicy. + ShadowBinder.setCallingUid(11111); // UID of the server *process*. + + serverPkgInfo.applicationInfo.uid = 22222; // UID of the server *app*, which can be different. + shadowOf(application.getPackageManager()).installPackage(serverPkgInfo); + shadowOf(application.getPackageManager()).addOrUpdateService(serviceInfo); + server = newServer(Collections.emptyList()); + server.start(serverListener); + + SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy(); + client = + newClientTransportBuilder() + .setClientTransportFactory( + newClientTransportFactoryBuilder() + .setSecurityPolicy(securityPolicy) + .buildClientTransportFactory()) + .build(); + runIfNotNull(client.start(mockClientTransportListener)); + + if (preAuthServersParam) { + AuthRequest preAuthRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_MS, MILLISECONDS); + assertThat(preAuthRequest.uid).isEqualTo(22222); + verify(mockClientTransportListener, never()).transportReady(); + preAuthRequest.setResult(Status.OK); + } + + AuthRequest authRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_MS, MILLISECONDS); + assertThat(authRequest.uid).isEqualTo(11111); + verify(mockClientTransportListener, never()).transportReady(); + authRequest.setResult(Status.OK); + + verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportReady(); + } + + @Test + public void eagAttributeCanOverrideChannelPreAuthServerSetting() throws Exception { + server.start(serverListener); + SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy(); + ClientTransportOptions options = new ClientTransportOptions(); + options.setEagAttributes( + Attributes.newBuilder().set(ApiConstants.PRE_AUTH_SERVER_OVERRIDE, true).build()); + client = + newClientTransportFactoryBuilder() + .setSecurityPolicy(securityPolicy) + .buildClientTransportFactory() + .newClientTransport(server.getListenSocketAddress(), options, null); + runIfNotNull(client.start(mockClientTransportListener)); + + AuthRequest preAuthRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_MS, MILLISECONDS); + verify(mockClientTransportListener, never()).transportReady(); + preAuthRequest.setResult(Status.OK); + + AuthRequest authRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_MS, MILLISECONDS); + verify(mockClientTransportListener, never()).transportReady(); + authRequest.setResult(Status.OK); + + verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportReady(); + } + @Test @Ignore("See BinderTransportTest#socketStats.") @Override diff --git a/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java b/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java index b0ad35e6806..b90547e6e93 100644 --- a/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java @@ -19,6 +19,7 @@ import static android.content.Context.BIND_AUTO_CREATE; import static android.os.Looper.getMainLooper; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; import static org.robolectric.Shadows.shadowOf; @@ -27,6 +28,7 @@ import android.content.ComponentName; import android.content.Context; import android.content.Intent; +import android.content.pm.ServiceInfo; import android.os.IBinder; import android.os.Parcel; import android.os.UserHandle; @@ -34,6 +36,7 @@ import androidx.test.core.app.ApplicationProvider; import io.grpc.Status; import io.grpc.Status.Code; +import io.grpc.StatusException; import io.grpc.binder.BinderChannelCredentials; import io.grpc.binder.internal.Bindable.Observer; import java.util.Arrays; @@ -59,6 +62,7 @@ public final class ServiceBindingTest { private Application appContext; private ComponentName serviceComponent; + private ServiceInfo serviceInfo = new ServiceInfo(); private ShadowApplication shadowApplication; private TestObserver observer; private ServiceBinding binding; @@ -67,13 +71,17 @@ public final class ServiceBindingTest { public void setUp() { appContext = ApplicationProvider.getApplicationContext(); serviceComponent = new ComponentName("DUMMY", "SERVICE"); + serviceInfo.packageName = serviceComponent.getPackageName(); + serviceInfo.name = serviceComponent.getClassName(); observer = new TestObserver(); shadowApplication = shadowOf(appContext); shadowApplication.setComponentNameAndServiceForBindService(serviceComponent, mockBinder); + shadowOf(appContext.getPackageManager()).addOrUpdateService(serviceInfo); // Don't call onServiceDisconnected() upon unbindService(), just like the real Android doesn't. shadowApplication.setUnbindServiceCallsOnServiceDisconnected(false); + shadowApplication.setBindServiceCallsOnServiceConnectedDirectly(false); binding = newBuilder().build(); shadowOf(getMainLooper()).idle(); @@ -276,6 +284,49 @@ public void testBindWithTargetUserHandle() throws Exception { assertThat(binding.isSourceContextCleared()).isFalse(); } + @Test + public void testResolve() throws Exception { + serviceInfo.processName = "x"; // ServiceInfo has no equals() so look for one distinctive field. + shadowOf(appContext.getPackageManager()).addOrUpdateService(serviceInfo); + ServiceInfo resolvedServiceInfo = binding.resolve(); + assertThat(resolvedServiceInfo.processName).isEqualTo(serviceInfo.processName); + } + + @Test + @Config(sdk = 33) + public void testResolveWithTargetUserHandle() throws Exception { + serviceInfo.processName = "x"; // ServiceInfo has no equals() so look for one distinctive field. + // Robolectric just ignores the user arg to resolveServiceAsUser() so this is all we can do. + shadowOf(appContext.getPackageManager()).addOrUpdateService(serviceInfo); + binding = newBuilder().setTargetUserHandle(generateUserHandle(/* userId= */ 0)).build(); + ServiceInfo resolvedServiceInfo = binding.resolve(); + assertThat(resolvedServiceInfo.processName).isEqualTo(serviceInfo.processName); + } + + @Test + public void testResolveNonExistentServiceThrows() throws Exception { + ComponentName doesNotExistService = new ComponentName("does.not.exist", "NoService"); + binding = newBuilder().setTargetComponent(doesNotExistService).build(); + StatusException statusException = assertThrows(StatusException.class, binding::resolve); + assertThat(statusException.getStatus().getCode()).isEqualTo(Code.UNIMPLEMENTED); + assertThat(statusException.getStatus().getDescription()).contains("does.not.exist"); + } + + @Test + @Config(sdk = 33) + public void testResolveNonExistentServiceWithTargetUserThrows() throws Exception { + ComponentName doesNotExistService = new ComponentName("does.not.exist", "NoService"); + binding = + newBuilder() + .setTargetUserHandle(generateUserHandle(/* userId= */ 12345)) + .setTargetComponent(doesNotExistService) + .build(); + StatusException statusException = assertThrows(StatusException.class, binding::resolve); + assertThat(statusException.getStatus().getCode()).isEqualTo(Code.UNIMPLEMENTED); + assertThat(statusException.getStatus().getDescription()).contains("does.not.exist"); + assertThat(statusException.getStatus().getDescription()).contains("12345"); + } + @Test @Config(sdk = 30) public void testBindWithDeviceAdmin() throws Exception { diff --git a/binder/src/testFixtures/java/io/grpc/binder/internal/BinderClientTransportBuilder.java b/binder/src/testFixtures/java/io/grpc/binder/internal/BinderClientTransportBuilder.java new file mode 100644 index 00000000000..18632a8c551 --- /dev/null +++ b/binder/src/testFixtures/java/io/grpc/binder/internal/BinderClientTransportBuilder.java @@ -0,0 +1,62 @@ +/* + * 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.base.Preconditions.checkNotNull; + +import io.grpc.ChannelLogger; +import io.grpc.internal.ClientTransportFactory.ClientTransportOptions; +import io.grpc.internal.TestUtils.NoopChannelLogger; +import java.net.SocketAddress; + +/** + * Helps unit tests create {@link BinderTransport.BinderClientTransport} instances without having to + * mention irrelevant details (go/tott/719). + */ +public class BinderClientTransportBuilder { + private BinderClientTransportFactory factory; + private SocketAddress serverAddress; + private ChannelLogger channelLogger = new NoopChannelLogger(); + private io.grpc.internal.ClientTransportFactory.ClientTransportOptions options = + new ClientTransportOptions(); + + public BinderClientTransportBuilder setServerAddress(SocketAddress serverAddress) { + this.serverAddress = checkNotNull(serverAddress); + return this; + } + + public BinderClientTransportBuilder setChannelLogger(ChannelLogger channelLogger) { + this.channelLogger = checkNotNull(channelLogger); + return this; + } + + public BinderClientTransportBuilder setClientTransportOptions(ClientTransportOptions options) { + this.options = checkNotNull(options); + return this; + } + + public BinderClientTransportBuilder setClientTransportFactory( + BinderClientTransportFactory factory) { + this.factory = checkNotNull(factory); + return this; + } + + public BinderTransport.BinderClientTransport build() { + return factory.newClientTransport( + checkNotNull(serverAddress), checkNotNull(options), checkNotNull(channelLogger)); + } +} diff --git a/binder/src/testFixtures/java/io/grpc/binder/internal/SettableAsyncSecurityPolicy.java b/binder/src/testFixtures/java/io/grpc/binder/internal/SettableAsyncSecurityPolicy.java new file mode 100644 index 00000000000..66ab8e9088d --- /dev/null +++ b/binder/src/testFixtures/java/io/grpc/binder/internal/SettableAsyncSecurityPolicy.java @@ -0,0 +1,78 @@ +/* + * 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.base.Preconditions.checkState; + +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import io.grpc.Status; +import io.grpc.binder.AsyncSecurityPolicy; +import java.util.concurrent.LinkedBlockingDeque; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +/** + * An {@link AsyncSecurityPolicy} that lets unit tests verify the exact order of authorization + * requests and respond to them one at a time. + */ +public class SettableAsyncSecurityPolicy extends AsyncSecurityPolicy { + private final LinkedBlockingDeque pendingRequests = new LinkedBlockingDeque<>(); + + @Override + public ListenableFuture checkAuthorizationAsync(int uid) { + AuthRequest request = new AuthRequest(uid); + pendingRequests.add(request); + return request.resultFuture; + } + + /** + * Waits for the next "check authorization" request to be made and returns it, throwing in case no + * request arrives in time. + */ + public AuthRequest takeNextAuthRequest(long timeout, TimeUnit unit) + throws InterruptedException, TimeoutException { + AuthRequest nextAuthRequest = pendingRequests.poll(timeout, unit); + if (nextAuthRequest == null) { + throw new TimeoutException(); + } + return nextAuthRequest; + } + + /** Represents a single call to {@link AsyncSecurityPolicy#checkAuthorizationAsync(int)}. */ + public static class AuthRequest { + + /** The argument passed to {@link AsyncSecurityPolicy#checkAuthorizationAsync(int)}. */ + public final int uid; + + private final SettableFuture resultFuture = SettableFuture.create(); + + private AuthRequest(int uid) { + this.uid = uid; + } + + /** Provides this SecurityPolicy's response to this authorization request. */ + public void setResult(Status result) { + checkState(resultFuture.set(result)); + } + + /** Simulates an exceptional response to this authorization request. */ + public void setResult(Throwable t) { + checkState(resultFuture.setException(t)); + } + } +} diff --git a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java index 1f4c2b41f15..3011c2679b9 100644 --- a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java @@ -93,7 +93,7 @@ public abstract class AbstractTransportTest { */ public static final int TEST_FLOW_CONTROL_WINDOW = 65 * 1024; - private static final int TIMEOUT_MS = 5000; + protected static final int TIMEOUT_MS = 5000; protected static final String GRPC_EXPERIMENTAL_SUPPORT_TRACING_MESSAGE_SIZES = "GRPC_EXPERIMENTAL_SUPPORT_TRACING_MESSAGE_SIZES"; @@ -2159,7 +2159,7 @@ private static boolean waitForFuture(Future future, long timeout, TimeUnit un return true; } - private static void runIfNotNull(Runnable runnable) { + protected static void runIfNotNull(Runnable runnable) { if (runnable != null) { runnable.run(); }