Fix validation of system paths in installd.

System apps are now installed under their own directory
(system_app_dir/app_dir/app.apk). The new path doesn't pass installd
validation because of obsolete checks which verify that the path does
not contain subdirectories past the system_app_dir.

The CL fixes the validation to accept at most on subdirectory.

Bug: 17109858
Change-Id: I13abb52c0016610ff436f6a26bb6b3b85dc4dfb0

diff --git a/cmds/installd/tests/installd_utils_test.cpp b/cmds/installd/tests/installd_utils_test.cpp
index eeafa10..94e4792 100644 (file)
--- a/cmds/installd/tests/installd_utils_test.cpp
+++ b/cmds/installd/tests/installd_utils_test.cpp
@@ -118,6 +118,14 @@ TEST_F(UtilsTest, IsValidApkPath_Internal) {
     const char *bad_path3 = TEST_APP_DIR "example.com/subdir/pkg.apk";
     EXPECT_EQ(-1, validate_apk_path(bad_path3))
             << bad_path3 << " should be rejected as a invalid path";
+
+    const char *bad_path4 = TEST_APP_DIR "example.com/subdir/../pkg.apk";
+    EXPECT_EQ(-1, validate_apk_path(bad_path4))
+            << bad_path4 << " should be rejected as a invalid path";
+
+    const char *bad_path5 = TEST_APP_DIR "example.com1/../example.com2/pkg.apk";
+    EXPECT_EQ(-1, validate_apk_path(bad_path5))
+            << bad_path5 << " should be rejected as a invalid path";
 }
 
 TEST_F(UtilsTest, IsValidApkPath_Private) {
@@ -143,6 +151,14 @@ TEST_F(UtilsTest, IsValidApkPath_Private) {
     const char *bad_path3 = TEST_APP_PRIVATE_DIR "example.com/subdir/pkg.apk";
     EXPECT_EQ(-1, validate_apk_path(bad_path3))
             << bad_path3 << " should be rejected as a invalid path";
+
+    const char *bad_path4 = TEST_APP_PRIVATE_DIR "example.com/subdir/../pkg.apk";
+    EXPECT_EQ(-1, validate_apk_path(bad_path4))
+            << bad_path4 << " should be rejected as a invalid path";
+
+    const char *bad_path5 = TEST_APP_PRIVATE_DIR "example.com1/../example.com2/pkg.apk";
+    EXPECT_EQ(-1, validate_apk_path(bad_path5))
+            << bad_path5 << " should be rejected as a invalid path";
 }
 
 
@@ -230,6 +246,24 @@ TEST_F(UtilsTest, CheckSystemApp_BadPathEscapeFail) {
             << badapp3 << " should be rejected not a system path";
 }
 
+TEST_F(UtilsTest, CheckSystemApp_Subdir) {
+    const char *sysapp = TEST_SYSTEM_DIR1 "com.example/com.example.apk";
+    EXPECT_EQ(0, validate_system_app_path(sysapp))
+            << sysapp << " should be allowed as a system path";
+
+    const char *badapp = TEST_SYSTEM_DIR1 "com.example/subdir/com.example.apk";
+    EXPECT_EQ(-1, validate_system_app_path(badapp))
+            << badapp << " should be rejected not a system path";
+
+    const char *badapp1 = TEST_SYSTEM_DIR1 "com.example/subdir/../com.example.apk";
+    EXPECT_EQ(-1, validate_system_app_path(badapp1))
+            << badapp1 << " should be rejected not a system path";
+
+    const char *badapp2 = TEST_SYSTEM_DIR1 "com.example1/../com.example2/com.example.apk";
+    EXPECT_EQ(-1, validate_system_app_path(badapp2))
+            << badapp2 << " should be rejected not a system path";
+}
+
 TEST_F(UtilsTest, GetPathFromString_NullPathFail) {
     dir_rec_t test1;
     EXPECT_EQ(-1, get_path_from_string(&test1, (const char *) NULL))

diff --git a/cmds/installd/utils.c b/cmds/installd/utils.c
index def2fc7..8cd1168 100644 (file)
--- a/cmds/installd/utils.c
+++ b/cmds/installd/utils.c
@@ -808,6 +808,33 @@ void finish_cache_collection(cache_t* cache)
 }
 
 /**
+ * Validate that the path is valid in the context of the provided directory.
+ * The path is allowed to have at most one subdirectory and no indirections
+ * to top level directories (i.e. have "..").
+ */
+static int validate_path(const dir_rec_t* dir, const char* path) {
+    size_t dir_len = dir->len;
+    const char* subdir = strchr(path + dir_len, '/');
+
+    // Only allow the path to have at most one subdirectory.
+    if (subdir != NULL) {
+        ++subdir;
+        if (strchr(subdir, '/') != NULL) {
+            ALOGE("invalid apk path '%s' (subdir?)\n", path);
+            return -1;
+        }
+    }
+
+    // Directories can't have a period directly after the directory markers to prevent "..".
+    if ((path[dir_len] == '.') || ((subdir != NULL) && (*subdir == '.'))) {
+        ALOGE("invalid apk path '%s' (trickery)\n", path);
+        return -1;
+    }
+
+    return 0;
+}
+
+/**
  * Checks whether a path points to a system app (.apk file). Returns 0
  * if it is a system app or -1 if it is not.
  */
@@ -817,11 +844,7 @@ int validate_system_app_path(const char* path) {
     for (i = 0; i < android_system_dirs.count; i++) {
         const size_t dir_len = android_system_dirs.dirs[i].len;
         if (!strncmp(path, android_system_dirs.dirs[i].path, dir_len)) {
-            if (path[dir_len] == '.' || strchr(path + dir_len, '/') != NULL) {
-                ALOGE("invalid system apk path '%s' (trickery)\n", path);
-                return -1;
-            }
-            return 0;
+            return validate_path(android_system_dirs.dirs + i, path);
         }
     }
 
@@ -920,37 +943,20 @@ int copy_and_append(dir_rec_t* dst, const dir_rec_t* src, const char* suffix) {
  */
 int validate_apk_path(const char *path)
 {
-    size_t dir_len;
+    const dir_rec_t* dir = NULL;
 
     if (!strncmp(path, android_app_dir.path, android_app_dir.len)) {
-        dir_len = android_app_dir.len;
+        dir = &android_app_dir;
     } else if (!strncmp(path, android_app_private_dir.path, android_app_private_dir.len)) {
-        dir_len = android_app_private_dir.len;
+        dir = &android_app_private_dir;
     } else if (!strncmp(path, android_asec_dir.path, android_asec_dir.len)) {
-        dir_len = android_asec_dir.len;
+        dir = &android_asec_dir;
     } else {
         ALOGE("invalid apk path '%s' (bad prefix)\n", path);
         return -1;
     }
 
-    const char* subdir = strchr(path + dir_len, '/');
-
-    // Only allow the path to have at most one subdirectory.
-    if (subdir != NULL) {
-        ++subdir;
-        if (strchr(subdir, '/') != NULL) {
-            ALOGE("invalid apk path '%s' (subdir?)\n", path);
-            return -1;
-        }
-    }
-
-    // Directories can't have a period directly after the directory markers to prevent "..".
-    if ((path[dir_len] == '.') || ((subdir != NULL) && (*subdir == '.'))) {
-        ALOGE("invalid apk path '%s' (trickery)\n", path);
-        return -1;
-    }
-
-    return 0;
+    return validate_path(dir, path);
 }
 
 int append_and_increment(char** dst, const char* src, size_t* dst_size) {