Jailer tweaks

src/jailer/src/chroot.rs

@@ -10,9 +10,9 @@
 
 use super::{to_cstring, JailerError};
 
-const OLD_ROOT_DIR_NAME_NUL_TERMINATED: &[u8] = b"old_root\0";
-const ROOT_DIR_NUL_TERMINATED: &[u8] = b"/\0";
-const CURRENT_DIR_NUL_TERMINATED: &[u8] = b".\0";
+const OLD_ROOT_DIR: &CStr = c"old_root";
+const ROOT_DIR: &CStr = c"/";
+const CURRENT_DIR: &CStr = c".";
 
 // This uses switching to a new mount namespace + pivot_root(), together with the regular chroot,
 // to provide a hardened jail (at least compared to only relying on chroot).
@@ -24,16 +24,13 @@
         .into_empty_result()
         .map_err(JailerError::UnshareNewNs)?;
 
-    let root_dir = CStr::from_bytes_with_nul(ROOT_DIR_NUL_TERMINATED)
-        .map_err(JailerError::FromBytesWithNul)?;
-
     // Recursively change the propagation type of all the mounts in this namespace to SLAVE, so
     // we can call pivot_root.
     // SAFETY: Safe because we provide valid parameters.
     SyscallReturnCode(unsafe {
         libc::mount(
             null(),
-            root_dir.as_ptr(),
+            ROOT_DIR.as_ptr(),
             null(),
             libc::MS_SLAVE | libc::MS_REC,
             null(),
@@ -64,45 +61,41 @@
     // Change current dir to the chroot dir, so we only need to handle relative paths from now on.
     env::set_current_dir(path).map_err(JailerError::SetCurrentDir)?;
 
-    // We use the CStr conversion to make sure the contents of the byte slice would be a
-    // valid C string (and for the as_ptr() method).
-    let old_root_dir = CStr::from_bytes_with_nul(OLD_ROOT_DIR_NAME_NUL_TERMINATED)
-        .map_err(JailerError::FromBytesWithNul)?;
-
     // Create the old_root folder we're going to use for pivot_root, using a relative path.
     // SAFETY: The call is safe because we provide valid arguments.
-    SyscallReturnCode(unsafe { libc::mkdir(old_root_dir.as_ptr(), libc::S_IRUSR | libc::S_IWUSR) })
+    SyscallReturnCode(unsafe { libc::mkdir(OLD_ROOT_DIR.as_ptr(), libc::S_IRUSR | libc::S_IWUSR) })
         .into_empty_result()
         .map_err(JailerError::MkdirOldRoot)?;
 
-    let cwd = CStr::from_bytes_with_nul(CURRENT_DIR_NUL_TERMINATED)
-        .map_err(JailerError::FromBytesWithNul)?;
-
     // We are now ready to call pivot_root. We have to use sys_call because there is no libc
     // wrapper for pivot_root.
     // SAFETY: Safe because we provide valid parameters.
     SyscallReturnCode(unsafe {
-        libc::syscall(libc::SYS_pivot_root, cwd.as_ptr(), old_root_dir.as_ptr())
+        libc::syscall(
+            libc::SYS_pivot_root,
+            CURRENT_DIR.as_ptr(),
+            OLD_ROOT_DIR.as_ptr(),
+        )
     })
     .into_empty_result()
     .map_err(JailerError::PivotRoot)?;
 
     // pivot_root doesn't guarantee that we will be in "/" at this point, so switch to "/"
     // explicitly.
     // SAFETY: Safe because we provide valid parameters.
-    SyscallReturnCode(unsafe { libc::chdir(root_dir.as_ptr()) })
+    SyscallReturnCode(unsafe { libc::chdir(ROOT_DIR.as_ptr()) })
         .into_empty_result()
         .map_err(JailerError::ChdirNewRoot)?;
 
     // Umount the old_root, thus isolating the process from everything outside the jail root folder.
     // SAFETY: Safe because we provide valid parameters.
-    SyscallReturnCode(unsafe { libc::umount2(old_root_dir.as_ptr(), libc::MNT_DETACH) })
+    SyscallReturnCode(unsafe { libc::umount2(OLD_ROOT_DIR.as_ptr(), libc::MNT_DETACH) })
         .into_empty_result()
         .map_err(JailerError::UmountOldRoot)?;
 
     // Remove the no longer necessary old_root directory.
     // SAFETY: Safe because we provide valid parameters.
-    SyscallReturnCode(unsafe { libc::rmdir(old_root_dir.as_ptr()) })
+    SyscallReturnCode(unsafe { libc::rmdir(OLD_ROOT_DIR.as_ptr()) })
         .into_empty_result()
         .map_err(JailerError::RmOldRootDir)
 }

src/jailer/src/env.rs

@@ -1,7 +1,7 @@
 // Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use std::ffi::{CString, OsString};
+use std::ffi::{CStr, CString, OsString};
 use std::fs::{self, canonicalize, read_to_string, File, OpenOptions, Permissions};
 use std::io;
 use std::io::Write;
@@ -30,19 +30,19 @@
 // Kernel-based virtual machine (hardware virtualization extensions)
 // minor/major numbers are taken from
 // https://www.kernel.org/doc/html/latest/admin-guide/devices.html
-const DEV_KVM_WITH_NUL: &str = "/dev/kvm";
+const DEV_KVM: &CStr = c"/dev/kvm";
 const DEV_KVM_MAJOR: u32 = 10;
 const DEV_KVM_MINOR: u32 = 232;
 
 // TUN/TAP device minor/major numbers are taken from
 // www.kernel.org/doc/Documentation/networking/tuntap.txt
-const DEV_NET_TUN_WITH_NUL: &str = "/dev/net/tun";
+const DEV_NET_TUN: &CStr = c"/dev/net/tun";
 const DEV_NET_TUN_MAJOR: u32 = 10;
 const DEV_NET_TUN_MINOR: u32 = 200;
 
 // Random number generator device minor/major numbers are taken from
 // https://www.kernel.org/doc/Documentation/admin-guide/devices.txt
-const DEV_URANDOM_WITH_NUL: &str = "/dev/urandom";
+const DEV_URANDOM: &CStr = c"/dev/urandom";
 const DEV_URANDOM_MAJOR: u32 = 1;
 const DEV_URANDOM_MINOR: u32 = 9;
 
@@ -54,7 +54,7 @@
 // so we will have to find it at initialization time parsing /proc/misc.
 // What we do know is the major number for misc devices:
 // https://elixir.bootlin.com/linux/v6.1.51/source/Documentation/admin-guide/devices.txt
-const DEV_UFFD_PATH: &str = "/dev/userfaultfd";
+const DEV_UFFD_PATH: &CStr = c"/dev/userfaultfd";
 const DEV_UFFD_MAJOR: u32 = 10;
 
 // Relevant folders inside the jail that we create or/and for which we change ownership.
@@ -425,18 +425,17 @@
 
     fn mknod_and_own_dev(
         &self,
-        dev_path_str: &'static str,
+        dev_path: &CStr,
         dev_major: u32,
         dev_minor: u32,
     ) -> Result<(), JailerError> {
-        let dev_path = CString::new(dev_path_str).unwrap();
         // As per sysstat.h:
         // S_IFCHR -> character special device
         // S_IRUSR -> read permission, owner
         // S_IWUSR -> write permission, owner
         // See www.kernel.org/doc/Documentation/networking/tuntap.txt, 'Configuration' chapter for
         // more clarity.
-        // SAFETY: This is safe because dev_path is CString, and hence null-terminated.
+        // SAFETY: This is safe because dev_path is CStr, and hence null-terminated.
         SyscallReturnCode(unsafe {
             libc::mknod(
                 dev_path.as_ptr(),
@@ -445,7 +444,7 @@
             )
         })
         .into_empty_result()
-        .map_err(|err| JailerError::MknodDev(err, dev_path_str.to_owned()))?;
+        .map_err(|err| JailerError::MknodDev(err, dev_path.to_str().unwrap().to_owned()))?;
 
         // SAFETY: This is safe because dev_path is CStr, and hence null-terminated.
         SyscallReturnCode(unsafe { libc::chown(dev_path.as_ptr(), self.uid(), self.gid()) })
@@ -663,14 +662,14 @@
         // $: mknod $dev_net_tun_path c 10 200
         // www.kernel.org/doc/Documentation/networking/tuntap.txt specifies 10 and 200 as the major
         // and minor for the /dev/net/tun device.
-        self.mknod_and_own_dev(DEV_NET_TUN_WITH_NUL, DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR)?;
+        self.mknod_and_own_dev(DEV_NET_TUN, DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR)?;
         // Do the same for /dev/kvm with (major, minor) = (10, 232).
-        self.mknod_and_own_dev(DEV_KVM_WITH_NUL, DEV_KVM_MAJOR, DEV_KVM_MINOR)?;
+        self.mknod_and_own_dev(DEV_KVM, DEV_KVM_MAJOR, DEV_KVM_MINOR)?;
         // And for /dev/urandom with (major, minor) = (1, 9).
         // If the device is not accessible on the host, output a warning to inform user that MMDS
         // version 2 will not be available to use.
         let _ = self
-            .mknod_and_own_dev(DEV_URANDOM_WITH_NUL, DEV_URANDOM_MAJOR, DEV_URANDOM_MINOR)
+            .mknod_and_own_dev(DEV_URANDOM, DEV_URANDOM_MAJOR, DEV_URANDOM_MINOR)
             .map_err(|err| {
                 println!(
                     "Warning! Could not create /dev/urandom device inside jailer: {}.",
@@ -1116,14 +1115,14 @@
         // process management; it can't be isolated from side effects.
     }
 
-    fn ensure_mknod_and_own_dev(env: &Env, dev_path: &'static str, major: u32, minor: u32) {
+    fn ensure_mknod_and_own_dev(env: &Env, dev_path: &CStr, major: u32, minor: u32) {
         use std::os::unix::fs::FileTypeExt;
 
         // Create a new device node.
         env.mknod_and_own_dev(dev_path, major, minor).unwrap();
 
         // Ensure device's properties.
-        let metadata = fs::metadata(dev_path).unwrap();
+        let metadata = fs::metadata(dev_path.to_str().unwrap()).unwrap();
         assert!(metadata.file_type().is_char_device());
         assert_eq!(get_major(metadata.st_rdev()), major);
         assert_eq!(get_minor(metadata.st_rdev()), minor);
@@ -1140,7 +1139,7 @@
             ),
             format!(
                 "Failed to create {} via mknod inside the jail: File exists (os error 17)",
-                dev_path
+                dev_path.to_str().unwrap()
             )
         );
     }
@@ -1152,25 +1151,25 @@
         let env = create_env(mock_cgroups.proc_mounts_path.as_str());
 
         // Ensure device nodes are created with correct major/minor numbers and permissions.
-        let mut dev_infos: Vec<(&str, u32, u32)> = vec![
-            ("/dev/net/tun-test", DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR),
-            ("/dev/kvm-test", DEV_KVM_MAJOR, DEV_KVM_MINOR),
+        let mut dev_infos: Vec<(&CStr, u32, u32)> = vec![
+            (c"/dev/net/tun-test", DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR),
+            (c"/dev/kvm-test", DEV_KVM_MAJOR, DEV_KVM_MINOR),
         ];
 
         if let Some(uffd_dev_minor) = env.uffd_dev_minor {
-            dev_infos.push(("/dev/userfaultfd-test", DEV_UFFD_MAJOR, uffd_dev_minor));
+            dev_infos.push((c"/dev/userfaultfd-test", DEV_UFFD_MAJOR, uffd_dev_minor));
         }
 
         for (dev, major, minor) in dev_infos {
             // Checking this just to be super sure there's no file at `dev_str` path (though
             // it shouldn't be as we deleted it at the end of the previous test run).
-            if Path::new(dev).exists() {
-                fs::remove_file(dev).unwrap();
+            if Path::new(dev.to_str().unwrap()).exists() {
+                fs::remove_file(dev.to_str().unwrap()).unwrap();
             }
 
             ensure_mknod_and_own_dev(&env, dev, major, minor);
             // Remove the device node.
-            fs::remove_file(dev).expect("Could not remove file.");
+            fs::remove_file(dev.to_str().unwrap()).expect("Could not remove file.");
         }
     }
 
@@ -1180,7 +1179,7 @@
         mock_cgroups.add_v1_mounts().unwrap();
         let env = create_env(mock_cgroups.proc_mounts_path.as_str());
 
-        if !Path::new(DEV_UFFD_PATH).exists() {
+        if !Path::new(DEV_UFFD_PATH.to_str().unwrap()).exists() {
             assert_eq!(env.uffd_dev_minor, None);
         } else {
             assert!(env.uffd_dev_minor.is_some());

src/jailer/src/main.rs

@@ -305,6 +305,7 @@ pub fn to_cstring<T: AsRef<Path> + Debug>(path: T) -> Result<CString, JailerErro
     CString::new(path_str).map_err(JailerError::CStringParsing)
 }
 
+/// We wrap the actual main in order to pretty print an error with Display trait.
 fn main() -> Result<(), JailerError> {
     let result = main_exec();
     if let Err(e) = result {