Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtio/fs: Check capabilities & act accordingly #244

Merged
merged 2 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/devices/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,4 @@ lru = ">=0.9"

[target.'cfg(target_os = "linux")'.dependencies]
rutabaga_gfx = { path = "../rutabaga_gfx", features = ["x"], optional = true }
caps = "0.5.5"
90 changes: 70 additions & 20 deletions src/devices/src/virtio/fs/linux/passthrough.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
use std::sync::{Arc, RwLock};
use std::time::Duration;

use caps::{has_cap, CapSet, Capability};
use nix::request_code_read;

use vm_memory::ByteValued;
Expand Down Expand Up @@ -78,11 +79,6 @@ macro_rules! scoped_cred {
// Changes the effective uid/gid of the current thread to `val`. Changes
// the thread's credentials back to root when the returned struct is dropped.
fn new(val: $ty) -> io::Result<Option<$name>> {
if val == 0 {
// Nothing to do since we are already uid 0.
return Ok(None);
}

// We want credential changes to be per-thread because otherwise
// we might interfere with operations being carried out on other
// threads with different uids/gids. However, posix requires that
Expand Down Expand Up @@ -122,15 +118,6 @@ macro_rules! scoped_cred {
scoped_cred!(ScopedUid, libc::uid_t, libc::SYS_setresuid);
scoped_cred!(ScopedGid, libc::gid_t, libc::SYS_setresgid);

fn set_creds(
uid: libc::uid_t,
gid: libc::gid_t,
) -> io::Result<(Option<ScopedUid>, Option<ScopedGid>)> {
// We have to change the gid before we change the uid because if we change the uid first then we
// lose the capability to change the gid. However changing back can happen in any order.
ScopedGid::new(gid).and_then(|gid| Ok((ScopedUid::new(uid)?, gid)))
}

fn ebadf() -> io::Error {
io::Error::from_raw_os_error(libc::EBADF)
}
Expand Down Expand Up @@ -351,6 +338,9 @@ pub struct PassthroughFs {
// `cfg.writeback` is true and `init` was called with `FsOptions::WRITEBACK_CACHE`.
writeback: AtomicBool,
announce_submounts: AtomicBool,
my_uid: Option<libc::uid_t>,
my_gid: Option<libc::gid_t>,
cap_fowner: bool,

cfg: Config,
}
Expand Down Expand Up @@ -385,6 +375,25 @@ impl PassthroughFs {
fd
};

let my_uid = if has_cap(None, CapSet::Effective, Capability::CAP_SETUID).unwrap_or_default()
{
None
} else {
// SAFETY: This syscall is always safe to call and always succeeds.
Some(unsafe { libc::getuid() })
};

let my_gid = if has_cap(None, CapSet::Effective, Capability::CAP_SETGID).unwrap_or_default()
{
None
} else {
// SAFETY: This syscall is always safe to call and always succeeds.
Some(unsafe { libc::getgid() })
};

let cap_fowner =
has_cap(None, CapSet::Effective, Capability::CAP_FOWNER).unwrap_or_default();

// Safe because we just opened this fd or it was provided by our caller.
let proc_self_fd = unsafe { File::from_raw_fd(fd) };

Expand All @@ -401,6 +410,9 @@ impl PassthroughFs {

writeback: AtomicBool::new(false),
announce_submounts: AtomicBool::new(false),
my_uid,
my_gid,
cap_fowner,
cfg,
})
}
Expand Down Expand Up @@ -669,8 +681,15 @@ impl PassthroughFs {
Ok(())
}

fn do_open(&self, inode: Inode, flags: u32) -> io::Result<(Option<Handle>, OpenOptions)> {
fn do_open(&self, inode: Inode, mut flags: u32) -> io::Result<(Option<Handle>, OpenOptions)> {
debug!("do_open: {:?}", inode);
if !self.cap_fowner {
// O_NOATIME can only be used with CAP_FOWNER or if we are the file
// owner. Not worth checking the latter, just drop it if we don't
// have the cap. This makes overlayfs mounts with virtiofs lower dirs
// work.
flags &= !(libc::O_NOATIME as u32);
}
let file = RwLock::new(self.open_inode(inode, flags as i32)?);

let handle = self.next_handle.fetch_add(1, Ordering::Relaxed);
Expand Down Expand Up @@ -758,6 +777,37 @@ impl PassthroughFs {
Err(io::Error::last_os_error())
}
}

fn set_creds(
&self,
uid: libc::uid_t,
gid: libc::gid_t,
) -> io::Result<(Option<ScopedUid>, Option<ScopedGid>)> {
// Change the gid first, since once we change the uid we lose the capability to change the gid.
let scoped_gid = if gid == 0 || self.my_gid == Some(gid) {
// Always allow "root" accesses even if we don't have root powers.
// This means guest processes running as root can use /tmp (though
// the files will not be actually owned by root), which is desirable.
None
} else if self.my_gid.is_some() {
// Reject writes as any other gid if we do not have setgid
// privileges.
return Err(io::Error::from_raw_os_error(libc::EPERM));
} else {
ScopedGid::new(gid)?
};

// Same logic as above, for uid.
let scoped_uid = if uid == 0 || self.my_uid == Some(uid) {
None
} else if self.my_uid.is_some() {
return Err(io::Error::from_raw_os_error(libc::EPERM));
} else {
ScopedUid::new(uid)?
};

Ok((scoped_uid, scoped_gid))
}
}

fn forget_one(
Expand Down Expand Up @@ -957,7 +1007,7 @@ impl FileSystem for PassthroughFs {
unimplemented!("SECURITY_CTX is not supported and should not be used by the guest");
}

let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?;
let (_uid, _gid) = self.set_creds(ctx.uid, ctx.gid)?;
let data = self
.inodes
.read()
Expand Down Expand Up @@ -1060,7 +1110,7 @@ impl FileSystem for PassthroughFs {
unimplemented!("SECURITY_CTX is not supported and should not be used by the guest");
}

let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?;
let (_uid, _gid) = self.set_creds(ctx.uid, ctx.gid)?;
let data = self
.inodes
.read()
Expand Down Expand Up @@ -1167,7 +1217,7 @@ impl FileSystem for PassthroughFs {
if kill_priv {
// We need to change credentials during a write so that the kernel will remove setuid
// or setgid bits from the file if it was written to by someone other than the owner.
let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?;
let (_uid, _gid) = self.set_creds(ctx.uid, ctx.gid)?;
}

let data = self
Expand Down Expand Up @@ -1395,7 +1445,7 @@ impl FileSystem for PassthroughFs {
unimplemented!("SECURITY_CTX is not supported and should not be used by the guest");
}

let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?;
let (_uid, _gid) = self.set_creds(ctx.uid, ctx.gid)?;
let data = self
.inodes
.read()
Expand Down Expand Up @@ -1476,7 +1526,7 @@ impl FileSystem for PassthroughFs {
unimplemented!("SECURITY_CTX is not supported and should not be used by the guest");
}

let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?;
let (_uid, _gid) = self.set_creds(ctx.uid, ctx.gid)?;
let data = self
.inodes
.read()
Expand Down
Loading