pxar: stricter file descriptor guards

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
This commit is contained in:
Wolfgang Bumiller 2020-11-30 10:49:18 +01:00
parent 35fe981c7d
commit dd519bbad1
2 changed files with 30 additions and 17 deletions

View File

@ -8,6 +8,7 @@ use nix::fcntl::OFlag;
use nix::sys::stat::{mkdirat, Mode}; use nix::sys::stat::{mkdirat, Mode};
use proxmox::sys::error::SysError; use proxmox::sys::error::SysError;
use proxmox::tools::fd::BorrowedFd;
use pxar::Metadata; use pxar::Metadata;
use crate::pxar::tools::{assert_single_path_component, perms_from_metadata}; use crate::pxar::tools::{assert_single_path_component, perms_from_metadata};
@ -35,7 +36,11 @@ impl PxarDir {
} }
} }
fn create_dir(&mut self, parent: RawFd, allow_existing_dirs: bool) -> Result<RawFd, Error> { fn create_dir(
&mut self,
parent: RawFd,
allow_existing_dirs: bool,
) -> Result<BorrowedFd, Error> {
match mkdirat( match mkdirat(
parent, parent,
self.file_name.as_os_str(), self.file_name.as_os_str(),
@ -52,7 +57,7 @@ impl PxarDir {
self.open_dir(parent) self.open_dir(parent)
} }
fn open_dir(&mut self, parent: RawFd) -> Result<RawFd, Error> { fn open_dir(&mut self, parent: RawFd) -> Result<BorrowedFd, Error> {
let dir = Dir::openat( let dir = Dir::openat(
parent, parent,
self.file_name.as_os_str(), self.file_name.as_os_str(),
@ -60,14 +65,14 @@ impl PxarDir {
Mode::empty(), Mode::empty(),
)?; )?;
let fd = dir.as_raw_fd(); let fd = BorrowedFd::new(&dir);
self.dir = Some(dir); self.dir = Some(dir);
Ok(fd) Ok(fd)
} }
pub fn try_as_raw_fd(&self) -> Option<RawFd> { pub fn try_as_borrowed_fd(&self) -> Option<BorrowedFd> {
self.dir.as_ref().map(AsRawFd::as_raw_fd) self.dir.as_ref().map(BorrowedFd::new)
} }
pub fn metadata(&self) -> &Metadata { pub fn metadata(&self) -> &Metadata {
@ -119,32 +124,39 @@ impl PxarDirStack {
Ok(out) Ok(out)
} }
pub fn last_dir_fd(&mut self, allow_existing_dirs: bool) -> Result<RawFd, Error> { pub fn last_dir_fd(&mut self, allow_existing_dirs: bool) -> Result<BorrowedFd, Error> {
// should not be possible given the way we use it: // should not be possible given the way we use it:
assert!(!self.dirs.is_empty(), "PxarDirStack underrun"); assert!(!self.dirs.is_empty(), "PxarDirStack underrun");
let dirs_len = self.dirs.len();
let mut fd = self.dirs[self.created - 1] let mut fd = self.dirs[self.created - 1]
.try_as_raw_fd() .try_as_borrowed_fd()
.ok_or_else(|| format_err!("lost track of directory file descriptors"))?; .ok_or_else(|| format_err!("lost track of directory file descriptors"))?
while self.created < self.dirs.len() { .as_raw_fd();
fd = self.dirs[self.created].create_dir(fd, allow_existing_dirs)?;
while self.created < dirs_len {
fd = self.dirs[self.created]
.create_dir(fd, allow_existing_dirs)?
.as_raw_fd();
self.created += 1; self.created += 1;
} }
Ok(fd) self.dirs[self.created - 1]
.try_as_borrowed_fd()
.ok_or_else(|| format_err!("lost track of directory file descriptors"))
} }
pub fn create_last_dir(&mut self, allow_existing_dirs: bool) -> Result<(), Error> { pub fn create_last_dir(&mut self, allow_existing_dirs: bool) -> Result<(), Error> {
let _: RawFd = self.last_dir_fd(allow_existing_dirs)?; let _: BorrowedFd = self.last_dir_fd(allow_existing_dirs)?;
Ok(()) Ok(())
} }
pub fn root_dir_fd(&self) -> Result<RawFd, Error> { pub fn root_dir_fd(&self) -> Result<BorrowedFd, Error> {
// should not be possible given the way we use it: // should not be possible given the way we use it:
assert!(!self.dirs.is_empty(), "PxarDirStack underrun"); assert!(!self.dirs.is_empty(), "PxarDirStack underrun");
self.dirs[0] self.dirs[0]
.try_as_raw_fd() .try_as_borrowed_fd()
.ok_or_else(|| format_err!("lost track of directory file descriptors")) .ok_or_else(|| format_err!("lost track of directory file descriptors"))
} }
} }

View File

@ -277,11 +277,11 @@ impl Extractor {
.map_err(|err| format_err!("unexpected end of directory entry: {}", err))? .map_err(|err| format_err!("unexpected end of directory entry: {}", err))?
.ok_or_else(|| format_err!("broken pxar archive (directory stack underrun)"))?; .ok_or_else(|| format_err!("broken pxar archive (directory stack underrun)"))?;
if let Some(fd) = dir.try_as_raw_fd() { if let Some(fd) = dir.try_as_borrowed_fd() {
metadata::apply( metadata::apply(
self.feature_flags, self.feature_flags,
dir.metadata(), dir.metadata(),
fd, fd.as_raw_fd(),
&CString::new(dir.file_name().as_bytes())?, &CString::new(dir.file_name().as_bytes())?,
&mut self.on_error, &mut self.on_error,
) )
@ -298,6 +298,7 @@ impl Extractor {
fn parent_fd(&mut self) -> Result<RawFd, Error> { fn parent_fd(&mut self) -> Result<RawFd, Error> {
self.dir_stack self.dir_stack
.last_dir_fd(self.allow_existing_dirs) .last_dir_fd(self.allow_existing_dirs)
.map(|d| d.as_raw_fd())
.map_err(|err| format_err!("failed to get parent directory file descriptor: {}", err)) .map_err(|err| format_err!("failed to get parent directory file descriptor: {}", err))
} }
@ -325,7 +326,7 @@ impl Extractor {
let root = self.dir_stack.root_dir_fd()?; let root = self.dir_stack.root_dir_fd()?;
let target = CString::new(link.as_bytes())?; let target = CString::new(link.as_bytes())?;
nix::unistd::linkat( nix::unistd::linkat(
Some(root), Some(root.as_raw_fd()),
target.as_c_str(), target.as_c_str(),
Some(parent), Some(parent),
file_name, file_name,