resolvers: opath: improve symlink-related errors

We want to return -ELOOP for our RESOLVE_NO_SYMLINKS and
RESOLVE_NO_MAGICLINKS emulation (to match openat2). In addition, the
checking of whether a symlink is "safe" to follow for was overly
aggressive -- /proc/self is fine to follow, it's the absolute links that
don't make sense to follow.

Some new tests are included to verify this parity.

Signed-off-by: Aleksa Sarai <[email protected]>

src/error.rs

@@ -143,6 +143,11 @@ pub enum Error {
 pub(crate) trait ErrorExt {
     /// Wrap a `Result<..., Error>` with an additional context string.
     fn wrap<S: Into<String>>(self, context: S) -> Self;
+
+    /// Wrap a `Result<..., Error>` with an additional context string created by a closure.
+    fn with_wrap<F>(self, context_fn: F) -> Self
+    where
+        F: FnOnce() -> String;
 }
 
 impl<T> ErrorExt for Result<T, Error> {
@@ -151,6 +156,13 @@ impl<T> ErrorExt for Result<T, Error> {
             context: context.into(),
         })
     }
+
+    fn with_wrap<F>(self, context_fn: F) -> Self
+    where
+        F: FnOnce() -> String,
+    {
+        self.wrap(context_fn())
+    }
 }
 
 /// A backport of the nightly-only [`Chain`]. This method

src/resolvers/opath.rs

@@ -247,23 +247,16 @@ pub(crate) fn resolve<P: AsRef<Path>>(
 
         // Don't continue walking if user asked for no symlinks.
         if flags.contains(ResolverFlags::NO_SYMLINKS) {
-            return error::SafetyViolationSnafu {
-                description: "next is a symlink and symlink resolution disabled",
-            }
-            .fail();
-        }
-
-        // Check if it's safe for us to touch. In principle this should
-        // never be an actual security issue (since we readlink(2) the
-        // symlink) but it's much better to be safe than sorry here.
-        if next
-            .is_dangerous()
-            .wrap("check if next is on a dangerous filesystem")?
-        {
-            return error::SafetyViolationSnafu {
-                description: "next is a symlink on a dangerous filesystem",
-            }
-            .fail();
+            return Err(IOError::from_raw_os_error(libc::ELOOP))
+                .context(error::OsSnafu {
+                    operation: "emulated symlink resolution",
+                })
+                .with_wrap(|| {
+                    format!(
+                        "component {:?} is a symlink but symlink resolution is disabled",
+                        part
+                    )
+                })?;
         }
 
         // We need a limit on the number of symlinks we traverse to
@@ -280,6 +273,34 @@ pub(crate) fn resolve<P: AsRef<Path>>(
                 operation: "readlink next symlink component",
             })?;
 
+        // Check if it's a good idea to walk this symlink. If we are on a
+        // filesystem that supports magic-links and we've hit an absolute
+        // symlink, it is incredibly likely that this component is a magic-link
+        // and it makes no sense to try to resolve it in userspace.
+        //
+        // NOTE: There are some pseudo-magic-links like /proc/self (which
+        // dynamically generates the symlink contents but doesn't use
+        // nd_jump_link). In the case of procfs, these are always relative, and
+        // they are reasonable for us to walk.
+        //
+        // In procfs, all magic-links use d_path() to generate readlink() and
+        // thus are all absolute paths. (Unfortunately, apparmorfs uses
+        // nd_jump_link to make /sys/kernel/security/apparmor/policy dynamic
+        // using actual nd_jump_link() and their readlink give us a dummy
+        // relative path like "apparmorfs:[123]". But in that case we will just
+        // get an error.)
+        if link_target.is_absolute()
+            && next
+                .is_dangerous()
+                .wrap("check if next is on a dangerous filesystem")?
+        {
+            return Err(IOError::from_raw_os_error(libc::ELOOP))
+                .context(error::OsSnafu {
+                    operation: "emulated RESOLVE_NO_MAGICLINKS",
+                })
+                .wrap("walked into a potential magic-link")?;
+        }
+
         // Remove the link component from our expectex path.
         expected_path.pop();
 

src/syscalls.rs

@@ -752,3 +752,9 @@ pub fn openat2<P: AsRef<Path>>(dirfd: RawFd, path: P, how: &OpenHow) -> Result<F
         })
     }
 }
+
+#[cfg(test)]
+pub fn getpid() -> libc::pid_t {
+    // SAFETY: Obviously safe libc function.
+    unsafe { libc::getpid() }
+}