Skip to content

Commit c643e68

Browse files
authored
binder: REMOTE_UID must hold exactly the uid passed to the SecurityPolicy and never change (#12314)
`attributes = setSecurityAttrs(attributes, remoteUid);` should not run for : - a malformed SETUP_TRANSPORT transaction - a rogue SETUP_TRANSPORT transaction that arrives post-TransportState.SETUP
1 parent afef4fe commit c643e68

File tree

2 files changed

+42
-2
lines changed

2 files changed

+42
-2
lines changed

binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,6 @@ void notifyTerminated() {
330330
@GuardedBy("this")
331331
protected void handleSetupTransport(Parcel parcel) {
332332
int remoteUid = Binder.getCallingUid();
333-
attributes = setSecurityAttrs(attributes, remoteUid);
334333
if (inState(TransportState.SETUP)) {
335334
int version = parcel.readInt();
336335
IBinder binder = parcel.readStrongBinder();
@@ -340,6 +339,7 @@ protected void handleSetupTransport(Parcel parcel) {
340339
shutdownInternal(
341340
Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true);
342341
} else {
342+
attributes = setSecurityAttrs(attributes, remoteUid);
343343
authResultFuture = checkServerAuthorizationAsync(remoteUid);
344344
Futures.addCallback(
345345
authResultFuture,

binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,11 @@
1616

1717
package io.grpc.binder.internal;
1818

19+
import static android.os.Process.myUid;
1920
import static com.google.common.truth.Truth.assertThat;
21+
import static io.grpc.binder.internal.BinderTransport.REMOTE_UID;
22+
import static io.grpc.binder.internal.BinderTransport.SETUP_TRANSPORT;
23+
import static io.grpc.binder.internal.BinderTransport.WIRE_FORMAT_VERSION;
2024
import static java.util.concurrent.TimeUnit.MILLISECONDS;
2125
import static org.mockito.Mockito.never;
2226
import static org.mockito.Mockito.timeout;
@@ -28,6 +32,8 @@
2832
import android.content.pm.ApplicationInfo;
2933
import android.content.pm.PackageInfo;
3034
import android.content.pm.ServiceInfo;
35+
import android.os.Binder;
36+
import android.os.Parcel;
3137
import androidx.test.core.app.ApplicationProvider;
3238
import androidx.test.core.content.pm.ApplicationInfoBuilder;
3339
import androidx.test.core.content.pm.PackageInfoBuilder;
@@ -38,9 +44,11 @@
3844
import io.grpc.binder.AndroidComponentAddress;
3945
import io.grpc.binder.ApiConstants;
4046
import io.grpc.binder.AsyncSecurityPolicy;
47+
import io.grpc.binder.SecurityPolicies;
4148
import io.grpc.binder.internal.SettableAsyncSecurityPolicy.AuthRequest;
4249
import io.grpc.internal.AbstractTransportTest;
4350
import io.grpc.internal.ClientTransportFactory.ClientTransportOptions;
51+
import io.grpc.internal.ConnectionClientTransport;
4452
import io.grpc.internal.GrpcUtil;
4553
import io.grpc.internal.InternalServer;
4654
import io.grpc.internal.ManagedClientTransport;
@@ -109,7 +117,7 @@ public static ImmutableList<Boolean> data() {
109117
public void setUp() {
110118
serverAppInfo =
111119
ApplicationInfoBuilder.newBuilder().setPackageName("the.server.package").build();
112-
serverAppInfo.uid = android.os.Process.myUid();
120+
serverAppInfo.uid = myUid();
113121
serverPkgInfo =
114122
PackageInfoBuilder.newBuilder()
115123
.setPackageName(serverAppInfo.packageName)
@@ -264,6 +272,38 @@ public void eagAttributeCanOverrideChannelPreAuthServerSetting() throws Exceptio
264272
verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportReady();
265273
}
266274

275+
@Test
276+
public void clientIgnoresDuplicateSetupTransaction() throws Exception {
277+
server.start(serverListener);
278+
client =
279+
newClientTransportBuilder()
280+
.setFactory(
281+
newClientTransportFactoryBuilder()
282+
.setSecurityPolicy(SecurityPolicies.internalOnly())
283+
.buildClientTransportFactory())
284+
.build();
285+
runIfNotNull(client.start(mockClientTransportListener));
286+
verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportReady();
287+
288+
assertThat(((ConnectionClientTransport) client).getAttributes().get(REMOTE_UID))
289+
.isEqualTo(myUid());
290+
291+
Parcel setupParcel = Parcel.obtain();
292+
try {
293+
setupParcel.writeInt(WIRE_FORMAT_VERSION);
294+
setupParcel.writeStrongBinder(new Binder());
295+
setupParcel.setDataPosition(0);
296+
ShadowBinder.setCallingUid(1 + myUid());
297+
((BinderClientTransport) client).handleTransaction(SETUP_TRANSPORT, setupParcel);
298+
} finally {
299+
ShadowBinder.setCallingUid(myUid());
300+
setupParcel.recycle();
301+
}
302+
303+
assertThat(((ConnectionClientTransport) client).getAttributes().get(REMOTE_UID))
304+
.isEqualTo(myUid());
305+
}
306+
267307
@Test
268308
@Ignore("See BinderTransportTest#socketStats.")
269309
@Override

0 commit comments

Comments
 (0)