diff --git a/core/java/android/net/vcn/VcnTransportInfo.java b/core/java/android/net/vcn/VcnTransportInfo.java index 0e9ccf144c2ea..1f1818420b561 100644 --- a/core/java/android/net/vcn/VcnTransportInfo.java +++ b/core/java/android/net/vcn/VcnTransportInfo.java @@ -16,23 +16,17 @@ package android.net.vcn; -import static android.net.NetworkCapabilities.REDACT_ALL; -import static android.net.NetworkCapabilities.REDACT_FOR_NETWORK_SETTINGS; +import static android.net.NetworkCapabilities.REDACT_NONE; import static android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID; -import static com.android.internal.annotations.VisibleForTesting.Visibility.PRIVATE; - import android.annotation.NonNull; import android.annotation.Nullable; -import android.net.NetworkCapabilities; import android.net.TransportInfo; import android.net.wifi.WifiInfo; import android.os.Parcel; import android.os.Parcelable; import android.telephony.SubscriptionManager; -import com.android.internal.annotations.VisibleForTesting; - import java.util.Objects; /** @@ -55,32 +49,17 @@ public class VcnTransportInfo implements TransportInfo, Parcelable { @Nullable private final WifiInfo mWifiInfo; private final int mSubId; - /** - * The redaction scheme to use when parcelling. - * - *

The TransportInfo/NetworkCapabilities redaction mechanisms rely on redaction being - * performed at parcelling time. This means that the redaction scheme must be stored for later - * use. - * - *

Since the redaction scheme itself is not parcelled, this field is listed as a transient. - * - *

Defaults to REDACT_ALL when constructed using public constructors, or creating from - * parcels. - */ - private final transient long mRedactions; - public VcnTransportInfo(@NonNull WifiInfo wifiInfo) { - this(wifiInfo, INVALID_SUBSCRIPTION_ID, REDACT_ALL); + this(wifiInfo, INVALID_SUBSCRIPTION_ID); } public VcnTransportInfo(int subId) { - this(null /* wifiInfo */, subId, REDACT_ALL); + this(null /* wifiInfo */, subId); } - private VcnTransportInfo(@Nullable WifiInfo wifiInfo, int subId, long redactions) { + private VcnTransportInfo(@Nullable WifiInfo wifiInfo, int subId) { mWifiInfo = wifiInfo; mSubId = subId; - mRedactions = redactions; } /** @@ -102,25 +81,14 @@ public class VcnTransportInfo implements TransportInfo, Parcelable { * SubscriptionManager#INVALID_SUBSCRIPTION_ID}. * * @return the Subscription ID if a cellular underlying Network is present, else {@link - * android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID}. + * android.telephony.SubscriptionManager#INVALID_SUBSCRIPTION_ID}. */ public int getSubId() { return mSubId; } - /** - * Gets the redaction scheme - * - * @hide - */ - @VisibleForTesting(visibility = PRIVATE) - public long getRedaction() { - return mRedactions; - } - @Override public int hashCode() { - // mRedactions not hashed, as it is a transient, for control of parcelling return Objects.hash(mWifiInfo, mSubId); } @@ -128,8 +96,6 @@ public class VcnTransportInfo implements TransportInfo, Parcelable { public boolean equals(Object o) { if (!(o instanceof VcnTransportInfo)) return false; final VcnTransportInfo that = (VcnTransportInfo) o; - - // mRedactions not compared, as it is a transient, for control of parcelling return Objects.equals(mWifiInfo, that.mWifiInfo) && mSubId == that.mSubId; } @@ -143,31 +109,19 @@ public class VcnTransportInfo implements TransportInfo, Parcelable { @NonNull public TransportInfo makeCopy(long redactions) { return new VcnTransportInfo( - mWifiInfo == null ? null : mWifiInfo.makeCopy(redactions), mSubId, redactions); + (mWifiInfo == null) ? null : mWifiInfo.makeCopy(redactions), mSubId); } @Override public long getApplicableRedactions() { - long redactions = REDACT_FOR_NETWORK_SETTINGS; - - // Add additional wifi redactions if necessary - if (mWifiInfo != null) { - redactions |= mWifiInfo.getApplicableRedactions(); - } - - return redactions; - } - - private boolean shouldParcelNetworkSettingsFields() { - return (mRedactions & NetworkCapabilities.REDACT_FOR_NETWORK_SETTINGS) == 0; + return (mWifiInfo == null) ? REDACT_NONE : mWifiInfo.getApplicableRedactions(); } /** {@inheritDoc} */ @Override public void writeToParcel(@NonNull Parcel dest, int flags) { - dest.writeInt(shouldParcelNetworkSettingsFields() ? mSubId : INVALID_SUBSCRIPTION_ID); - dest.writeParcelable( - shouldParcelNetworkSettingsFields() ? (Parcelable) mWifiInfo : null, flags); + dest.writeInt(mSubId); + dest.writeParcelable(mWifiInfo, flags); } @Override @@ -181,17 +135,7 @@ public class VcnTransportInfo implements TransportInfo, Parcelable { public VcnTransportInfo createFromParcel(Parcel in) { final int subId = in.readInt(); final WifiInfo wifiInfo = in.readParcelable(null); - - // If all fields are their null values, return null TransportInfo to avoid - // leaking information about this being a VCN Network (instead of macro - // cellular, etc) - if (wifiInfo == null && subId == INVALID_SUBSCRIPTION_ID) { - return null; - } - - // Prevent further forwarding by redacting everything in future parcels from - // this VcnTransportInfo - return new VcnTransportInfo(wifiInfo, subId, REDACT_ALL); + return new VcnTransportInfo(wifiInfo, subId); } public VcnTransportInfo[] newArray(int size) { diff --git a/packages/Connectivity/framework/src/android/net/NetworkCapabilities.java b/packages/Connectivity/framework/src/android/net/NetworkCapabilities.java index 4a99d290f38c1..c19a906ecd70e 100644 --- a/packages/Connectivity/framework/src/android/net/NetworkCapabilities.java +++ b/packages/Connectivity/framework/src/android/net/NetworkCapabilities.java @@ -139,19 +139,13 @@ public final class NetworkCapabilities implements Parcelable { */ private String mRequestorPackageName; - /** - * Indicates what fields should be redacted from this instance. - */ - private final @RedactionType long mRedactions; - public NetworkCapabilities() { - mRedactions = REDACT_ALL; clearAll(); mNetworkCapabilities = DEFAULT_CAPABILITIES; } public NetworkCapabilities(NetworkCapabilities nc) { - this(nc, REDACT_ALL); + this(nc, REDACT_NONE); } /** @@ -163,10 +157,12 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public NetworkCapabilities(@Nullable NetworkCapabilities nc, @RedactionType long redactions) { - mRedactions = redactions; if (nc != null) { set(nc); } + if (mTransportInfo != null) { + mTransportInfo = nc.mTransportInfo.makeCopy(redactions); + } } /** @@ -175,14 +171,6 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public void clearAll() { - // Ensures that the internal copies maintained by the connectivity stack does not set it to - // anything other than |REDACT_ALL|. - if (mRedactions != REDACT_ALL) { - // This is needed because the current redaction mechanism relies on redaction while - // parceling. - throw new UnsupportedOperationException( - "Cannot clear NetworkCapabilities when mRedactions is set"); - } mNetworkCapabilities = mTransportTypes = mForbiddenNetworkCapabilities = 0; mLinkUpBandwidthKbps = mLinkDownBandwidthKbps = LINK_BANDWIDTH_UNSPECIFIED; mNetworkSpecifier = null; @@ -211,7 +199,7 @@ public final class NetworkCapabilities implements Parcelable { mLinkDownBandwidthKbps = nc.mLinkDownBandwidthKbps; mNetworkSpecifier = nc.mNetworkSpecifier; if (nc.getTransportInfo() != null) { - setTransportInfo(nc.getTransportInfo().makeCopy(mRedactions)); + setTransportInfo(nc.getTransportInfo()); } else { setTransportInfo(null); } diff --git a/packages/Connectivity/tests/common/java/android/net/NetworkCapabilitiesTest.java b/packages/Connectivity/tests/common/java/android/net/NetworkCapabilitiesTest.java index b178bad7123d8..d74b802c87291 100644 --- a/packages/Connectivity/tests/common/java/android/net/NetworkCapabilitiesTest.java +++ b/packages/Connectivity/tests/common/java/android/net/NetworkCapabilitiesTest.java @@ -340,7 +340,7 @@ public class NetworkCapabilitiesTest { private void testParcelSane(NetworkCapabilities cap) { if (isAtLeastS()) { - assertParcelSane(cap, 17); + assertParcelSane(cap, 16); } else if (isAtLeastR()) { assertParcelSane(cap, 15); } else { diff --git a/packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java index ab5079820611d..221cc9b26ae46 100644 --- a/packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -19,6 +19,7 @@ package com.android.server; import static android.Manifest.permission.CHANGE_NETWORK_STATE; import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS; import static android.Manifest.permission.DUMP; +import static android.Manifest.permission.LOCAL_MAC_ADDRESS; import static android.Manifest.permission.NETWORK_FACTORY; import static android.Manifest.permission.NETWORK_SETTINGS; import static android.app.PendingIntent.FLAG_IMMUTABLE; @@ -9407,9 +9408,9 @@ public class ConnectivityServiceTest { @Override public TransportInfo makeCopy(@NetworkCapabilities.RedactionType long redactions) { return new TestTransportInfo( - (redactions & REDACT_FOR_ACCESS_FINE_LOCATION) != 0, - (redactions & REDACT_FOR_LOCAL_MAC_ADDRESS) != 0, - (redactions & REDACT_FOR_NETWORK_SETTINGS) != 0 + locationRedacted | (redactions & REDACT_FOR_ACCESS_FINE_LOCATION) != 0, + localMacAddressRedacted | (redactions & REDACT_FOR_LOCAL_MAC_ADDRESS) != 0, + settingsRedacted | (redactions & REDACT_FOR_NETWORK_SETTINGS) != 0 ); } @@ -9432,8 +9433,26 @@ public class ConnectivityServiceTest { public int hashCode() { return Objects.hash(locationRedacted, localMacAddressRedacted, settingsRedacted); } + + @Override + public String toString() { + return String.format( + "TestTransportInfo{locationRedacted=%s macRedacted=%s settingsRedacted=%s}", + locationRedacted, localMacAddressRedacted, settingsRedacted); + } } + private TestTransportInfo getTestTransportInfo(NetworkCapabilities nc) { + return (TestTransportInfo) nc.getTransportInfo(); + } + + private TestTransportInfo getTestTransportInfo(TestNetworkAgentWrapper n) { + final NetworkCapabilities nc = mCm.getNetworkCapabilities(n.getNetwork()); + assertNotNull(nc); + return getTestTransportInfo(nc); + } + + private void verifyNetworkCallbackLocationDataInclusionUsingTransportInfoAndOwnerUidInNetCaps( @NonNull TestNetworkCallback wifiNetworkCallback, int actualOwnerUid, @NonNull TransportInfo actualTransportInfo, int expectedOwnerUid, @@ -9462,7 +9481,6 @@ public class ConnectivityServiceTest { wifiNetworkCallback.expectCapabilitiesThat(mWiFiNetworkAgent, nc -> Objects.equals(expectedOwnerUid, nc.getOwnerUid()) && Objects.equals(expectedTransportInfo, nc.getTransportInfo())); - } @Test @@ -9483,6 +9501,40 @@ public class ConnectivityServiceTest { wifiNetworkCallack, ownerUid, transportInfo, INVALID_UID, sanitizedTransportInfo); } + @Test + public void testTransportInfoRedactionInSynchronousCalls() throws Exception { + final NetworkCapabilities ncTemplate = new NetworkCapabilities() + .addTransportType(TRANSPORT_WIFI) + .setTransportInfo(new TestTransportInfo()); + + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, new LinkProperties(), + ncTemplate); + mWiFiNetworkAgent.connect(true /* validated; waits for callback */); + + // NETWORK_SETTINGS redaction is controlled by the NETWORK_SETTINGS permission + assertTrue(getTestTransportInfo(mWiFiNetworkAgent).settingsRedacted); + withPermission(NETWORK_SETTINGS, () -> { + assertFalse(getTestTransportInfo(mWiFiNetworkAgent).settingsRedacted); + }); + assertTrue(getTestTransportInfo(mWiFiNetworkAgent).settingsRedacted); + + // LOCAL_MAC_ADDRESS redaction is controlled by the LOCAL_MAC_ADDRESS permission + assertTrue(getTestTransportInfo(mWiFiNetworkAgent).localMacAddressRedacted); + withPermission(LOCAL_MAC_ADDRESS, () -> { + assertFalse(getTestTransportInfo(mWiFiNetworkAgent).localMacAddressRedacted); + }); + assertTrue(getTestTransportInfo(mWiFiNetworkAgent).localMacAddressRedacted); + + // Synchronous getNetworkCapabilities calls never return unredacted location-sensitive + // information. + assertTrue(getTestTransportInfo(mWiFiNetworkAgent).locationRedacted); + setupLocationPermissions(Build.VERSION_CODES.S, true, AppOpsManager.OPSTR_FINE_LOCATION, + Manifest.permission.ACCESS_FINE_LOCATION); + assertTrue(getTestTransportInfo(mWiFiNetworkAgent).locationRedacted); + denyAllLocationPrivilegedPermissions(); + assertTrue(getTestTransportInfo(mWiFiNetworkAgent).locationRedacted); + } + private void setupConnectionOwnerUid(int vpnOwnerUid, @VpnManager.VpnType int vpnType) throws Exception { final Set vpnRange = Collections.singleton(PRIMARY_UIDRANGE); @@ -9840,12 +9892,27 @@ public class ConnectivityServiceTest { // Connect the cell agent verify that it notifies TestNetworkCallback that it is available final TestNetworkCallback callback = new TestNetworkCallback(); mCm.registerDefaultNetworkCallback(callback); - mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); + + final NetworkCapabilities ncTemplate = new NetworkCapabilities() + .addTransportType(TRANSPORT_CELLULAR) + .setTransportInfo(new TestTransportInfo()); + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, new LinkProperties(), + ncTemplate); mCellNetworkAgent.connect(true); callback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); callback.assertNoCallback(); } + private boolean areConnDiagCapsRedacted(NetworkCapabilities nc) { + TestTransportInfo ti = (TestTransportInfo) nc.getTransportInfo(); + return nc.getUids() == null + && nc.getAdministratorUids().length == 0 + && nc.getOwnerUid() == Process.INVALID_UID + && getTestTransportInfo(nc).locationRedacted + && getTestTransportInfo(nc).localMacAddressRedacted + && getTestTransportInfo(nc).settingsRedacted; + } + @Test public void testConnectivityDiagnosticsCallbackOnConnectivityReportAvailable() throws Exception { @@ -9856,12 +9923,7 @@ public class ConnectivityServiceTest { // Verify onConnectivityReport fired verify(mConnectivityDiagnosticsCallback).onConnectivityReportAvailable( - argThat(report -> { - final NetworkCapabilities nc = report.getNetworkCapabilities(); - return nc.getUids() == null - && nc.getAdministratorUids().length == 0 - && nc.getOwnerUid() == Process.INVALID_UID; - })); + argThat(report -> areConnDiagCapsRedacted(report.getNetworkCapabilities()))); } @Test @@ -9877,12 +9939,7 @@ public class ConnectivityServiceTest { // Verify onDataStallSuspected fired verify(mConnectivityDiagnosticsCallback).onDataStallSuspected( - argThat(report -> { - final NetworkCapabilities nc = report.getNetworkCapabilities(); - return nc.getUids() == null - && nc.getAdministratorUids().length == 0 - && nc.getOwnerUid() == Process.INVALID_UID; - })); + argThat(report -> areConnDiagCapsRedacted(report.getNetworkCapabilities()))); } @Test diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 39a990cf5d730..ac97fe18c8c31 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -9041,7 +9041,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } private NetworkCapabilities getNetworkCapabilitiesWithoutUids(@NonNull NetworkCapabilities nc) { - final NetworkCapabilities sanitized = new NetworkCapabilities(nc); + final NetworkCapabilities sanitized = new NetworkCapabilities(nc, + NetworkCapabilities.REDACT_ALL); sanitized.setUids(null); sanitized.setAdministratorUids(new int[0]); sanitized.setOwnerUid(Process.INVALID_UID); diff --git a/tests/vcn/java/android/net/vcn/VcnTransportInfoTest.java b/tests/vcn/java/android/net/vcn/VcnTransportInfoTest.java index 582275d0547d6..abae81cf17429 100644 --- a/tests/vcn/java/android/net/vcn/VcnTransportInfoTest.java +++ b/tests/vcn/java/android/net/vcn/VcnTransportInfoTest.java @@ -16,15 +16,17 @@ package android.net.vcn; -import static android.net.NetworkCapabilities.REDACT_ALL; -import static android.net.NetworkCapabilities.REDACT_FOR_NETWORK_SETTINGS; +import static android.net.NetworkCapabilities.REDACT_FOR_ACCESS_FINE_LOCATION; +import static android.net.NetworkCapabilities.REDACT_NONE; import static android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; +import android.net.wifi.WifiConfiguration; import android.net.wifi.WifiInfo; +import android.os.Build; import android.os.Parcel; import org.junit.Test; @@ -38,12 +40,6 @@ public class VcnTransportInfoTest { private static final VcnTransportInfo CELL_UNDERLYING_INFO = new VcnTransportInfo(SUB_ID); private static final VcnTransportInfo WIFI_UNDERLYING_INFO = new VcnTransportInfo(WIFI_INFO); - @Test - public void testRedactionDefaults() { - assertEquals(REDACT_ALL, CELL_UNDERLYING_INFO.getRedaction()); - assertEquals(REDACT_ALL, WIFI_UNDERLYING_INFO.getRedaction()); - } - @Test public void testGetWifiInfo() { assertEquals(WIFI_INFO, WIFI_UNDERLYING_INFO.getWifiInfo()); @@ -59,15 +55,19 @@ public class VcnTransportInfoTest { } @Test - public void testMakeCopySetsRedactions() { + public void testMakeCopyRedactForAccessFineLocation() { assertEquals( - REDACT_FOR_NETWORK_SETTINGS, - ((VcnTransportInfo) CELL_UNDERLYING_INFO.makeCopy(REDACT_FOR_NETWORK_SETTINGS)) - .getRedaction()); - assertEquals( - REDACT_FOR_NETWORK_SETTINGS, - ((VcnTransportInfo) WIFI_UNDERLYING_INFO.makeCopy(REDACT_FOR_NETWORK_SETTINGS)) - .getRedaction()); + SUB_ID, + ((VcnTransportInfo) CELL_UNDERLYING_INFO.makeCopy(REDACT_FOR_ACCESS_FINE_LOCATION)) + .getSubId()); + + // TODO: remove the if statement when S pushes to AOSP. + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { + assertEquals( + WifiConfiguration.INVALID_NETWORK_ID, + ((VcnTransportInfo) WIFI_UNDERLYING_INFO.makeCopy( + REDACT_FOR_ACCESS_FINE_LOCATION)).getWifiInfo().getNetworkId()); + } } @Test @@ -78,35 +78,31 @@ public class VcnTransportInfoTest { } @Test - public void testParcelUnparcel() { - verifyParcelingIsNull(CELL_UNDERLYING_INFO); - verifyParcelingIsNull(WIFI_UNDERLYING_INFO); - } + public void testApplicableRedactions() { + assertEquals(REDACT_NONE, CELL_UNDERLYING_INFO.getApplicableRedactions()); - private void verifyParcelingIsNull(VcnTransportInfo vcnTransportInfo) { - // Verify redacted by default - Parcel parcel = Parcel.obtain(); - vcnTransportInfo.writeToParcel(parcel, 0 /* flags */); - parcel.setDataPosition(0); - - assertNull(VcnTransportInfo.CREATOR.createFromParcel(parcel)); + final long wifiRedactions = WIFI_INFO.getApplicableRedactions(); + assertEquals(wifiRedactions, WIFI_UNDERLYING_INFO.getApplicableRedactions()); } @Test - public void testParcelUnparcelNotRedactedForSysUi() { - verifyParcelingForSysUi(CELL_UNDERLYING_INFO); - verifyParcelingForSysUi(WIFI_UNDERLYING_INFO); + public void testParcelNotRedactedForSysUi() { + VcnTransportInfo cellRedacted = parcelForSysUi(CELL_UNDERLYING_INFO); + assertEquals(SUB_ID, cellRedacted.getSubId()); + VcnTransportInfo wifiRedacted = parcelForSysUi(WIFI_UNDERLYING_INFO); + assertEquals(NETWORK_ID, wifiRedacted.getWifiInfo().getNetworkId()); } - private void verifyParcelingForSysUi(VcnTransportInfo vcnTransportInfo) { + private VcnTransportInfo parcelForSysUi(VcnTransportInfo vcnTransportInfo) { // Allow fully unredacted; SysUI will have all the relevant permissions. - final VcnTransportInfo unRedacted = (VcnTransportInfo) vcnTransportInfo.makeCopy(0); + final VcnTransportInfo unRedacted = (VcnTransportInfo) vcnTransportInfo.makeCopy( + REDACT_NONE); final Parcel parcel = Parcel.obtain(); unRedacted.writeToParcel(parcel, 0 /* flags */); parcel.setDataPosition(0); final VcnTransportInfo unparceled = VcnTransportInfo.CREATOR.createFromParcel(parcel); assertEquals(vcnTransportInfo, unparceled); - assertEquals(REDACT_ALL, unparceled.getRedaction()); + return unparceled; } }