From c597b6dd895dbb2b28c757ce7a2651b3cdc9b00c Mon Sep 17 00:00:00 2001 From: Calin Juravle Date: Tue, 19 Aug 2014 17:43:05 +0100 Subject: [PATCH] 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 --- cmds/installd/tests/installd_utils_test.cpp | 34 ++++++++++++ cmds/installd/utils.c | 60 +++++++++++---------- 2 files changed, 67 insertions(+), 27 deletions(-) diff --git a/cmds/installd/tests/installd_utils_test.cpp b/cmds/installd/tests/installd_utils_test.cpp index eeafa1043..94e479282 100644 --- 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 def2fc7fe..8cd116874 100644 --- a/cmds/installd/utils.c +++ b/cmds/installd/utils.c @@ -807,6 +807,33 @@ void finish_cache_collection(cache_t* cache) free(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) {