From 9891efc61f4203a0b56bb49456d8d4486993ecd8 Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Wed, 13 Oct 2021 15:43:30 +1100
Subject: [PATCH] rustjail: Correct sanity checks on device path

For each user supplied device, create_devices() checks that the given path
actually is in /dev, by checking that its path starts with /dev and does
not contain "..".

However, this has subtle errors because it's interpreting the path as a raw
string without considering separators.  It will accept the path /devfoo
which it should not, while it will not accept the valid (though weird)
paths /dev/... and /dev/a..b.

Correct this by using std::path::Path methods designed for the purpose.
Having done this, it's trivial to also generate the relative path that
mknod_dev() or bind_dev() will need, so do that at the same time.

We also move this logic into a helper function so that we can add some unit
tests for it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 src/agent/rustjail/src/mount.rs | 46 ++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs
index ec808d5492..ac9498c73d 100644
--- a/src/agent/rustjail/src/mount.rs
+++ b/src/agent/rustjail/src/mount.rs
@@ -3,7 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0
 //
 
-use anyhow::{anyhow, bail, Context, Result};
+use anyhow::{anyhow, Context, Result};
 use libc::uid_t;
 use nix::errno::Errno;
 use nix::fcntl::{self, OFlag};
@@ -19,7 +19,7 @@ use std::fs::{self, OpenOptions};
 use std::mem::MaybeUninit;
 use std::os::unix;
 use std::os::unix::io::RawFd;
-use std::path::{Path, PathBuf};
+use std::path::{Component, Path, PathBuf};
 
 use path_absolutize::*;
 use std::fs::File;
@@ -828,6 +828,19 @@ fn default_symlinks() -> Result<()> {
     }
     Ok(())
 }
+
+fn dev_rel_path(path: &str) -> Option<&Path> {
+    let path = Path::new(path);
+
+    if !path.starts_with("/dev")
+        || path == Path::new("/dev")
+        || path.components().any(|c| c == Component::ParentDir)
+    {
+        return None;
+    }
+    path.strip_prefix("/").ok()
+}
+
 fn create_devices(devices: &[LinuxDevice], bind: bool) -> Result<()> {
     let op: fn(&LinuxDevice, &Path) -> Result<()> = if bind { bind_dev } else { mknod_dev };
     let old = stat::umask(Mode::from_bits_truncate(0o000));
@@ -836,11 +849,10 @@ fn create_devices(devices: &[LinuxDevice], bind: bool) -> Result<()> {
         op(dev, path).context(format!("Creating container device {:?}", dev))?;
     }
     for dev in devices {
-        if !dev.path.starts_with("/dev") || dev.path.contains("..") {
+        let path = dev_rel_path(&dev.path).ok_or_else(|| {
             let msg = format!("{} is not a valid device path", dev.path);
-            bail!(anyhow!(msg));
-        }
-        let path = Path::new(&dev.path[1..]);
+            anyhow!(msg)
+        })?;
         op(dev, path).context(format!("Creating container device {:?}", dev))?;
     }
     stat::umask(old);
@@ -1382,4 +1394,26 @@ mod tests {
             assert!(result == t.result, "{}", msg);
         }
     }
+
+    #[test]
+    fn test_dev_rel_path() {
+        // Valid device paths
+        assert_eq!(dev_rel_path("/dev/sda").unwrap(), Path::new("dev/sda"));
+        assert_eq!(dev_rel_path("//dev/sda").unwrap(), Path::new("dev/sda"));
+        assert_eq!(
+            dev_rel_path("/dev/vfio/99").unwrap(),
+            Path::new("dev/vfio/99")
+        );
+        assert_eq!(dev_rel_path("/dev/...").unwrap(), Path::new("dev/..."));
+        assert_eq!(dev_rel_path("/dev/a..b").unwrap(), Path::new("dev/a..b"));
+        assert_eq!(dev_rel_path("/dev//foo").unwrap(), Path::new("dev/foo"));
+
+        // Bad device paths
+        assert!(dev_rel_path("/devfoo").is_none());
+        assert!(dev_rel_path("/etc/passwd").is_none());
+        assert!(dev_rel_path("/dev/../etc/passwd").is_none());
+        assert!(dev_rel_path("dev/foo").is_none());
+        assert!(dev_rel_path("").is_none());
+        assert!(dev_rel_path("/dev").is_none());
+    }
 }