From: Alex Murray Date: Wed, 17 Nov 2021 03:57:22 +0000 (+1030) Subject: [PATCH 11/36] cmd/libsnap-confine-private: Defend against hardlink attacks X-Git-Tag: archive/raspbian/2.37.4-1+rpi1+deb10u1^2~5 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=8561877645ede04c86390acd8d83b88381669d17;p=snapd.git [PATCH 11/36] cmd/libsnap-confine-private: Defend against hardlink attacks When snap-confine goes to execute other helper binaries (snap-update-ns etc) via sc_open_snapd_tool(), these other binaries are located relative to the currently executing snap-confine process via /proc/self/exe. Since it is possible for regular users to hardlink setuid binaries when fs.protected_hardlinks is 0, it is possible to hardlink snap-confine to another location and then place an attacker controlled binary in place of snap-update-ns and have this executed as root by snap-confine. Protect against this by checking that snap-confine is located either within /usr/lib/snapd or within the core or snapd snaps as expected. This resolves CVE-2021-44730. Signed-off-by: Alex Murray Gbp-Pq: Topic cve202144730 Gbp-Pq: Name 0011-cmd-libsnap-confine-private-Defend-against-hardlink-.patch --- diff --git a/cmd/libsnap-confine-private/tool.c b/cmd/libsnap-confine-private/tool.c index 36666df9..9b5c7de3 100644 --- a/cmd/libsnap-confine-private/tool.c +++ b/cmd/libsnap-confine-private/tool.c @@ -154,14 +154,21 @@ static int sc_open_snapd_tool(const char *tool_name) // want to store the terminating '\0'. The readlink system call doesn't add // terminating null, but our initialization of buf handles this for us. char buf[PATH_MAX + 1] = { 0 }; - if (readlink("/proc/self/exe", buf, sizeof buf) < 0) { + if (readlink("/proc/self/exe", buf, sizeof(buf) - 1) < 0) { die("cannot readlink /proc/self/exe"); } if (buf[0] != '/') { // this shouldn't happen, but make sure have absolute path die("readlink /proc/self/exe returned relative path"); } + // as we are looking up other tools relative to our own path, check + // we are located where we think we should be - otherwise we + // may have been hardlink'd elsewhere and then may execute the + // wrong tool as a result + if (!sc_is_expected_path(buf)) { + die("running from unexpected location: %s", buf); + } char *dir_name = dirname(buf); - int dir_fd SC_CLEANUP(sc_cleanup_close) = 1; + int dir_fd SC_CLEANUP(sc_cleanup_close) = -1; dir_fd = open(dir_name, O_PATH | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); if (dir_fd < 0) { die("cannot open path %s", dir_name); diff --git a/cmd/libsnap-confine-private/utils-test.c b/cmd/libsnap-confine-private/utils-test.c index 83364801..a0ae441e 100644 --- a/cmd/libsnap-confine-private/utils-test.c +++ b/cmd/libsnap-confine-private/utils-test.c @@ -71,6 +71,37 @@ static void test_parse_bool(void) g_assert_cmpint(errno, ==, EFAULT); } +static void test_sc_is_expected_path(void) +{ + struct { + const char *path; + bool expected; + } test_cases[] = { + {"/tmp/snap-confine", false}, + {"/tmp/foo", false}, + {"/home/ ", false}, + {"/usr/lib/snapd/snap-confine1", false}, + {"/usr/lib/snapd/snap—confine", false}, + {"/snap/core/usr/lib/snapd/snap-confine", false}, + {"/snap/core/x1x/usr/lib/snapd/snap-confine", false}, + {"/snap/core/z1/usr/lib/snapd/snap-confine", false}, + {"/snap/cꓳre/1/usr/lib/snapd/snap-confine", false}, + {"/snap/snapd1/1/usr/lib/snapd/snap-confine", false}, + {"/snap/core/current/usr/lib/snapd/snap-confine", false}, + {"/usr/lib/snapd/snap-confine", true}, + {"/usr/libexec/snapd/snap-confine", true}, + {"/snap/core/1/usr/lib/snapd/snap-confine", true}, + {"/snap/core/x1/usr/lib/snapd/snap-confine", true}, + {"/snap/snapd/1/usr/lib/snapd/snap-confine", true}, + {"/snap/snapd/1/usr/libexec/snapd/snap-confine", false}, + }; + size_t i; + for (i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) { + bool result = sc_is_expected_path(test_cases[i].path); + g_assert_cmpint(result, ==, test_cases[i].expected); + } +} + static void test_die(void) { if (g_test_subprocess()) { @@ -194,6 +225,7 @@ static void test_sc_nonfatal_mkpath__absolute(void) static void __attribute__ ((constructor)) init(void) { g_test_add_func("/utils/parse_bool", test_parse_bool); + g_test_add_func("/utils/sc_is_expected_path", test_sc_is_expected_path); g_test_add_func("/utils/die", test_die); g_test_add_func("/utils/die_with_errno", test_die_with_errno); g_test_add_func("/utils/sc_nonfatal_mkpath/relative", diff --git a/cmd/libsnap-confine-private/utils.c b/cmd/libsnap-confine-private/utils.c index c0170eae..3dca9935 100644 --- a/cmd/libsnap-confine-private/utils.c +++ b/cmd/libsnap-confine-private/utils.c @@ -16,6 +16,7 @@ */ #include #include +#include #include #include #include @@ -217,3 +218,15 @@ int sc_nonfatal_mkpath(const char *const path, mode_t mode) } return 0; } + +bool sc_is_expected_path(const char *path) +{ + const char *expected_path_re = + "^(/snap/(snapd|core)/x?[0-9]+/usr/lib|/usr/lib(exec)?)/snapd/snap-confine$"; + regex_t re; + if (regcomp(&re, expected_path_re, REG_EXTENDED | REG_NOSUB) != 0) + die("can not compile regex %s", expected_path_re); + int status = regexec(&re, path, 0, NULL, 0); + regfree(&re); + return status == 0; +} diff --git a/cmd/libsnap-confine-private/utils.h b/cmd/libsnap-confine-private/utils.h index b544f6cf..004f8db0 100644 --- a/cmd/libsnap-confine-private/utils.h +++ b/cmd/libsnap-confine-private/utils.h @@ -60,4 +60,10 @@ void write_string_to_file(const char *filepath, const char *buf); **/ __attribute__ ((warn_unused_result)) int sc_nonfatal_mkpath(const char *const path, mode_t mode); + +/** + * Return true if path is a valid path for the snap-confine binary + **/ +__attribute__((warn_unused_result)) +bool sc_is_expected_path(const char *path); #endif