From 98267b36b73a99514ff209a484ed7ba8cbbf5c3c Mon Sep 17 00:00:00 2001
From: Dan Sandler <dsandler@android.com>
Date: Thu, 13 Dec 2018 15:32:13 -0500
Subject: [PATCH] Harden against invalid paths.

Don't write them, and also, catch runtime exceptions during
persist operations.

Bug: 120749813
Test: atest frameworks/base/services/tests/uiservicestests/src/com/android/server/slice/SlicePermissionManagerTest.java
Change-Id: Ie478814abf5b44e48c8179bee79578e90f4fe3d4
(cherry picked from commit 2d8e3d17f6a2f38b83da6a1b05246dde4e77cb59)
---
 .../server/slice/SliceClientPermissions.java       |  9 ++++--
 .../server/slice/SlicePermissionManager.java       | 11 ++++++--
 .../server/slice/SlicePermissionManagerTest.java   | 32 +++++++++++++++++++++-
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/services/core/java/com/android/server/slice/SliceClientPermissions.java b/services/core/java/com/android/server/slice/SliceClientPermissions.java
index e461e0d43735..ab94a59c4d9c 100644
--- a/services/core/java/com/android/server/slice/SliceClientPermissions.java
+++ b/services/core/java/com/android/server/slice/SliceClientPermissions.java
@@ -282,9 +282,12 @@ public class SliceClientPermissions implements DirtyTracker, Persistable {
         public synchronized void writeTo(XmlSerializer out) throws IOException {
             final int N = mPaths.size();
             for (int i = 0; i < N; i++) {
-                out.startTag(NAMESPACE, TAG_PATH);
-                out.text(encodeSegments(mPaths.valueAt(i)));
-                out.endTag(NAMESPACE, TAG_PATH);
+                final String[] segments = mPaths.valueAt(i);
+                if (segments != null) {
+                    out.startTag(NAMESPACE, TAG_PATH);
+                    out.text(encodeSegments(segments));
+                    out.endTag(NAMESPACE, TAG_PATH);
+                }
             }
         }
 
diff --git a/services/core/java/com/android/server/slice/SlicePermissionManager.java b/services/core/java/com/android/server/slice/SlicePermissionManager.java
index 780bc962afba..315d5e39c94b 100644
--- a/services/core/java/com/android/server/slice/SlicePermissionManager.java
+++ b/services/core/java/com/android/server/slice/SlicePermissionManager.java
@@ -315,7 +315,8 @@ public class SlicePermissionManager implements DirtyTracker {
         return new AtomicFile(new File(mSliceDir, fileName));
     }
 
-    private void handlePersist() {
+    @VisibleForTesting
+    void handlePersist() {
         synchronized (this) {
             for (Persistable persistable : mDirty) {
                 AtomicFile file = getFile(persistable.getFileName());
@@ -335,7 +336,7 @@ public class SlicePermissionManager implements DirtyTracker {
 
                     out.flush();
                     file.finishWrite(stream);
-                } catch (IOException | XmlPullParserException e) {
+                } catch (IOException | XmlPullParserException | RuntimeException e) {
                     Slog.w(TAG, "Failed to save access file, restoring backup", e);
                     file.failWrite(stream);
                 }
@@ -344,6 +345,12 @@ public class SlicePermissionManager implements DirtyTracker {
         }
     }
 
+    // use addPersistableDirty(); this is just for tests
+    @VisibleForTesting
+    void addDirtyImmediate(Persistable obj) {
+        mDirty.add(obj);
+    }
+
     private void handleRemove(PkgUser pkgUser) {
         getFile(SliceClientPermissions.getFileName(pkgUser)).delete();
         getFile(SliceProviderPermissions.getFileName(pkgUser)).delete();
diff --git a/services/tests/uiservicestests/src/com/android/server/slice/SlicePermissionManagerTest.java b/services/tests/uiservicestests/src/com/android/server/slice/SlicePermissionManagerTest.java
index dc057d564a84..b315e514d6a6 100644
--- a/services/tests/uiservicestests/src/com/android/server/slice/SlicePermissionManagerTest.java
+++ b/services/tests/uiservicestests/src/com/android/server/slice/SlicePermissionManagerTest.java
@@ -101,4 +101,34 @@ public class SlicePermissionManagerTest extends UiServiceTestCase {
         assertTrue(FileUtils.deleteContentsAndDir(sliceDir));
     }
 
-}
\ No newline at end of file
+    @Test
+    public void testInvalid() throws Exception {
+        File sliceDir = new File(mContext.getCacheDir(), "slices-test");
+        if (!sliceDir.exists()) {
+            sliceDir.mkdir();
+        }
+        SlicePermissionManager permissions = new SlicePermissionManager(mContext,
+                TestableLooper.get(this).getLooper(), sliceDir);
+
+        DirtyTracker.Persistable junk = new DirtyTracker.Persistable() {
+            @Override
+            public String getFileName() {
+                return "invalidData";
+            }
+
+            @Override
+            public void writeTo(XmlSerializer out) throws IOException {
+                throw new RuntimeException("this doesn't work");
+            }
+        };
+
+        // let's put something bad in here
+        permissions.addDirtyImmediate(junk);
+        // force a persist. if this throws, it would take down system_server
+        permissions.handlePersist();
+
+        // Cleanup.
+        assertTrue(FileUtils.deleteContentsAndDir(sliceDir));
+    }
+
+}
-- 
2.11.0