[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 <alex.murray@canonical.com>
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 36666df919f17be92f8d96efd77b900fbd1d6bfc..9b5c7de39cd8357aff47cd41d83c8cfd78ad03a0 100644 (file)
--- 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 83364801e9b26505c11d42317d56fe102cf31c67..a0ae441eef4784fbbb381a31a24265dda5b5a3bf 100644 (file)
--- 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 c0170eae73b3e465371ad732b25cc3304e67ebc0..3dca9935d963c6d97eb6c2051d8392b245c33975 100644 (file)
--- a/cmd/libsnap-confine-private/utils.c
+++ b/cmd/libsnap-confine-private/utils.c
@@ -16,6 +16,7 @@
  */
 #include <errno.h>
 #include <fcntl.h>
+#include <regex.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -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 b544f6cfa70286cce6837b5087be0310138441cc..004f8db092d22836fee610f05560b320f1214116 100644 (file)
--- 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