From 3d03902cf34e2e6a741c009b45a916f7baca8d42 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 21 Mar 2025 23:58:41 -0700 Subject: [PATCH 01/24] Create jules-setup.sh --- jules-setup.sh | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 jules-setup.sh diff --git a/jules-setup.sh b/jules-setup.sh new file mode 100644 index 00000000000..c66eb4fe6ea --- /dev/null +++ b/jules-setup.sh @@ -0,0 +1,9 @@ +export ANDROID_HOME=$HOME/Android/Sdk # Adjust to your liking +mkdir $HOME/Android +mkdir $ANDROID_HOME +mkdir $ANDROID_HOME/cmdline-tools +wget --progress=dot:mega --no-verbose https://dl.google.com/android/repository/commandlinetools-linux-11076708_latest.zip +unzip -d $ANDROID_HOME/cmdline-tools commandlinetools-*.zip +mv $ANDROID_HOME/cmdline-tools/cmdline-tools $ANDROID_HOME/cmdline-tools/latest +# Android SDK is now ready. Now accept licenses so the build can auto-download packages +yes | $ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager --licenses From d31d4883f99b17e569544d87445b0d648f1137c2 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Sat, 22 Mar 2025 01:12:58 -0700 Subject: [PATCH 02/24] Update jules-setup.sh --- jules-setup.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jules-setup.sh b/jules-setup.sh index c66eb4fe6ea..638bbf869c4 100644 --- a/jules-setup.sh +++ b/jules-setup.sh @@ -3,7 +3,7 @@ mkdir $HOME/Android mkdir $ANDROID_HOME mkdir $ANDROID_HOME/cmdline-tools wget --progress=dot:mega --no-verbose https://dl.google.com/android/repository/commandlinetools-linux-11076708_latest.zip -unzip -d $ANDROID_HOME/cmdline-tools commandlinetools-*.zip +unzip -q -n -d $ANDROID_HOME/cmdline-tools commandlinetools-*.zip mv $ANDROID_HOME/cmdline-tools/cmdline-tools $ANDROID_HOME/cmdline-tools/latest # Android SDK is now ready. Now accept licenses so the build can auto-download packages yes | $ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager --licenses From 4bf794c70c7bf81e9e42f5962bcc114d2694e7a9 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Sat, 22 Mar 2025 01:37:27 -0700 Subject: [PATCH 03/24] Create jules-run-tests.sh --- jules-run-tests.sh | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 jules-run-tests.sh diff --git a/jules-run-tests.sh b/jules-run-tests.sh new file mode 100644 index 00000000000..28d66e8c418 --- /dev/null +++ b/jules-run-tests.sh @@ -0,0 +1,3 @@ +./gradlew :grpc-api:check +./gradlew :grpc-core:check +./gradlew :grpc-binder:check From 3996edadb9663a76c8a11661f688934c4ff59a15 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Sat, 22 Mar 2025 01:38:23 -0700 Subject: [PATCH 04/24] Update jules-setup.sh --- jules-setup.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/jules-setup.sh b/jules-setup.sh index 638bbf869c4..095bc5d7ed0 100644 --- a/jules-setup.sh +++ b/jules-setup.sh @@ -1,4 +1,14 @@ export ANDROID_HOME=$HOME/Android/Sdk # Adjust to your liking + +PROTOBUF_VERSION=21.7 +curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v$PROTOBUF_VERSION/protobuf-all-$PROTOBUF_VERSION.tar.gz +tar xzf protobuf-all-$PROTOBUF_VERSION.tar.gz +cd protobuf-$PROTOBUF_VERSION +./configure --disable-shared +make -j 4 # You may want to pass -j to make this run faster; see make --help +sudo make install +cd .. + mkdir $HOME/Android mkdir $ANDROID_HOME mkdir $ANDROID_HOME/cmdline-tools From 5107080d6f1c2dc50c9b0d595bef6a8f1d921e6d Mon Sep 17 00:00:00 2001 From: John Cormie Date: Sat, 22 Mar 2025 01:49:11 -0700 Subject: [PATCH 05/24] Update jules-setup.sh --- jules-setup.sh | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/jules-setup.sh b/jules-setup.sh index 095bc5d7ed0..a57cfdda3ce 100644 --- a/jules-setup.sh +++ b/jules-setup.sh @@ -1,13 +1,15 @@ export ANDROID_HOME=$HOME/Android/Sdk # Adjust to your liking -PROTOBUF_VERSION=21.7 -curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v$PROTOBUF_VERSION/protobuf-all-$PROTOBUF_VERSION.tar.gz -tar xzf protobuf-all-$PROTOBUF_VERSION.tar.gz -cd protobuf-$PROTOBUF_VERSION -./configure --disable-shared -make -j 4 # You may want to pass -j to make this run faster; see make --help -sudo make install -cd .. +#PROTOBUF_VERSION=21.7 +#curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v$PROTOBUF_VERSION/protobuf-all-$PROTOBUF_VERSION.tar.gz +#tar xzf protobuf-all-$PROTOBUF_VERSION.tar.gz +#cd protobuf-$PROTOBUF_VERSION +#./configure --disable-shared +#make -j 4 # You may want to pass -j to make this run faster; see make --help +#sudo make install +#cd .. + +echo "skipCodegen=true" > gradle.properties mkdir $HOME/Android mkdir $ANDROID_HOME From a68435914e5da33de972c0daada7d9b7c52a8884 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Thu, 1 May 2025 18:40:51 -0700 Subject: [PATCH 06/24] binder: Optionally pre-authorize servers using their PackageManager identity before binding. --- .../internal/BinderClientTransportTest.java | 106 ++++++++++++------ .../java/io/grpc/binder/ApiConstants.java | 20 ++++ .../io/grpc/binder/BinderChannelBuilder.java | 27 ++++- .../io/grpc/binder/internal/Bindable.java | 5 + .../BinderClientTransportFactory.java | 14 +++ .../grpc/binder/internal/BinderTransport.java | 82 ++++++++++++-- .../grpc/binder/internal/ServiceBinding.java | 39 +++++++ .../grpc/binder/BinderChannelBuilderTest.java | 14 +++ .../binder/RobolectricBinderSecurityTest.java | 18 +++ .../binder/internal/ServiceBindingTest.java | 21 ++++ 10 files changed, 304 insertions(+), 42 deletions(-) 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..c4cedbd2e7f 100644 --- a/binder/src/main/java/io/grpc/binder/ApiConstants.java +++ b/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 TARGET_ANDROID_USER = NameResolver.Args.Key.create("target-android-user"); + + /** + * Marks an {@link io.grpc.EquivalentAddressGroup} as needing pre-authorization. + * + *

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 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 TARGET_SERVICE_INFO = + Attributes.Key.create("target-service-info"); } diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index 0b8f0bb4b3f..fd6bb9a85dc 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/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. + * + *

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

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

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( 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..43c05f155f2 100644 --- a/binder/src/main/java/io/grpc/binder/internal/Bindable.java +++ b/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. * 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..71b815883c2 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; 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; + } } } 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..c8563162cd0 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -22,6 +22,7 @@ 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; @@ -51,6 +52,7 @@ import io.grpc.Status; import io.grpc.StatusException; import io.grpc.binder.AndroidComponentAddress; +import io.grpc.binder.ApiConstants; import io.grpc.binder.AsyncSecurityPolicy; import io.grpc.binder.InboundParcelablePolicy; import io.grpc.binder.SecurityPolicy; @@ -299,7 +301,10 @@ protected boolean setOutgoingBinder(OneWayBinderProxy binder) { @Override public synchronized void binderDied() { - shutdownInternal(Status.UNAVAILABLE.withDescription("Peer process crashed, exited or was killed (binderDied)"), true); + shutdownInternal( + Status.UNAVAILABLE.withDescription( + "Peer process crashed, exited or was killed (binderDied)"), + true); } @GuardedBy("this") @@ -569,6 +574,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 +584,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 +611,9 @@ public BinderClientTransport( this.securityPolicy = factory.securityPolicy; this.offloadExecutor = offloadExecutorPool.getObject(); this.readyTimeoutMillis = factory.readyTimeoutMillis; + this.preAuthorizeServer = + factory.preAuthorizeServers + || (options.getEagAttributes().get(ApiConstants.PRE_AUTH_REQUIRED) != null); numInUseStreams = new AtomicInteger(); pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id)); @@ -643,7 +655,11 @@ public synchronized Runnable start(ManagedClientTransport.Listener clientTranspo synchronized (BinderClientTransport.this) { if (inState(TransportState.NOT_STARTED)) { setState(TransportState.SETUP); - serviceBinding.bind(); + if (preAuthorizeServer) { + preAuthorizeServer(); + } else { + serviceBinding.bind(); + } if (readyTimeoutMillis >= 0) { readyTimeoutFuture = getScheduledExecutorService() @@ -657,6 +673,41 @@ public synchronized Runnable start(ManagedClientTransport.Listener clientTranspo }; } + @GuardedBy("this") + private void preAuthorizeServer() { + Attributes eagAttrs = checkNotNull(attributes.get(GrpcAttributes.ATTR_CLIENT_EAG_ATTRS)); + ServiceInfo serviceInfo = eagAttrs.get(ApiConstants.TARGET_SERVICE_INFO); + if (serviceInfo == null) { + // It's unlikely, but the identity of the server could change by the time we actually + // bind/connect. It doesn't matter though, because: + // - if pre-auth fails, grpc-core will eventually retry against the replacement server. + // - even if pre-auth succeeds, we'll check the SecurityPolicy again in SETUP_TRANSPORT + // against the actual/new server's identity. + serviceInfo = serviceBinding.resolve(); + } + if (serviceInfo == null) { + preAuthResultFuture = + Futures.immediateFuture( + Status.UNIMPLEMENTED.withDescription("resolveService() was null")); + } else { + 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 onReadyTimeout() { if (inState(TransportState.SETUP)) { readyTimeoutFuture = null; @@ -751,6 +802,10 @@ void notifyTerminated() { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } + if (preAuthResultFuture != null) { + preAuthResultFuture.cancel(false); + preAuthResultFuture = null; + } serviceBinding.unbind(); clientTransportListener.transportTerminated(); } @@ -770,13 +825,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 +843,22 @@ 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 handlePreAuthResult(Status authorization) { + if (inState(TransportState.SETUP)) { + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + } else { + serviceBinding.bind(); + } + } + } + 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..abfe3dc9754 100644 --- a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java +++ b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java @@ -23,6 +23,9 @@ 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; @@ -31,6 +34,7 @@ import com.google.errorprone.annotations.concurrent.GuardedBy; import io.grpc.Status; import io.grpc.binder.BinderChannelCredentials; +import java.lang.reflect.Method; import java.util.concurrent.Executor; import java.util.logging.Level; import java.util.logging.Logger; @@ -247,6 +251,41 @@ void unbindInternal(Status reason) { } } + private static int getIdentifier(UserHandle userHandle) throws ReflectiveOperationException { + return (int) userHandle.getClass().getDeclaredMethod("getIdentifier").invoke(userHandle); + } + + private static ResolveInfo resolveServiceAsUser( + PackageManager packageManager, Intent bindIntent, int flags, UserHandle targetUserHandle) + throws ReflectiveOperationException { + Method resolveService; + Object[] args; + if (targetUserHandle == null) { + resolveService = + packageManager.getClass().getMethod("resolveService", Intent.class, int.class); + args = new Object[] {bindIntent, flags}; + } else { + resolveService = + packageManager + .getClass() + .getMethod("resolveServiceAsUser", Intent.class, int.class, int.class); + args = new Object[] {bindIntent, flags, getIdentifier(targetUserHandle)}; + } + return (ResolveInfo) resolveService.invoke(packageManager, args); + } + + @AnyThread + public ServiceInfo resolve() { + checkState(sourceContext != null); + try { + ResolveInfo resolveInfo = + resolveServiceAsUser(sourceContext.getPackageManager(), bindIntent, 0, targetUserHandle); + return resolveInfo != null ? resolveInfo.serviceInfo : null; + } catch (ReflectiveOperationException e) { + throw Status.fromThrowable(e).asRuntimeException(); + } + } + @MainThread private void clearReferences() { sourceContext = null; diff --git a/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java b/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java index 5dd7e13107e..e4f0c593bd3 100644 --- a/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java +++ b/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java @@ -16,6 +16,7 @@ package io.grpc.binder; +import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; import android.content.Context; @@ -41,4 +42,17 @@ public void strictLifecycleManagementForbidsIdleTimers() { // Expected. } } + + @Test + public void preAuthorizeTargetUris() { + BinderChannelBuilder builder = BinderChannelBuilder.forTarget("foo://bar", appContext); + assertThat(builder.transportFactoryBuilder.getPreAuthorizeServers()).isTrue(); + } + + @Test + public void noPreAuthorizeDirectAddresses() { + // TODO(jdcormie): Turn this on by default in a future release. + BinderChannelBuilder builder = BinderChannelBuilder.forAddress(addr, appContext); + assertThat(builder.transportFactoryBuilder.getPreAuthorizeServers()).isFalse(); + } } diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index ab81fc6b6d0..53c8bf3ad1e 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -80,6 +80,7 @@ public void setUp() { .executor(executor) .scheduledExecutorService(executor) .offloadExecutor(executor) + .preAuthorizeServers(false) .build(); idleLoopers(); } @@ -148,6 +149,23 @@ private static MethodDescriptor getMethodDescriptor() { .build(); } + static class ConstantSecurityPolicy extends AsyncSecurityPolicy { + final SettableFuture result = SettableFuture.create(); + + ConstantSecurityPolicy(Status status) { + result.set(status); + } + + ConstantSecurityPolicy(Throwable t) { + result.setException(t); + } + + @Override + public ListenableFuture checkAuthorizationAsync(int uid) { + return Futures.nonCancellationPropagating(result); + } + } + private static class SomeService extends LifecycleService { private final IBinderReceiver binderReceiver = new IBinderReceiver(); 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..25e81afc8af 100644 --- a/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/ServiceBindingTest.java @@ -27,6 +27,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; @@ -59,6 +60,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,10 +69,13 @@ 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); @@ -276,6 +281,22 @@ public void testBindWithTargetUserHandle() throws Exception { assertThat(binding.isSourceContextCleared()).isFalse(); } + @Test + public void testResolve() throws Exception { + ServiceInfo resolvedServiceInfo = binding.resolve(); + assertThat(resolvedServiceInfo.packageName).isEqualTo(serviceInfo.packageName); + assertThat(resolvedServiceInfo.name).isEqualTo(serviceInfo.name); + } + + @Test + @Config(sdk = 33) + public void testResolveWithTargetUserHandle() throws Exception { + binding = newBuilder().setTargetUserHandle(generateUserHandle(/* userId= */ 0)).build(); + ServiceInfo resolvedServiceInfo = binding.resolve(); + assertThat(resolvedServiceInfo.packageName).isEqualTo(serviceInfo.packageName); + assertThat(resolvedServiceInfo.name).isEqualTo(serviceInfo.name); + } + @Test @Config(sdk = 30) public void testBindWithDeviceAdmin() throws Exception { From 7cc704d815bd19c8328d2db3217466ebbf4c7358 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 2 May 2025 11:52:41 -0700 Subject: [PATCH 07/24] Cut the pre-auth-required attribute for now. --- binder/src/main/java/io/grpc/binder/ApiConstants.java | 11 ----------- .../java/io/grpc/binder/internal/BinderTransport.java | 4 +--- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/ApiConstants.java b/binder/src/main/java/io/grpc/binder/ApiConstants.java index c4cedbd2e7f..b5147480791 100644 --- a/binder/src/main/java/io/grpc/binder/ApiConstants.java +++ b/binder/src/main/java/io/grpc/binder/ApiConstants.java @@ -46,17 +46,6 @@ private ApiConstants() {} public static final NameResolver.Args.Key TARGET_ANDROID_USER = NameResolver.Args.Key.create("target-android-user"); - /** - * Marks an {@link io.grpc.EquivalentAddressGroup} as needing pre-authorization. - * - *

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 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. 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 c8563162cd0..3c7ddcd862d 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -611,9 +611,7 @@ public BinderClientTransport( this.securityPolicy = factory.securityPolicy; this.offloadExecutor = offloadExecutorPool.getObject(); this.readyTimeoutMillis = factory.readyTimeoutMillis; - this.preAuthorizeServer = - factory.preAuthorizeServers - || (options.getEagAttributes().get(ApiConstants.PRE_AUTH_REQUIRED) != null); + this.preAuthorizeServer = factory.preAuthorizeServers; numInUseStreams = new AtomicInteger(); pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id)); From 80c9b900f091b2286c31b4891f51590b0413dbbd Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 2 May 2025 12:02:23 -0700 Subject: [PATCH 08/24] undo java format --- .../src/main/java/io/grpc/binder/BinderChannelBuilder.java | 3 +-- .../main/java/io/grpc/binder/internal/BinderTransport.java | 5 +---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index fd6bb9a85dc..09e7442faea 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java @@ -179,8 +179,7 @@ private BinderChannelBuilder( } else { managedChannelImplBuilder = new ManagedChannelImplBuilder(target, transportFactoryBuilder, null); - - preAuthorizeServers(true); // Pre-authorize resolved addresses by default. + preAuthorizeServers(true); // Pre-authorize addresses from a NameResolver by default. } idleTimeout(60, TimeUnit.SECONDS); } 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 3c7ddcd862d..e2bc9385b1b 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -301,10 +301,7 @@ protected boolean setOutgoingBinder(OneWayBinderProxy binder) { @Override public synchronized void binderDied() { - shutdownInternal( - Status.UNAVAILABLE.withDescription( - "Peer process crashed, exited or was killed (binderDied)"), - true); + shutdownInternal(Status.UNAVAILABLE.withDescription("Peer process crashed, exited or was killed (binderDied)"), true); } @GuardedBy("this") From 71943c6422e8aed200f453cb9c2f62f36d243002 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 2 May 2025 12:55:53 -0700 Subject: [PATCH 09/24] undo default enabled --- .../java/io/grpc/binder/BinderChannelBuilder.java | 7 +++++-- .../internal/BinderClientTransportFactory.java | 5 ----- .../io/grpc/binder/BinderChannelBuilderTest.java | 14 -------------- 3 files changed, 5 insertions(+), 21 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index 09e7442faea..1d65917da8b 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java @@ -158,7 +158,7 @@ public static BinderChannelBuilder forTarget(String target) { } private final ManagedChannelImplBuilder managedChannelImplBuilder; - final BinderClientTransportFactory.Builder transportFactoryBuilder; + private final BinderClientTransportFactory.Builder transportFactoryBuilder; private boolean strictLifecycleManagement; @@ -176,10 +176,13 @@ private BinderChannelBuilder( managedChannelImplBuilder = new ManagedChannelImplBuilder( directAddress, directAddress.getAuthority(), transportFactoryBuilder, null); + // TODO(jdcormie): Rollout step 2: Pre-auth *all* addresses by default. + preAuthorizeServers(false); } else { managedChannelImplBuilder = new ManagedChannelImplBuilder(target, transportFactoryBuilder, null); - preAuthorizeServers(true); // Pre-authorize addresses from a NameResolver by default. + // TODO(jdcormie): Rollout step 1: Pre-auth NameResolver results by default. + preAuthorizeServers(false); } idleTimeout(60, TimeUnit.SECONDS); } 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 71b815883c2..07c38231cc5 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java @@ -225,10 +225,5 @@ 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; - } } } diff --git a/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java b/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java index e4f0c593bd3..5dd7e13107e 100644 --- a/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java +++ b/binder/src/test/java/io/grpc/binder/BinderChannelBuilderTest.java @@ -16,7 +16,6 @@ package io.grpc.binder; -import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; import android.content.Context; @@ -42,17 +41,4 @@ public void strictLifecycleManagementForbidsIdleTimers() { // Expected. } } - - @Test - public void preAuthorizeTargetUris() { - BinderChannelBuilder builder = BinderChannelBuilder.forTarget("foo://bar", appContext); - assertThat(builder.transportFactoryBuilder.getPreAuthorizeServers()).isTrue(); - } - - @Test - public void noPreAuthorizeDirectAddresses() { - // TODO(jdcormie): Turn this on by default in a future release. - BinderChannelBuilder builder = BinderChannelBuilder.forAddress(addr, appContext); - assertThat(builder.transportFactoryBuilder.getPreAuthorizeServers()).isFalse(); - } } From af3ff6c3f550e13e3782f82df9a7668f27dc3f53 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 6 May 2025 12:25:26 -0700 Subject: [PATCH 10/24] move/update the comment --- .../io/grpc/binder/internal/BinderTransport.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) 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 e2bc9385b1b..2bd940e7c91 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -673,13 +673,16 @@ private void preAuthorizeServer() { Attributes eagAttrs = checkNotNull(attributes.get(GrpcAttributes.ATTR_CLIENT_EAG_ATTRS)); ServiceInfo serviceInfo = eagAttrs.get(ApiConstants.TARGET_SERVICE_INFO); if (serviceInfo == null) { - // It's unlikely, but the identity of the server could change by the time we actually - // bind/connect. It doesn't matter though, because: - // - if pre-auth fails, grpc-core will eventually retry against the replacement server. - // - even if pre-auth succeeds, we'll check the SecurityPolicy again in SETUP_TRANSPORT - // against the actual/new server's identity. serviceInfo = serviceBinding.resolve(); } + + // It's unlikely, but in theory the server identity/existence represented by this ServiceInfo + // could change by the time we actually bind/connect. It doesn't matter though, because: + // - If pre-auth fails (but would succeed for the new identity), grpc-core will retry + // against the replacement server using a new instance of BinderClientTransport. + // - If pre-auth succeeds (but would fail for the new identity), we might incorrectly bind + // to an unauthorized server, but we'll check the SecurityPolicy again in SETUP_TRANSPORT + // against the actual/new server's identity. if (serviceInfo == null) { preAuthResultFuture = Futures.immediateFuture( From a0e76afa3a6f18467992ad45da09d049643c538a Mon Sep 17 00:00:00 2001 From: John Cormie Date: Mon, 2 Jun 2025 17:01:29 -0700 Subject: [PATCH 11/24] remove jules --- .../binder/RobolectricBinderSecurityTest.java | 17 --------------- jules-run-tests.sh | 3 --- jules-setup.sh | 21 ------------------- 3 files changed, 41 deletions(-) delete mode 100644 jules-run-tests.sh delete mode 100644 jules-setup.sh diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index 53c8bf3ad1e..4716009bed7 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -149,23 +149,6 @@ private static MethodDescriptor getMethodDescriptor() { .build(); } - static class ConstantSecurityPolicy extends AsyncSecurityPolicy { - final SettableFuture result = SettableFuture.create(); - - ConstantSecurityPolicy(Status status) { - result.set(status); - } - - ConstantSecurityPolicy(Throwable t) { - result.setException(t); - } - - @Override - public ListenableFuture checkAuthorizationAsync(int uid) { - return Futures.nonCancellationPropagating(result); - } - } - private static class SomeService extends LifecycleService { private final IBinderReceiver binderReceiver = new IBinderReceiver(); diff --git a/jules-run-tests.sh b/jules-run-tests.sh deleted file mode 100644 index 28d66e8c418..00000000000 --- a/jules-run-tests.sh +++ /dev/null @@ -1,3 +0,0 @@ -./gradlew :grpc-api:check -./gradlew :grpc-core:check -./gradlew :grpc-binder:check diff --git a/jules-setup.sh b/jules-setup.sh deleted file mode 100644 index a57cfdda3ce..00000000000 --- a/jules-setup.sh +++ /dev/null @@ -1,21 +0,0 @@ -export ANDROID_HOME=$HOME/Android/Sdk # Adjust to your liking - -#PROTOBUF_VERSION=21.7 -#curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v$PROTOBUF_VERSION/protobuf-all-$PROTOBUF_VERSION.tar.gz -#tar xzf protobuf-all-$PROTOBUF_VERSION.tar.gz -#cd protobuf-$PROTOBUF_VERSION -#./configure --disable-shared -#make -j 4 # You may want to pass -j to make this run faster; see make --help -#sudo make install -#cd .. - -echo "skipCodegen=true" > gradle.properties - -mkdir $HOME/Android -mkdir $ANDROID_HOME -mkdir $ANDROID_HOME/cmdline-tools -wget --progress=dot:mega --no-verbose https://dl.google.com/android/repository/commandlinetools-linux-11076708_latest.zip -unzip -q -n -d $ANDROID_HOME/cmdline-tools commandlinetools-*.zip -mv $ANDROID_HOME/cmdline-tools/cmdline-tools $ANDROID_HOME/cmdline-tools/latest -# Android SDK is now ready. Now accept licenses so the build can auto-download packages -yes | $ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager --licenses From 0c494696805f00af36f1ce6728c004986d6d684c Mon Sep 17 00:00:00 2001 From: John Cormie Date: Mon, 2 Jun 2025 17:22:08 -0700 Subject: [PATCH 12/24] javadoc --- .../io/grpc/binder/BinderChannelBuilder.java | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index 1d65917da8b..885a3ae5da0 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java @@ -284,22 +284,26 @@ public BinderChannelBuilder strictLifecycleManagement() { } /** - * Checks server addresses against this channel's {@link SecurityPolicy} *before* connecting. + * 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 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. + * an appropriate {@link SecurityPolicy}. But merely binding to a malicious server can enable + * "keep-alive" and "background activity launch" attacks, 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. * - *

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

Note that, unlike ordinary authorization, pre-authorization is performed against the server + * app's UID, not the UID of the server process. 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. + * Clients that require a particular behavior should configure it explicitly using this method + * rather than relying on the default. */ public BinderChannelBuilder preAuthorizeServers(boolean preAuthorize) { transportFactoryBuilder.setPreAuthorizeServers(preAuthorize); From bde880fff65ee08fd0363cdc4861fcbba22cb10c Mon Sep 17 00:00:00 2001 From: John Cormie Date: Mon, 2 Jun 2025 17:46:54 -0700 Subject: [PATCH 13/24] Leave TARGET_SERVICE_INFO for a follow up --- binder/src/main/java/io/grpc/binder/ApiConstants.java | 7 ------- .../java/io/grpc/binder/internal/BinderTransport.java | 10 +++------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/ApiConstants.java b/binder/src/main/java/io/grpc/binder/ApiConstants.java index b5147480791..f2b2931ca49 100644 --- a/binder/src/main/java/io/grpc/binder/ApiConstants.java +++ b/binder/src/main/java/io/grpc/binder/ApiConstants.java @@ -45,11 +45,4 @@ private ApiConstants() {} */ public static final NameResolver.Args.Key TARGET_ANDROID_USER = NameResolver.Args.Key.create("target-android-user"); - - /** - * 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 TARGET_SERVICE_INFO = - Attributes.Key.create("target-service-info"); } 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 2bd940e7c91..a1c1f1ff69d 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -670,19 +670,15 @@ public synchronized Runnable start(ManagedClientTransport.Listener clientTranspo @GuardedBy("this") private void preAuthorizeServer() { - Attributes eagAttrs = checkNotNull(attributes.get(GrpcAttributes.ATTR_CLIENT_EAG_ATTRS)); - ServiceInfo serviceInfo = eagAttrs.get(ApiConstants.TARGET_SERVICE_INFO); - if (serviceInfo == null) { - serviceInfo = serviceBinding.resolve(); - } + ServiceInfo serviceInfo = serviceBinding.resolve(); // It's unlikely, but in theory the server identity/existence represented by this ServiceInfo // could change by the time we actually bind/connect. It doesn't matter though, because: // - If pre-auth fails (but would succeed for the new identity), grpc-core will retry // against the replacement server using a new instance of BinderClientTransport. // - If pre-auth succeeds (but would fail for the new identity), we might incorrectly bind - // to an unauthorized server, but we'll check the SecurityPolicy again in SETUP_TRANSPORT - // against the actual/new server's identity. + // to an unauthorized server, but we'll notice when we the SecurityPolicy again as part of the + // usual handshake. if (serviceInfo == null) { preAuthResultFuture = Futures.immediateFuture( From 9ec685bc9de273b792a4474b2a687b4fd3b09acc Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 00:50:33 -0700 Subject: [PATCH 14/24] resolve() throws --- .../io/grpc/binder/internal/Bindable.java | 4 +++- .../grpc/binder/internal/BinderTransport.java | 24 +++++++++---------- .../grpc/binder/internal/ServiceBinding.java | 11 +++++++-- 3 files changed, 24 insertions(+), 15 deletions(-) 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 43c05f155f2..12886ff36ea 100644 --- a/binder/src/main/java/io/grpc/binder/internal/Bindable.java +++ b/binder/src/main/java/io/grpc/binder/internal/Bindable.java @@ -20,7 +20,9 @@ import android.os.IBinder; import androidx.annotation.AnyThread; import androidx.annotation.MainThread; +import com.google.common.util.concurrent.ListenableFuture; import io.grpc.Status; +import io.grpc.StatusException; /** An interface for managing a {@code Binder} connection. */ interface Bindable { @@ -48,7 +50,7 @@ interface Observer { /** Fetches details about the remote service from PackageManager *before* binding to it. */ @AnyThread - ServiceInfo resolve(); + ServiceInfo resolve() throws StatusException; /** * Attempt to bind with the remote service. 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 a1c1f1ff69d..c9118cc6f1a 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -670,22 +670,22 @@ public synchronized Runnable start(ManagedClientTransport.Listener clientTranspo @GuardedBy("this") private void preAuthorizeServer() { - ServiceInfo serviceInfo = serviceBinding.resolve(); + ServiceInfo serviceInfo; + try { + serviceInfo = serviceBinding.resolve(); + } catch (StatusException e) { + shutdownInternal(e.getStatus(), true); + return; + } - // It's unlikely, but in theory the server identity/existence represented by this ServiceInfo - // could change by the time we actually bind/connect. It doesn't matter though, because: + // It's unlikely, but the server 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 for the new identity), grpc-core will retry // against the replacement server using a new instance of BinderClientTransport. // - If pre-auth succeeds (but would fail for the new identity), we might incorrectly bind - // to an unauthorized server, but we'll notice when we the SecurityPolicy again as part of the - // usual handshake. - if (serviceInfo == null) { - preAuthResultFuture = - Futures.immediateFuture( - Status.UNIMPLEMENTED.withDescription("resolveService() was null")); - } else { - preAuthResultFuture = checkServerAuthorizationAsync(serviceInfo.applicationInfo.uid); - } + // to an unauthorized server, but we'll notice when we check SecurityPolicy again as part of + // the usual handshake. + preAuthResultFuture = checkServerAuthorizationAsync(serviceInfo.applicationInfo.uid); Futures.addCallback( preAuthResultFuture, new FutureCallback() { 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 abfe3dc9754..686bee7d8e9 100644 --- a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java +++ b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java @@ -33,6 +33,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.errorprone.annotations.concurrent.GuardedBy; import io.grpc.Status; +import io.grpc.StatusException; import io.grpc.binder.BinderChannelCredentials; import java.lang.reflect.Method; import java.util.concurrent.Executor; @@ -275,12 +276,18 @@ private static ResolveInfo resolveServiceAsUser( } @AnyThread - public ServiceInfo resolve() { + public ServiceInfo resolve() throws StatusException { checkState(sourceContext != null); try { ResolveInfo resolveInfo = resolveServiceAsUser(sourceContext.getPackageManager(), bindIntent, 0, targetUserHandle); - return resolveInfo != null ? resolveInfo.serviceInfo : null; + if (resolveInfo == null) { + // Same code as when bindService() returns false. + throw Status.UNIMPLEMENTED + .withDescription("resolveService(" + bindIntent + ") returned null") + .asException(); + } + return resolveInfo.serviceInfo; } catch (ReflectiveOperationException e) { throw Status.fromThrowable(e).asRuntimeException(); } From b2b006b9bd54ae8bad195521ba7fc1fb2f058b1c Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 11:37:48 -0700 Subject: [PATCH 15/24] Test with and without preauth --- .../RobolectricBinderTransportTest.java | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) 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..0a05a6df563 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -21,6 +21,7 @@ import android.app.Application; import android.content.Intent; import androidx.test.core.app.ApplicationProvider; +import com.google.common.collect.ImmutableList; import io.grpc.ServerStreamTracer; import io.grpc.binder.AndroidComponentAddress; import io.grpc.internal.AbstractTransportTest; @@ -33,10 +34,13 @@ 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.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; @@ -52,7 +56,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 { @@ -66,6 +70,20 @@ public final class RobolectricBinderTransportTest extends AbstractTransportTest private int nextServerAddress; + @Parameter + public boolean preAuthorizeServers; + + @Parameters(name = "preAuthorizeServers={0}") + public static ImmutableList data() { + return ImmutableList.of(true, false); + } + + @Before + public void requestRealisticBindServiceBehavior() { + shadowOf(application).setBindServiceCallsOnServiceConnectedDirectly(false); + shadowOf(application).setUnbindServiceCallsOnServiceDisconnected(false); + } + @Override protected InternalServer newServer(List streamTracerFactories) { AndroidComponentAddress listenAddr = AndroidComponentAddress.forBindIntent( @@ -81,6 +99,7 @@ protected InternalServer newServer(List streamTracer .setStreamTracerFactories(streamTracerFactories) .build(); + shadowOf(application.getPackageManager()).addServiceIfNotPresent(listenAddr.getComponent()); shadowOf(application) .setComponentNameAndServiceForBindServiceForIntent( listenAddr.asBindIntent(), listenAddr.getComponent(), binderServer.getHostBinder()); @@ -101,6 +120,7 @@ protected InternalServer newServer( protected ManagedClientTransport newClientTransport(InternalServer server) { BinderClientTransportFactory.Builder builder = new BinderClientTransportFactory.Builder() + .setPreAuthorizeServers(preAuthorizeServers) .setSourceContext(application) .setScheduledExecutorPool(executorServicePool) .setOffloadExecutorPool(offloadExecutorPool); From 741ef767a32ec7d204cf72fdd89fb7b79ddb15a5 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 12:26:38 -0700 Subject: [PATCH 16/24] ServiceBinding docs/tests --- .../io/grpc/binder/internal/Bindable.java | 11 ++++++++- .../binder/internal/ServiceBindingTest.java | 23 +++++++++++++++---- 2 files changed, 29 insertions(+), 5 deletions(-) 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 12886ff36ea..04adf724bf2 100644 --- a/binder/src/main/java/io/grpc/binder/internal/Bindable.java +++ b/binder/src/main/java/io/grpc/binder/internal/Bindable.java @@ -48,7 +48,16 @@ interface Observer { void onUnbound(Status reason); } - /** Fetches details about the remote service from PackageManager *before* binding to it. */ + /** + * 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; 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 25e81afc8af..0194cb2d276 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; @@ -35,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; @@ -79,6 +81,7 @@ public void setUp() { // 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(); @@ -283,18 +286,30 @@ public void testBindWithTargetUserHandle() throws Exception { @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.packageName).isEqualTo(serviceInfo.packageName); - assertThat(resolvedServiceInfo.name).isEqualTo(serviceInfo.name); + 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.packageName).isEqualTo(serviceInfo.packageName); - assertThat(resolvedServiceInfo.name).isEqualTo(serviceInfo.name); + 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 From 2eb26ec71aa7fc9ccf9632cc6cc45d47efd158c5 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 12:33:16 -0700 Subject: [PATCH 17/24] unused imports --- binder/src/main/java/io/grpc/binder/ApiConstants.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/ApiConstants.java b/binder/src/main/java/io/grpc/binder/ApiConstants.java index f2b2931ca49..292c580c2b8 100644 --- a/binder/src/main/java/io/grpc/binder/ApiConstants.java +++ b/binder/src/main/java/io/grpc/binder/ApiConstants.java @@ -17,9 +17,7 @@ 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; From 148457debaabb462a99fa2d29ed2c1f92d779831 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 12:40:25 -0700 Subject: [PATCH 18/24] doc tweaks --- .../main/java/io/grpc/binder/BinderChannelBuilder.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index 885a3ae5da0..9002653378b 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java @@ -284,19 +284,19 @@ public BinderChannelBuilder strictLifecycleManagement() { } /** - * Checks servers against this channel's {@link SecurityPolicy} *before* binding. + * 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" attacks, even if security policy ultimately + * "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 server process. These can be different, most commonly due to - * services that set `android:isolatedProcess=true`. + * 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. From 56c6f903f3e6266aa9ff6c70a567ceaee06dc0bb Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 12:41:10 -0700 Subject: [PATCH 19/24] doc tweaks --- .../java/io/grpc/binder/internal/Bindable.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 04adf724bf2..b2f03688dc8 100644 --- a/binder/src/main/java/io/grpc/binder/internal/Bindable.java +++ b/binder/src/main/java/io/grpc/binder/internal/Bindable.java @@ -49,14 +49,14 @@ interface Observer { } /** - * Fetches details about the remote service from PackageManager without binding to it. + * 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. + *

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; From 49ee8784ba0282c8e474a7b59ced692d4b10f1ca Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 12:43:19 -0700 Subject: [PATCH 20/24] java format --- .../src/main/java/io/grpc/binder/BinderChannelBuilder.java | 6 +++--- binder/src/main/java/io/grpc/binder/internal/Bindable.java | 1 - .../main/java/io/grpc/binder/internal/BinderTransport.java | 1 - .../binder/internal/RobolectricBinderTransportTest.java | 3 +-- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index 9002653378b..78a04343ab8 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java @@ -289,9 +289,9 @@ public BinderChannelBuilder strictLifecycleManagement() { *

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 + * "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 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 b2f03688dc8..fdbeefddbd2 100644 --- a/binder/src/main/java/io/grpc/binder/internal/Bindable.java +++ b/binder/src/main/java/io/grpc/binder/internal/Bindable.java @@ -20,7 +20,6 @@ import android.os.IBinder; import androidx.annotation.AnyThread; import androidx.annotation.MainThread; -import com.google.common.util.concurrent.ListenableFuture; import io.grpc.Status; import io.grpc.StatusException; 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 c9118cc6f1a..2ebf63c2d24 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -52,7 +52,6 @@ import io.grpc.Status; import io.grpc.StatusException; import io.grpc.binder.AndroidComponentAddress; -import io.grpc.binder.ApiConstants; import io.grpc.binder.AsyncSecurityPolicy; import io.grpc.binder.InboundParcelablePolicy; import io.grpc.binder.SecurityPolicy; 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 0a05a6df563..c6afa4dcc1a 100644 --- a/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java @@ -70,8 +70,7 @@ public final class RobolectricBinderTransportTest extends AbstractTransportTest private int nextServerAddress; - @Parameter - public boolean preAuthorizeServers; + @Parameter public boolean preAuthorizeServers; @Parameters(name = "preAuthorizeServers={0}") public static ImmutableList data() { From 2632951cb61b9b897fa7594f746dd5ca51806168 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 13:03:21 -0700 Subject: [PATCH 21/24] comment --- .../io/grpc/binder/internal/BinderTransport.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) 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 2ebf63c2d24..a7af27f8858 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -677,13 +677,14 @@ private void preAuthorizeServer() { return; } - // It's unlikely, but the server 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 for the new identity), grpc-core will retry - // against the replacement server using a new instance of BinderClientTransport. - // - If pre-auth succeeds (but would fail for the new identity), we might incorrectly bind - // to an unauthorized server, but we'll notice when we check SecurityPolicy again as part of - // the usual handshake. + // 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, From 5928c45d6616879aa6934581f01974779d936b02 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 13:08:25 -0700 Subject: [PATCH 22/24] format --- .../src/main/java/io/grpc/binder/internal/ServiceBinding.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 686bee7d8e9..ed8c96c4671 100644 --- a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java +++ b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java @@ -282,8 +282,7 @@ public ServiceInfo resolve() throws StatusException { ResolveInfo resolveInfo = resolveServiceAsUser(sourceContext.getPackageManager(), bindIntent, 0, targetUserHandle); if (resolveInfo == null) { - // Same code as when bindService() returns false. - throw Status.UNIMPLEMENTED + throw Status.UNIMPLEMENTED // Same code as when bindService() returns false. .withDescription("resolveService(" + bindIntent + ") returned null") .asException(); } From daddd3c78a7dfa85c08fd491d70154a5d87a6315 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 13:24:46 -0700 Subject: [PATCH 23/24] hide reflection mess in a static method --- .../grpc/binder/internal/ServiceBinding.java | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-) 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 ed8c96c4671..308d70e2002 100644 --- a/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java +++ b/binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java @@ -31,11 +31,11 @@ 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.lang.reflect.Method; import java.util.concurrent.Executor; import java.util.logging.Level; import java.util.logging.Logger; @@ -257,39 +257,32 @@ private static int getIdentifier(UserHandle userHandle) throws ReflectiveOperati } private static ResolveInfo resolveServiceAsUser( - PackageManager packageManager, Intent bindIntent, int flags, UserHandle targetUserHandle) - throws ReflectiveOperationException { - Method resolveService; - Object[] args; - if (targetUserHandle == null) { - resolveService = - packageManager.getClass().getMethod("resolveService", Intent.class, int.class); - args = new Object[] {bindIntent, flags}; - } else { - resolveService = + PackageManager packageManager, Intent intent, int flags, UserHandle targetUserHandle) { + try { + return (ResolveInfo) packageManager .getClass() - .getMethod("resolveServiceAsUser", Intent.class, int.class, int.class); - args = new Object[] {bindIntent, flags, getIdentifier(targetUserHandle)}; + .getMethod("resolveServiceAsUser", Intent.class, int.class, int.class) + .invoke(packageManager, intent, flags, getIdentifier(targetUserHandle)); + } catch (ReflectiveOperationException e) { + throw new VerifyException(e); } - return (ResolveInfo) resolveService.invoke(packageManager, args); } @AnyThread public ServiceInfo resolve() throws StatusException { checkState(sourceContext != null); - try { - ResolveInfo resolveInfo = - resolveServiceAsUser(sourceContext.getPackageManager(), bindIntent, 0, targetUserHandle); - if (resolveInfo == null) { - throw Status.UNIMPLEMENTED // Same code as when bindService() returns false. - .withDescription("resolveService(" + bindIntent + ") returned null") - .asException(); - } - return resolveInfo.serviceInfo; - } catch (ReflectiveOperationException e) { - throw Status.fromThrowable(e).asRuntimeException(); + 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 + ") returned null") + .asException(); } + return resolveInfo.serviceInfo; } @MainThread From c2a75961a1b0fb89279b6d4fbea02ea24a163783 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 3 Jun 2025 13:33:38 -0700 Subject: [PATCH 24/24] experimental --- binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java | 1 + 1 file changed, 1 insertion(+) diff --git a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java index 78a04343ab8..8be0d675f84 100644 --- a/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java @@ -305,6 +305,7 @@ public BinderChannelBuilder strictLifecycleManagement() { * 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;