From 464d354de0858253b5c9e6f5eb25d9430c1024a6 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 12 Sep 2024 15:47:24 -0400 Subject: [PATCH 1/2] dumpfile: Limit max inline content Corresponding to what we did on the C side. (Yeah, if we grow more of this we can try to use bindgen to share constants) This I'm pretty sure is the last section of thing that accepted arbitrarily sized data. Signed-off-by: Colin Walters --- rust/composefs/src/dumpfile.rs | 45 ++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/rust/composefs/src/dumpfile.rs b/rust/composefs/src/dumpfile.rs index 327da7f6..398445ff 100644 --- a/rust/composefs/src/dumpfile.rs +++ b/rust/composefs/src/dumpfile.rs @@ -15,11 +15,14 @@ use std::os::unix::ffi::{OsStrExt, OsStringExt}; use std::path::{Path, PathBuf}; use std::process::Command; use std::str::FromStr; +use std::usize; use anyhow::Context; use anyhow::{anyhow, Result}; use libc::S_IFDIR; +/// Maximum size accepted for inline content. +const MAX_INLINE_CONTENT: u16 = 5000; /// https://github.com/torvalds/linux/blob/47ac09b91befbb6a235ab620c32af719f8208399/include/uapi/linux/limits.h#L15 /// This isn't exposed in libc/rustix, and in any case we should be conservative...if this ever /// gets bumped it'd be a hazard. @@ -118,15 +121,25 @@ pub enum Item<'p> { }, } -/// Unescape a byte array according to the composefs dump file escaping format. -fn unescape(s: &str) -> Result> { - // If there are no escapes, just return the input unchanged - if !s.contains('\\') { +/// Unescape a byte array according to the composefs dump file escaping format, +/// limiting the maximum possible size. +fn unescape_limited(s: &str, max: usize) -> Result> { + // If there are no escapes, just return the input unchanged. However, + // it must also be ASCII to maintain a 1-1 correspondence between byte + // and character. + if !s.contains('\\') && s.chars().all(|c| c.is_ascii()) { + let len = s.len(); + if len > max { + anyhow::bail!("Input {len} exceeded maximum length {max}"); + } return Ok(Cow::Borrowed(s.as_bytes())); } let mut it = s.chars(); let mut r = Vec::new(); while let Some(c) = it.next() { + if r.len() == max { + anyhow::bail!("Input exceeded maximum length {max}"); + } if c != '\\' { write!(r, "{c}").unwrap(); continue; @@ -157,6 +170,11 @@ fn unescape(s: &str) -> Result> { Ok(r.into()) } +/// Unescape a byte array according to the composefs dump file escaping format. +fn unescape(s: &str) -> Result> { + return unescape_limited(s, usize::MAX); +} + /// Unescape a string into a Rust `OsStr` which is really just an alias for a byte array, /// but we also impose a constraint that it can not have an embedded NUL byte. fn unescape_to_osstr(s: &str) -> Result> { @@ -364,7 +382,9 @@ impl<'p> Entry<'p> { libc::S_IFREG => Item::Regular { size, nlink, - inline_content: content.map(unescape).transpose()?, + inline_content: content + .map(|c| unescape_limited(c, MAX_INLINE_CONTENT.into())) + .transpose()?, fsverity_digest: fsverity_digest.map(ToOwned::to_owned), }, libc::S_IFLNK => { @@ -605,6 +625,21 @@ mod tests { } } + #[test] + fn test_unescape() { + assert_eq!(unescape("").unwrap().len(), 0); + assert_eq!(unescape_limited("", 0).unwrap().len(), 0); + assert!(unescape_limited("foobar", 3).is_err()); + // This is borrowed input + assert!(matches!( + unescape_limited("foobar", 6).unwrap(), + Cow::Borrowed(_) + )); + // But non-ASCII is currently owned out of conservatism + assert!(matches!(unescape_limited("→", 6).unwrap(), Cow::Owned(_))); + assert!(unescape_limited("foo→bar", 3).is_err()); + } + #[test] fn test_unescape_path() { // Empty From dd6ea604514bf3c0d36cd58db48e97fedf9d5015 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 13 Sep 2024 13:55:25 -0400 Subject: [PATCH 2/2] dumpfile: Deny content being set for non-regfiles Just additional validation. Signed-off-by: Colin Walters --- rust/composefs/src/dumpfile.rs | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/rust/composefs/src/dumpfile.rs b/rust/composefs/src/dumpfile.rs index 398445ff..168600fd 100644 --- a/rust/composefs/src/dumpfile.rs +++ b/rust/composefs/src/dumpfile.rs @@ -388,6 +388,9 @@ impl<'p> Entry<'p> { fsverity_digest: fsverity_digest.map(ToOwned::to_owned), }, libc::S_IFLNK => { + if content.is_some() { + anyhow::bail!("symlinks cannot have content"); + } // Note that the target of *symlinks* is not required to be in canonical form, // as we don't actually traverse those links on our own, and we need to support // symlinks that e.g. contain `//` or other things. @@ -399,9 +402,24 @@ impl<'p> Entry<'p> { } Item::Symlink { nlink, target } } - libc::S_IFIFO => Item::Fifo { nlink }, - libc::S_IFCHR | libc::S_IFBLK => Item::Device { nlink, rdev }, - libc::S_IFDIR => Item::Directory { size, nlink }, + libc::S_IFIFO => { + if content.is_some() { + anyhow::bail!("entry cannot have content"); + } + Item::Fifo { nlink } + } + libc::S_IFCHR | libc::S_IFBLK => { + if content.is_some() { + anyhow::bail!("entry cannot have content"); + } + Item::Device { nlink, rdev } + } + libc::S_IFDIR => { + if content.is_some() { + anyhow::bail!("entry cannot have content"); + } + Item::Directory { size, nlink } + } o => { anyhow::bail!("Unhandled mode {o:o}") } @@ -773,6 +791,10 @@ mod tests { "dir hardlink", include_str!("../../../tests/assets/should-fail-dir-hardlink.dump"), ), + ( + "content in fifo", + "/ 4096 40755 2 0 0 0 0.0 - - -\n/fifo 0 10777 1 0 0 0 0.0 - foobar -", + ), ]; for (name, case) in CASES.iter().copied() { assert!(