Skip to content

Commit

Permalink
lints: add check for non-utf-8 filesystem content
Browse files Browse the repository at this point in the history
As mentioned in containers#975, we should add a lint to ensure that all filenames
are UTF-8, giving nice errors (with full pathnames) in the event that we
encounter invalid filenames.

We also check symlink targets.

Add a unit test that tries various valid and invalid scenarios.

Signed-off-by: Allison Karlitskaya <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
  • Loading branch information
allisonkarlitskaya authored and cgwalters committed Dec 19, 2024
1 parent 6759010 commit 23063f3
Show file tree
Hide file tree
Showing 2 changed files with 172 additions and 3 deletions.
144 changes: 142 additions & 2 deletions lib/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,26 @@
use std::env::consts::ARCH;

use anyhow::Result;
use anyhow::{bail, ensure, Result};
use cap_std::fs::Dir;
use cap_std_ext::cap_std;
use cap_std_ext::dirext::CapStdExtDirExt as _;
use fn_error_context::context;

use crate::utils::openat2_with_retry;

/// check for the existence of the /var/run directory
/// if it exists we need to check that it links to /run if not error
/// if it does not exist error.
#[context("Linting")]
pub(crate) fn lint(root: &Dir) -> Result<()> {
let lints = [check_var_run, check_kernel, check_parse_kargs, check_usretc];
let lints = [
check_var_run,
check_kernel,
check_parse_kargs,
check_usretc,
check_utf8,
];
for lint in lints {
lint(&root)?;
}
Expand Down Expand Up @@ -59,12 +67,72 @@ fn check_kernel(root: &Dir) -> Result<()> {
Ok(())
}

/// Open the target directory, but return Ok(None) if this would cross a mount point.
fn open_dir_noxdev(
parent: &Dir,
path: impl AsRef<std::path::Path>,
) -> std::io::Result<Option<Dir>> {
use rustix::fs::{Mode, OFlags, ResolveFlags};
match openat2_with_retry(
parent,
path,
OFlags::CLOEXEC | OFlags::DIRECTORY | OFlags::NOFOLLOW,
Mode::empty(),
ResolveFlags::NO_XDEV | ResolveFlags::BENEATH,
) {
Ok(r) => Ok(Some(Dir::reopen_dir(&r)?)),
Err(e) if e == rustix::io::Errno::XDEV => Ok(None),
Err(e) => return Err(e.into()),
}
}

fn check_utf8(dir: &Dir) -> Result<()> {
for entry in dir.entries()? {
let entry = entry?;
let name = entry.file_name();

let Some(strname) = &name.to_str() else {
// will escape nicely like "abc\xFFdéf"
bail!("/: Found non-utf8 filename {name:?}");
};

let ifmt = entry.file_type()?;
if ifmt.is_symlink() {
let target = dir.read_link_contents(&name)?;
ensure!(
target.to_str().is_some(),
"/{strname}: Found non-utf8 symlink target"
);
} else if ifmt.is_dir() {
let Some(subdir) = open_dir_noxdev(dir, entry.file_name())? else {
continue;
};
if let Err(err) = check_utf8(&subdir) {
// Try to do the path pasting only in the event of an error
bail!("/{strname}{err:?}");
}
}
}
Ok(())
}

#[cfg(test)]
fn fixture() -> Result<cap_std_ext::cap_tempfile::TempDir> {
let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?;
Ok(tempdir)
}

#[test]
fn test_open_noxdev() -> Result<()> {
let root = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
// This hard requires the host setup to have /usr/bin on the same filesystem as /
let usr = Dir::open_ambient_dir("/usr", cap_std::ambient_authority())?;
assert!(open_dir_noxdev(&usr, "bin").unwrap().is_some());
// Requires a mounted /proc, but that also seems ane.
assert!(open_dir_noxdev(&root, "proc").unwrap().is_none());
Ok(())
}

#[test]
fn test_var_run() -> Result<()> {
let root = &fixture()?;
Expand Down Expand Up @@ -117,3 +185,75 @@ fn test_usr_etc() -> Result<()> {
check_usretc(root).unwrap();
Ok(())
}

#[test]
fn test_non_utf8() {
use std::{ffi::OsStr, os::unix::ffi::OsStrExt};

let root = &fixture().unwrap();

// Try to create some adversarial symlink situations to ensure the walk doesn't crash
root.create_dir("subdir").unwrap();
// Self-referential symlinks
root.symlink("self", "self").unwrap();
// Infinitely looping dir symlinks
root.symlink("..", "subdir/parent").unwrap();
// Broken symlinks
root.symlink("does-not-exist", "broken").unwrap();
// Out-of-scope symlinks
root.symlink("../../x", "escape").unwrap();
// Should be fine
check_utf8(root).unwrap();

// But this will cause an issue
let baddir = OsStr::from_bytes(b"subdir/2/bad\xffdir");
root.create_dir("subdir/2").unwrap();
root.create_dir(baddir).unwrap();
let Err(err) = check_utf8(root) else {
unreachable!("Didn't fail");
};
assert_eq!(
err.to_string(),
r#"/subdir/2/: Found non-utf8 filename "bad\xFFdir""#
);
root.remove_dir(baddir).unwrap(); // Get rid of the problem
check_utf8(root).unwrap(); // Check it

// Create a new problem in the form of a regular file
let badfile = OsStr::from_bytes(b"regular\xff");
root.write(badfile, b"Hello, world!\n").unwrap();
let Err(err) = check_utf8(root) else {
unreachable!("Didn't fail");
};
assert_eq!(
err.to_string(),
r#"/: Found non-utf8 filename "regular\xFF""#
);
root.remove_file(badfile).unwrap(); // Get rid of the problem
check_utf8(root).unwrap(); // Check it

// And now test invalid symlink targets
root.symlink(badfile, "subdir/good-name").unwrap();
let Err(err) = check_utf8(root) else {
unreachable!("Didn't fail");
};
assert_eq!(
err.to_string(),
r#"/subdir/good-name: Found non-utf8 symlink target"#
);
root.remove_file("subdir/good-name").unwrap(); // Get rid of the problem
check_utf8(root).unwrap(); // Check it

// Finally, test a self-referential symlink with an invalid name.
// We should spot the invalid name before we check the target.
root.symlink(badfile, badfile).unwrap();
let Err(err) = check_utf8(root) else {
unreachable!("Didn't fail");
};
assert_eq!(
err.to_string(),
r#"/: Found non-utf8 filename "regular\xFF""#
);
root.remove_file(badfile).unwrap(); // Get rid of the problem
check_utf8(root).unwrap(); // Check it
}
31 changes: 30 additions & 1 deletion lib/src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::future::Future;
use std::io::Write;
use std::os::fd::BorrowedFd;
use std::os::fd::{AsFd, BorrowedFd, OwnedFd};
use std::path::Path;
use std::process::Command;
use std::time::Duration;

Expand All @@ -20,6 +21,7 @@ use libsystemd::logging::journal_print;
use ostree::glib;
use ostree_ext::container::SignatureSource;
use ostree_ext::ostree;
use rustix::path::Arg;

/// Try to look for keys injected by e.g. rpm-ostree requesting machine-local
/// changes; if any are present, return `true`.
Expand Down Expand Up @@ -56,6 +58,33 @@ pub(crate) fn deployment_fd(
sysroot_dir.open_dir(&dirpath).map_err(Into::into)
}

/// A thin wrapper for [`openat2`] but that retries on interruption.
pub fn openat2_with_retry(
dirfd: impl AsFd,
path: impl AsRef<Path>,
oflags: rustix::fs::OFlags,
mode: rustix::fs::Mode,
resolve: rustix::fs::ResolveFlags,
) -> rustix::io::Result<OwnedFd> {
let dirfd = dirfd.as_fd();
let path = path.as_ref();
// We loop forever on EAGAIN right now. The cap-std version loops just 4 times,
// which seems really arbitrary.
path.into_with_c_str(|path_c_str| 'start: loop {
match rustix::fs::openat2(dirfd, path_c_str, oflags, mode, resolve) {
Ok(file) => {
return Ok(file);
}
Err(rustix::io::Errno::AGAIN | rustix::io::Errno::INTR) => {
continue 'start;
}
Err(e) => {
return Err(e);
}
}
})
}

/// Given an mount option string list like foo,bar=baz,something=else,ro parse it and find
/// the first entry like $optname=
/// This will not match a bare `optname` without an equals.
Expand Down

0 comments on commit 23063f3

Please sign in to comment.