mount: forbid mount on path with symlinks

sourcejedi · sourcejedi · commit 25cd49647c83 · 2018-01-20T22:06:34.000Z
It was forbidden to create mount units for a symlink.  But the reason is
that the mount unit needs to know the real path that will appear in
/proc/self/mountinfo.  The kernel dereferences *all* the symlinks in the
path at mount time (I checked this with `mount -c` running under `strace`).

This will have no effect on most systems.  As recommended by docs, most
systems use /etc/fstab, as opposed to native mount unit files.
fstab-generator dereferences symlinks for backwards compatibility.

A relatively minor issue regarding Time Of Check / Time Of Use also exists
here.  I can't see how to get rid of it entirely.  If we pass an absolute
path to mount, the racing process can replace it with a symlink.  If we
chdir() to the mount point and pass ".", the racing process can move the
directory.  The latter might potentially be nicer, except that it breaks
WorkingDirectory=.

I'm not saying the race is relevant to security - I just want to consider
how bad the effect is.  Currently, it can make the mount unit active (and
hence the job return success), despite there never being a matching entry
in /proc/self/mountinfo.  This wart will be removed in the next commit;
i.e. it will make the mount unit fail instead.
diff --git a/src/core/automount.c b/src/core/automount.c
@@ -578,7 +578,7 @@ static void automount_enter_waiting(Automount *a) {
 
         set_clear(a->tokens);
 
-        r = unit_fail_if_symlink(UNIT(a), a->where);
+        r = unit_fail_if_noncanonical(UNIT(a), a->where);
         if (r < 0)
                 goto fail;
 
diff --git a/src/core/mount.c b/src/core/mount.c
@@ -942,7 +942,7 @@ static void mount_enter_mounting(Mount *m) {
 
         assert(m);
 
-        r = unit_fail_if_symlink(UNIT(m), m->where);
+        r = unit_fail_if_noncanonical(UNIT(m), m->where);
         if (r < 0)
                 goto fail;
 
diff --git a/src/core/unit.c b/src/core/unit.c
@@ -4705,25 +4705,29 @@ void unit_warn_if_dir_nonempty(Unit *u, const char* where) {
                    NULL);
 }
 
-int unit_fail_if_symlink(Unit *u, const char* where) {
+int unit_fail_if_noncanonical(Unit *u, const char* where) {
+        _cleanup_free_ char *canonical_where;
         int r;
 
         assert(u);
         assert(where);
 
-        r = is_symlink(where);
+        r = chase_symlinks(where, NULL, CHASE_NONEXISTENT, &canonical_where);
         if (r < 0) {
-                log_unit_debug_errno(u, r, "Failed to check symlink %s, ignoring: %m", where);
+                log_unit_debug_errno(u, r, "Failed to check %s for symlinks, ignoring: %m", where);
                 return 0;
         }
-        if (r == 0)
+
+        /* We will happily ignore a trailing slash (or any redundant slashes) */
+        if (path_equal(where, canonical_where))
                 return 0;
 
+        /* No need to mention "." or "..", they would already have been rejected by unit_name_from_path() */
         log_struct(LOG_ERR,
                    "MESSAGE_ID=" SD_MESSAGE_OVERMOUNTING_STR,
                    LOG_UNIT_ID(u),
                    LOG_UNIT_INVOCATION_ID(u),
-                   LOG_UNIT_MESSAGE(u, "Mount on symlink %s not allowed.", where),
+                   LOG_UNIT_MESSAGE(u, "Mount path %s is not canonical (contains a symlink).", where),
                    "WHERE=%s", where,
                    NULL);
 
diff --git a/src/core/unit.h b/src/core/unit.h
@@ -760,7 +760,7 @@ static inline bool unit_supported(Unit *u) {
 }
 
 void unit_warn_if_dir_nonempty(Unit *u, const char* where);
-int unit_fail_if_symlink(Unit *u, const char* where);
+int unit_fail_if_noncanonical(Unit *u, const char* where);
 
 int unit_start_limit_test(Unit *u);
 

Original file line number	Diff line number	Diff line change
`@@ -760,7 +760,7 @@ static inline bool unit_supported(Unit *u) {`
`760`	`760`	`}`
`761`	`761`
`762`	`762`	`void unit_warn_if_dir_nonempty(Unit u, const char where);`
`763`		`-int unit_fail_if_symlink(Unit u, const char where);`
	`763`	`+int unit_fail_if_noncanonical(Unit u, const char where);`
`764`	`764`
`765`	`765`	`int unit_start_limit_test(Unit *u);`
`766`	`766`