commit d80a6ed9e283d4e5a577e606c39685507584aeea
author Chenbo Feng <fengc@google.com> Mon Jun 17 16:22:28 2019 -0700
committer junyulai <junyulai@google.com> Mon Apr 27 18:38:56 2020 +0800
tree 47e1aab9d57c7ffdfc15c194ff1da6ce610b7561
parent 5d0d638f540ae558e3fbe55b1470af84c6e5673d

Enforce permission check in getUidStats function

The NetworkStatsService.getUidStats() currently doesn't have any
permission check to make sure unpriviledged apps cannot read the stats
of a different uid. It will protentially have security problem since
apps with ACCESS_NETWORK_STATS permission can directly calling into
NetworkStatsService and bypass the check in TrafficStats. Move the uid
check from TrafficStats to NetworkStatsService to fix the problem.

Bug: 129151407
Test: atest AppSecurityTests#testAppFailAccessPrivateData_full
Test: atest AppSecurityTests#testAppFailAccessPrivateData_instant
Test: atest android.app.usage.cts.NetworkUsageStatsTest
Test: atest NetworkStatsBinderTest

Change-Id: Iae85676cfe5f114da69ec278afc2c904bc907234

core/java/android/net/TrafficStats.java

diff --git a/core/java/android/net/TrafficStats.java b/core/java/android/net/TrafficStats.java
index 8108cf0..e7bba69 100644
--- a/core/java/android/net/TrafficStats.java
+++ b/core/java/android/net/TrafficStats.java
@@ -775,17 +775,10 @@
      * @see android.content.pm.ApplicationInfo#uid
      */
     public static long getUidTxBytes(int uid) {
-        // This isn't actually enforcing any security; it just returns the
-        // unsupported value. The real filtering is done at the kernel level.
-        final int callingUid = android.os.Process.myUid();
-        if (callingUid == android.os.Process.SYSTEM_UID || callingUid == uid) {
-            try {
-                return getStatsService().getUidStats(uid, TYPE_TX_BYTES);
-            } catch (RemoteException e) {
-                throw e.rethrowFromSystemServer();
-            }
-        } else {
-            return UNSUPPORTED;
+        try {
+            return getStatsService().getUidStats(uid, TYPE_TX_BYTES);
+        } catch (RemoteException e) {
+            throw e.rethrowFromSystemServer();
         }
     }
 
@@ -808,17 +801,10 @@
      * @see android.content.pm.ApplicationInfo#uid
      */
     public static long getUidRxBytes(int uid) {
-        // This isn't actually enforcing any security; it just returns the
-        // unsupported value. The real filtering is done at the kernel level.
-        final int callingUid = android.os.Process.myUid();
-        if (callingUid == android.os.Process.SYSTEM_UID || callingUid == uid) {
-            try {
-                return getStatsService().getUidStats(uid, TYPE_RX_BYTES);
-            } catch (RemoteException e) {
-                throw e.rethrowFromSystemServer();
-            }
-        } else {
-            return UNSUPPORTED;
+        try {
+            return getStatsService().getUidStats(uid, TYPE_RX_BYTES);
+        } catch (RemoteException e) {
+            throw e.rethrowFromSystemServer();
         }
     }
 
@@ -841,17 +827,10 @@
      * @see android.content.pm.ApplicationInfo#uid
      */
     public static long getUidTxPackets(int uid) {
-        // This isn't actually enforcing any security; it just returns the
-        // unsupported value. The real filtering is done at the kernel level.
-        final int callingUid = android.os.Process.myUid();
-        if (callingUid == android.os.Process.SYSTEM_UID || callingUid == uid) {
-            try {
-                return getStatsService().getUidStats(uid, TYPE_TX_PACKETS);
-            } catch (RemoteException e) {
-                throw e.rethrowFromSystemServer();
-            }
-        } else {
-            return UNSUPPORTED;
+        try {
+            return getStatsService().getUidStats(uid, TYPE_TX_PACKETS);
+        } catch (RemoteException e) {
+            throw e.rethrowFromSystemServer();
         }
     }
 
@@ -874,17 +853,10 @@
      * @see android.content.pm.ApplicationInfo#uid
      */
     public static long getUidRxPackets(int uid) {
-        // This isn't actually enforcing any security; it just returns the
-        // unsupported value. The real filtering is done at the kernel level.
-        final int callingUid = android.os.Process.myUid();
-        if (callingUid == android.os.Process.SYSTEM_UID || callingUid == uid) {
-            try {
-                return getStatsService().getUidStats(uid, TYPE_RX_PACKETS);
-            } catch (RemoteException e) {
-                throw e.rethrowFromSystemServer();
-            }
-        } else {
-            return UNSUPPORTED;
+        try {
+            return getStatsService().getUidStats(uid, TYPE_RX_PACKETS);
+        } catch (RemoteException e) {
+            throw e.rethrowFromSystemServer();
         }
     }
 

services/core/java/com/android/server/net/NetworkStatsService.java

diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java
index d8264b3..003e7fc 100644
--- a/services/core/java/com/android/server/net/NetworkStatsService.java
+++ b/services/core/java/com/android/server/net/NetworkStatsService.java
@@ -49,6 +49,7 @@
 import static android.net.NetworkTemplate.getCollapsedRatType;
 import static android.net.TrafficStats.KB_IN_BYTES;
 import static android.net.TrafficStats.MB_IN_BYTES;
+import static android.net.TrafficStats.UNSUPPORTED;
 import static android.os.Trace.TRACE_TAG_NETWORK;
 import static android.provider.Settings.Global.NETSTATS_AUGMENT_ENABLED;
 import static android.provider.Settings.Global.NETSTATS_COMBINE_SUBTYPE_ENABLED;
@@ -1021,6 +1022,10 @@
 
     @Override
     public long getUidStats(int uid, int type) {
+        final int callingUid = Binder.getCallingUid();
+        if (callingUid != android.os.Process.SYSTEM_UID && callingUid != uid) {
+            return UNSUPPORTED;
+        }
         return nativeGetUidStat(uid, type, checkBpfStatsEnable());
     }