From 8ac1143594397394ff84afe3e9abcdef5da3feac Mon Sep 17 00:00:00 2001 From: Aaron Huang Date: Sat, 4 Feb 2023 16:07:05 +0800 Subject: [PATCH] Remove requireNonNull check from matchesWifiNetworkKey matchesWifiNetworkKey expects a non-null value of the wifiNetworkKey, however, it might be null in practice and the null wifiNetworkKey will be stored into disk. And then when the matchesWifiNetworkKey is called, the requireNonNull will crash the system. Thus, remove the requireNonNull from matchesWifiNetworkKey to avoid system crash and handle if the wifiNetworkKey is null then it should not match a template with non-empty mMatchWifiNetworkKeys. Check if WifiInfo contains a null network key then skip it to prevent adding the identity to the network identity set. Also, add a Log.wtf when setWifiNetworkKey(info.getNetworkKey()), this might be useful to catch why the wifiNetworkKey is null. Bug: 267815242 Bug: 266598304 Test: FrmeworksNetTests Change-Id: I9c21f7e3dca9482133c7e331741cf808105414e9 --- .../src/android/net/NetworkIdentity.java | 13 +++++++++-- .../src/android/net/NetworkTemplate.java | 10 ++++++++- .../server/net/NetworkStatsService.java | 10 +++++++++ .../java/android/net/NetworkTemplateTest.kt | 22 +++++++++++++++++++ 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/framework-t/src/android/net/NetworkIdentity.java b/framework-t/src/android/net/NetworkIdentity.java index edfd21cedf..947a0925c6 100644 --- a/framework-t/src/android/net/NetworkIdentity.java +++ b/framework-t/src/android/net/NetworkIdentity.java @@ -32,6 +32,7 @@ import android.content.Context; import android.net.wifi.WifiInfo; import android.service.NetworkIdentityProto; import android.telephony.TelephonyManager; +import android.util.Log; import android.util.proto.ProtoOutputStream; import com.android.net.module.util.BitUtils; @@ -406,10 +407,18 @@ public class NetworkIdentity { setOemManaged(getOemBitfield(snapshot.getNetworkCapabilities())); if (mType == TYPE_WIFI) { - final TransportInfo transportInfo = snapshot.getNetworkCapabilities() - .getTransportInfo(); + final NetworkCapabilities nc = snapshot.getNetworkCapabilities(); + final TransportInfo transportInfo = nc.getTransportInfo(); if (transportInfo instanceof WifiInfo) { final WifiInfo info = (WifiInfo) transportInfo; + // Log.wtf to catch trying to set a null wifiNetworkKey into NetworkIdentity. + // See b/266598304. The problematic data that has null wifi network key is + // thrown out when storing data, which is handled by the service. + if (info.getNetworkKey() == null) { + Log.wtf(TAG, "WifiInfo contains a null wifiNetworkKey and it will" + + " be set into NetworkIdentity, netId=" + snapshot.getNetwork() + + "NetworkCapabilities=" + nc); + } setWifiNetworkKey(info.getNetworkKey()); } } else if (mType == TYPE_TEST) { diff --git a/framework-t/src/android/net/NetworkTemplate.java b/framework-t/src/android/net/NetworkTemplate.java index 55fcc4ae4d..d90bd8d218 100644 --- a/framework-t/src/android/net/NetworkTemplate.java +++ b/framework-t/src/android/net/NetworkTemplate.java @@ -700,7 +700,15 @@ public final class NetworkTemplate implements Parcelable { * to know details about the key. */ private boolean matchesWifiNetworkKey(@NonNull String wifiNetworkKey) { - Objects.requireNonNull(wifiNetworkKey); + // Note that this code accepts null wifi network keys because of a past bug where wifi + // code was sending a null network key for some connected networks, which isn't expected + // and ended up stored in the data on many devices. + // A null network key in the data matches a wildcard template (one where + // {@code mMatchWifiNetworkKeys} is empty), but not one where {@code MatchWifiNetworkKeys} + // contains null. See b/266598304. + if (wifiNetworkKey == null) { + return CollectionUtils.isEmpty(mMatchWifiNetworkKeys); + } return CollectionUtils.isEmpty(mMatchWifiNetworkKeys) || CollectionUtils.contains(mMatchWifiNetworkKeys, wifiNetworkKey); } diff --git a/service-t/src/com/android/server/net/NetworkStatsService.java b/service-t/src/com/android/server/net/NetworkStatsService.java index 9176ec226d..ee7044a6bf 100644 --- a/service-t/src/com/android/server/net/NetworkStatsService.java +++ b/service-t/src/com/android/server/net/NetworkStatsService.java @@ -106,6 +106,7 @@ import android.net.TelephonyNetworkSpecifier; import android.net.TetherStatsParcel; import android.net.TetheringManager; import android.net.TrafficStats; +import android.net.TransportInfo; import android.net.UnderlyingNetworkInfo; import android.net.Uri; import android.net.netstats.IUsageCallback; @@ -113,6 +114,7 @@ import android.net.netstats.NetworkStatsDataMigrationUtils; import android.net.netstats.provider.INetworkStatsProvider; import android.net.netstats.provider.INetworkStatsProviderCallback; import android.net.netstats.provider.NetworkStatsProvider; +import android.net.wifi.WifiInfo; import android.os.Binder; import android.os.Build; import android.os.Bundle; @@ -2125,6 +2127,14 @@ public class NetworkStatsService extends INetworkStatsService.Stub { final NetworkIdentity ident = NetworkIdentity.buildNetworkIdentity(mContext, snapshot, isDefault, ratType); + // If WifiInfo contains a null network key then this identity should not be added into + // the network identity set. See b/266598304. + final TransportInfo transportInfo = snapshot.getNetworkCapabilities() + .getTransportInfo(); + if (transportInfo instanceof WifiInfo) { + final WifiInfo info = (WifiInfo) transportInfo; + if (info.getNetworkKey() == null) continue; + } // Traffic occurring on the base interface is always counted for // both total usage and UID details. final String baseIface = snapshot.getLinkProperties().getInterfaceName(); diff --git a/tests/unit/java/android/net/NetworkTemplateTest.kt b/tests/unit/java/android/net/NetworkTemplateTest.kt index edbcea9f35..fc25fd8524 100644 --- a/tests/unit/java/android/net/NetworkTemplateTest.kt +++ b/tests/unit/java/android/net/NetworkTemplateTest.kt @@ -130,10 +130,17 @@ class NetworkTemplateTest { mockContext, buildWifiNetworkState(null, TEST_WIFI_KEY1), true, 0) val identWifiImsi1Key1 = buildNetworkIdentity( mockContext, buildWifiNetworkState(TEST_IMSI1, TEST_WIFI_KEY1), true, 0) + // This identity with a null wifiNetworkKey is to test matchesWifiNetworkKey won't crash + // the system when a null wifiNetworkKey is provided, which happens because of a bug in wifi + // and it should still match the wifi wildcard template. See b/266598304. + val identWifiNullKey = buildNetworkIdentity( + mockContext, buildWifiNetworkState(null /* subscriberId */, + null /* wifiNetworkKey */), true, 0) templateWifiWildcard.assertDoesNotMatch(identMobileImsi1) templateWifiWildcard.assertMatches(identWifiImsiNullKey1) templateWifiWildcard.assertMatches(identWifiImsi1Key1) + templateWifiWildcard.assertMatches(identWifiNullKey) } @Test @@ -148,6 +155,9 @@ class NetworkTemplateTest { .setWifiNetworkKeys(setOf(TEST_WIFI_KEY1)).build() val templateWifiKeyAllImsi1 = NetworkTemplate.Builder(MATCH_WIFI) .setSubscriberIds(setOf(TEST_IMSI1)).build() + val templateNullWifiKey = NetworkTemplate(MATCH_WIFI, + emptyArray() /* subscriberIds */, arrayOf(null) /* wifiNetworkKeys */, + METERED_ALL, ROAMING_ALL, DEFAULT_NETWORK_ALL, NETWORK_TYPE_ALL, OEM_MANAGED_ALL) val identMobile1 = buildNetworkIdentity(mockContext, buildMobileNetworkState(TEST_IMSI1), false, TelephonyManager.NETWORK_TYPE_UMTS) @@ -159,6 +169,12 @@ class NetworkTemplateTest { mockContext, buildWifiNetworkState(TEST_IMSI2, TEST_WIFI_KEY1), true, 0) val identWifiImsi1Key2 = buildNetworkIdentity( mockContext, buildWifiNetworkState(TEST_IMSI1, TEST_WIFI_KEY2), true, 0) + // This identity with a null wifiNetworkKey is to test the matchesWifiNetworkKey won't crash + // the system when a null wifiNetworkKey is provided, which would happen in some unknown + // cases, see b/266598304. + val identWifiNullKey = buildNetworkIdentity( + mockContext, buildWifiNetworkState(null /* subscriberId */, + null /* wifiNetworkKey */), true, 0) // Verify that template with WiFi Network Key only matches any subscriberId and // specific WiFi Network Key. @@ -191,6 +207,12 @@ class NetworkTemplateTest { templateWifiKeyAllImsi1.assertMatches(identWifiImsi1Key1) templateWifiKeyAllImsi1.assertDoesNotMatch(identWifiImsi2Key1) templateWifiKeyAllImsi1.assertMatches(identWifiImsi1Key2) + + // Test a network identity with null wifiNetworkKey won't crash. + // It should not match a template with wifiNetworkKeys is non-null. + // Also, it should not match a template with wifiNetworkKeys that contains null. + templateWifiKey1.assertDoesNotMatch(identWifiNullKey) + templateNullWifiKey.assertDoesNotMatch(identWifiNullKey) } @Test