From d4927ae12aa839b98bb6c0deaccc78ea09b15f81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Tue, 27 Dec 2022 11:47:28 +0100 Subject: [PATCH] compat: Improve TryCompat trait type safety and fix consistency issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is possible to implement try_compat() in an inconsistent way compared to other implementations. Instead, implement the whole compatibility logic in the default try_compat() implementation, and create tailored method to be implemented by objects: try_compat_inner() and try_compat_children(). The new CompatResult enum contains all the required informations to implement a consistent try_compat() for all trait implementations: the full, partial or not-supported state, the associated compatible value, and the potential associated error. This way, try_compat() has everything to take the decision to either return a Some(Self) or a CompatError according to the CompatLevel. The try_compat() default implementation now always update the self's CompatState, and then return a result according to its CompatLevel thanks to clean and exhaustive CompatResult matches. We now only use the Compatibility struct for Ruleset and RulesetCreated, but we pass separately ABI, CompatLevel and &mut CompatState to try_compat(). This enables to differentiate mutability and store CompatState in all Compatible types. ABI and CompatLevel should only be inherited from Ruleset and RulesetCreated. try_compat_inner() needs to be implemented by all TryCompat types. This method only gets a non-mutable ABI, and returns a new CompatResult or a CompatError. Delegating nested TryCompat objects management (e.g. allowed_access by PathBeneath) can lead to inconsistencies between different objects. To avoid this issue as much as possible, create a new try_compat_children() method to specifically call try_compat() on all the nested objects, if any. For instance, PathBeneath objects contain an AccessFs object and they both implement Compatible, which lets them have an independent CompatLevel thanks to set_compatible(). It is not possible to call try_compat() from a try_compat_inner() implementation because they don't return the same result type. Instead, the new try_compat_children() returns the same type as try_compat() and can then call it for all the children. try_compat_children() is only called by try_compat(), which then makes all the compatibility process consistent across different object implementations. Test this fixed behavior with a more complete too_much_access_rights_for_a_file(), initially introduced with a previous commit for the purpose of this fix. As a side effect, the PathBeneath implementation now first checks its allowed_access generic compatibility (with try_compat_children), and then the compatibility with the file type (with try_compat_inner). To better fit with the Compatible contract, replace CompatLevel with Option. None is set by default until users change it. This enables to inherit the parent compatibility level if the current one is not set. Add the TailoredCompatLevel trait with the tailored_compat_level() method to get the current compatibility level accordingly. Automatically implements TailoredCompatLevel for each Compatible implementation (only PathBeneath for now), which helps avoid missing an implementation for a new Compatible types. Signed-off-by: Mickaël Salaün --- src/access.rs | 124 +++++++++++++++++------------- src/compat.rs | 200 +++++++++++++++++++++++++++++++++++++++++++++---- src/fs.rs | 132 ++++++++++++++++++-------------- src/lib.rs | 24 +++++- src/ruleset.rs | 25 +++++-- 5 files changed, 371 insertions(+), 134 deletions(-) diff --git a/src/access.rs b/src/access.rs index 7878968..782f8d1 100644 --- a/src/access.rs +++ b/src/access.rs @@ -1,11 +1,11 @@ use crate::{ - AccessError, AddRuleError, AddRulesError, BitFlags, CompatError, CompatLevel, CompatState, - Compatibility, HandleAccessError, HandleAccessesError, Ruleset, TryCompat, ABI, + AccessError, AddRuleError, AddRulesError, BitFlags, CompatError, CompatResult, + HandleAccessError, HandleAccessesError, Ruleset, TailoredCompatLevel, TryCompat, ABI, }; use enumflags2::BitFlag; #[cfg(test)] -use crate::{make_bitflags, AccessFs}; +use crate::{make_bitflags, AccessFs, CompatLevel, CompatState, Compatibility}; pub trait Access: PrivateAccess { /// Gets the access rights defined by a specific [`ABI`]. @@ -61,57 +61,41 @@ fn bit_flags_full_negation() { assert_ne!(scoped_negation, full_negation(BitFlags::::all())); } -impl TryCompat for BitFlags +impl TailoredCompatLevel for BitFlags where A: Access {} + +impl TryCompat for BitFlags where - T: Access, + A: Access, { - fn try_compat(self, compat: &mut Compatibility) -> Result, CompatError> { - let (state, ret) = if self.is_empty() { + fn try_compat_inner(self, abi: ABI) -> Result, CompatError> { + if self.is_empty() { // Empty access-rights would result to a runtime error. - (CompatState::Dummy, Err(AccessError::Empty.into())) + Err(AccessError::Empty.into()) } else if !Self::all().contains(self) { // Unknown access-rights (at build time) would result to a runtime error. // This can only be reached by using the unsafe BitFlags::from_bits_unchecked(). - ( - CompatState::Dummy, - Err(AccessError::Unknown { - access: self, - unknown: self & full_negation(Self::all()), - } - .into()), - ) + Err(AccessError::Unknown { + access: self, + unknown: self & full_negation(Self::all()), + } + .into()) } else { - let compat_bits = self & T::from_all(compat.abi()); - if compat_bits.is_empty() { - match compat.level { - // Empty access-rights are ignored to avoid an error when passing them to - // landlock_add_rule(). - CompatLevel::BestEffort => (CompatState::No, Ok(None)), - CompatLevel::SoftRequirement => (CompatState::Dummy, Ok(None)), - CompatLevel::HardRequirement => ( - CompatState::Dummy, - Err(AccessError::Incompatible { access: self }.into()), - ), - } - } else if compat_bits != self { - match compat.level { - CompatLevel::BestEffort => (CompatState::Partial, Ok(Some(compat_bits))), - CompatLevel::SoftRequirement => (CompatState::Dummy, Ok(None)), - CompatLevel::HardRequirement => ( - CompatState::Dummy, - Err(AccessError::PartiallyCompatible { - access: self, - incompatible: self & full_negation(compat_bits), - } - .into()), - ), + let compat = self & A::from_all(abi); + if compat.is_empty() { + Ok(CompatResult::No( + AccessError::Incompatible { access: self }.into(), + )) + } else if compat != self { + let error = AccessError::PartiallyCompatible { + access: self, + incompatible: self & full_negation(compat), } + .into(); + Ok(CompatResult::Partial(compat, error)) } else { - (CompatState::Full, Ok(Some(compat_bits))) + Ok(CompatResult::Full(self)) } - }; - compat.update(state); - ret + } } } @@ -125,19 +109,24 @@ fn compat_bit_flags() { let ro_access = make_bitflags!(AccessFs::{Execute | ReadFile | ReadDir}); assert_eq!( ro_access, - ro_access.try_compat(&mut compat).unwrap().unwrap() + ro_access + .try_compat(compat.abi(), compat.level, &mut compat.state) + .unwrap() + .unwrap() ); assert!(compat.state == CompatState::Full); let empty_access = BitFlags::::empty(); assert!(matches!( - empty_access.try_compat(&mut compat).unwrap_err(), + empty_access + .try_compat(compat.abi(), compat.level, &mut compat.state) + .unwrap_err(), CompatError::Access(AccessError::Empty) )); let all_unknown_access = unsafe { BitFlags::::from_bits_unchecked(1 << 63) }; assert!(matches!( - all_unknown_access.try_compat(&mut compat).unwrap_err(), + all_unknown_access.try_compat(compat.abi(), compat.level, &mut compat.state).unwrap_err(), CompatError::Access(AccessError::Unknown { access, unknown }) if access == all_unknown_access && unknown == all_unknown_access )); // An error makes the state final. @@ -145,7 +134,7 @@ fn compat_bit_flags() { let some_unknown_access = unsafe { BitFlags::::from_bits_unchecked(1 << 63 | 1) }; assert!(matches!( - some_unknown_access.try_compat(&mut compat).unwrap_err(), + some_unknown_access.try_compat(compat.abi(), compat.level, &mut compat.state).unwrap_err(), CompatError::Access(AccessError::Unknown { access, unknown }) if access == some_unknown_access && unknown == all_unknown_access )); assert!(compat.state == CompatState::Dummy); @@ -156,16 +145,49 @@ fn compat_bit_flags() { assert!(compat.state == CompatState::No); // Access-rights are valid (but ignored) when they are not required for the current ABI. - assert_eq!(None, ro_access.try_compat(&mut compat).unwrap()); + assert_eq!( + None, + ro_access + .try_compat(compat.abi(), compat.level, &mut compat.state) + .unwrap() + ); // Tests that the ruleset is in an unsupported state, which is important to be able to still // enforce no_new_privs. assert!(compat.state == CompatState::No); // Access-rights are not valid when they are required for the current ABI. - compat.level = CompatLevel::HardRequirement; + compat.level = Some(CompatLevel::HardRequirement); assert!(matches!( - ro_access.try_compat(&mut compat).unwrap_err(), + ro_access.try_compat(compat.abi(), compat.level, &mut compat.state).unwrap_err(), CompatError::Access(AccessError::Incompatible { access }) if access == ro_access )); + + compat = ABI::V1.into(); + + // Tests that the ruleset is marked as the unknown compatibility state. + assert!(compat.state == CompatState::Init); + + // Access-rights are valid (but ignored) when they are not required for the current ABI. + assert_eq!( + ro_access, + ro_access + .try_compat(compat.abi(), compat.level, &mut compat.state) + .unwrap() + .unwrap() + ); + + // Tests that the ruleset is in an unsupported state, which is important to be able to still + // enforce no_new_privs. + assert!(compat.state == CompatState::Full); + + let v2_access = ro_access | AccessFs::Refer; + + // Access-rights are not valid when they are required for the current ABI. + compat.level = Some(CompatLevel::HardRequirement); + assert!(matches!( + v2_access.try_compat(compat.abi(), compat.level, &mut compat.state).unwrap_err(), + CompatError::Access(AccessError::PartiallyCompatible { access, incompatible }) + if access == v2_access && incompatible == AccessFs::Refer + )); } diff --git a/src/compat.rs b/src/compat.rs index 3ca2268..7c2611a 100644 --- a/src/compat.rs +++ b/src/compat.rs @@ -165,10 +165,11 @@ fn current_kernel_abi() { assert_eq!(*TEST_ABI, ABI::new_current()); } +// CompatState is not public outside this crate. /// Returned by ruleset builder. #[cfg_attr(test, derive(Debug))] -#[derive(Copy, Clone, PartialEq)] -pub(crate) enum CompatState { +#[derive(Copy, Clone, PartialEq, Eq)] +pub enum CompatState { /// Initial undefined state. Init, /// All requested restrictions are enforced. @@ -236,10 +237,9 @@ fn compat_state_update_2() { #[cfg_attr(test, derive(Debug, PartialEq))] #[derive(Clone)] -// Compatibility is not public outside this crate. -pub struct Compatibility { +pub(crate) struct Compatibility { abi: ABI, - pub(crate) level: CompatLevel, + pub(crate) level: Option, pub(crate) state: CompatState, } @@ -247,7 +247,7 @@ impl From for Compatibility { fn from(abi: ABI) -> Self { Compatibility { abi, - level: CompatLevel::default(), + level: Default::default(), state: match abi { // Don't forces the state as Dummy because no_new_privs may still be legitimate. ABI::Unsupported => CompatState::No, @@ -258,7 +258,7 @@ impl From for Compatibility { } impl Compatibility { - // Compatibility is an opaque struct. + // Compatibility is a semi-opaque struct. #[allow(clippy::new_without_default)] pub(crate) fn new() -> Self { ABI::new_current().into() @@ -288,7 +288,7 @@ impl Compatibility { /// (e.g. applications carefully designed to only be run with a specific set of kernel features), /// it may be required to error out if some of these features are not available /// and will then not be enforced. -pub trait Compatible: Sized + AsMut { +pub trait Compatible: Sized + AsMut> { // TODO: Update ruleset_handling_renames() with ABI::V3 /// To enable a best-effort security approach, /// Landlock features that are not supported by the running system @@ -393,7 +393,7 @@ pub trait Compatible: Sized + AsMut { /// } /// ``` fn set_compatibility(mut self, level: CompatLevel) -> Self { - *self.as_mut() = level; + *self.as_mut() = Some(level); self } @@ -434,7 +434,8 @@ fn deprecated_set_best_effort() { } /// See the [`Compatible`] documentation. -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +#[cfg_attr(test, derive(EnumIter))] +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum CompatLevel { /// Takes into account the build requests if they are supported by the running system, /// or silently ignores them otherwise. @@ -454,10 +455,181 @@ pub enum CompatLevel { HardRequirement, } +impl From> for CompatLevel { + fn from(opt: Option) -> Self { + match opt { + None => CompatLevel::default(), + Some(ref level) => *level, + } + } +} + +// TailoredCompatLevel could be replaced with AsMut>, but only traits defined +// in the current crate can be implemented for types defined outside of the crate. Furthermore it +// provides a default implementation which is handy for types such as BitFlags. +pub trait TailoredCompatLevel { + fn tailored_compat_level(&mut self, parent_level: L) -> CompatLevel + where + L: Into, + { + parent_level.into() + } +} + +impl TailoredCompatLevel for T +where + Self: Compatible, +{ + // Every Compatible trait implementation returns its own compatibility level, if set. + fn tailored_compat_level(&mut self, parent_level: L) -> CompatLevel + where + L: Into, + { + // Using a mutable reference is not required but it makes the code simpler (no double AsRef + // implementations for each Compatible types), and more importantly it guarantees + // consistency with Compatible::set_compatibility(). + match self.as_mut() { + None => parent_level.into(), + // Returns the most constrained compatibility level. + Some(ref level) => parent_level.into().max(*level), + } + } +} + +#[test] +fn tailored_compat_level() { + use crate::{AccessFs, PathBeneath, PathFd}; + + fn new_path(level: CompatLevel) -> PathBeneath { + PathBeneath::new(PathFd::new("/").unwrap(), AccessFs::Execute).set_compatibility(level) + } + + for parent_level in CompatLevel::iter() { + assert_eq!( + new_path(CompatLevel::BestEffort).tailored_compat_level(parent_level), + parent_level + ); + assert_eq!( + new_path(CompatLevel::HardRequirement).tailored_compat_level(parent_level), + CompatLevel::HardRequirement + ); + } + + assert_eq!( + new_path(CompatLevel::SoftRequirement).tailored_compat_level(CompatLevel::SoftRequirement), + CompatLevel::SoftRequirement + ); + + for child_level in CompatLevel::iter() { + assert_eq!( + new_path(child_level).tailored_compat_level(CompatLevel::BestEffort), + child_level + ); + assert_eq!( + new_path(child_level).tailored_compat_level(CompatLevel::HardRequirement), + CompatLevel::HardRequirement + ); + } +} + +// CompatResult is useful because we don't want to duplicate objects (potentially wrapping a file +// descriptor), and we may not have compatibility errors for some objects. TryCompat::try_compat() +// is responsible to either take T or CompatError according to the compatibility level. +// +// CompatResult is not public outside this crate. +pub enum CompatResult +where + T: TryCompat, + A: Access, +{ + // Fully matches the request. + Full(T), + // Partially matches the request. + Partial(T, CompatError), + // Doesn't matches the request. + No(CompatError), +} + // TryCompat is not public outside this crate. -pub trait TryCompat { - fn try_compat(self, compat: &mut Compatibility) -> Result, CompatError> +pub trait TryCompat +where + Self: Sized + TailoredCompatLevel, + A: Access, +{ + fn try_compat_inner(self, abi: ABI) -> Result, CompatError>; + + // Default implementation for objects without children. + // + // If returning something other than Ok(Some(self)), the implementation must use its own + // compatibility level, if any, with self.tailored_compat_level(default_compat_level), and pass + // it with the abi and compat_state to each child.try_compat(). See PathBeneath implementation + // and the self.allowed_access.try_compat() call. + fn try_compat_children( + self, + _abi: ABI, + _parent_level: L, + _compat_state: &mut CompatState, + ) -> Result, CompatError> where - Self: Sized, - T: Access; + L: Into, + { + Ok(Some(self)) + } + + // Update compat_state and return an error according to try_compat_*() error, or to the + // compatibility level, i.e. either route compatible object or error. + fn try_compat( + mut self, + abi: ABI, + parent_level: L, + compat_state: &mut CompatState, + ) -> Result, CompatError> + where + L: Into, + { + let compat_level = self.tailored_compat_level(parent_level); + let new_self = match self.try_compat_children(abi, compat_level, compat_state)? { + Some(n) => n, + None => return Ok(None), + }; + match new_self.try_compat_inner(abi) { + Ok(CompatResult::Full(new_self)) => { + compat_state.update(CompatState::Full); + Ok(Some(new_self)) + } + Ok(CompatResult::Partial(new_self, error)) => match compat_level { + CompatLevel::BestEffort => { + compat_state.update(CompatState::Partial); + Ok(Some(new_self)) + } + CompatLevel::SoftRequirement => { + compat_state.update(CompatState::Dummy); + Ok(None) + } + CompatLevel::HardRequirement => { + compat_state.update(CompatState::Dummy); + Err(error) + } + }, + Ok(CompatResult::No(error)) => match compat_level { + CompatLevel::BestEffort => { + compat_state.update(CompatState::No); + Ok(None) + } + CompatLevel::SoftRequirement => { + compat_state.update(CompatState::Dummy); + Ok(None) + } + CompatLevel::HardRequirement => { + compat_state.update(CompatState::Dummy); + Err(error) + } + }, + Err(e) => { + // Safeguard to help for test consistency. + compat_state.update(CompatState::Dummy); + Err(e) + } + } + } } diff --git a/src/fs.rs b/src/fs.rs index 416945e..c12f816 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1,7 +1,7 @@ use crate::{ - uapi, Access, AddRuleError, AddRulesError, CompatError, CompatLevel, CompatState, - Compatibility, Compatible, HandleAccessError, HandleAccessesError, PathBeneathError, - PathFdError, PrivateAccess, PrivateRule, Rule, Ruleset, RulesetCreated, RulesetError, + uapi, Access, AddRuleError, AddRulesError, CompatError, CompatLevel, CompatResult, CompatState, + Compatible, HandleAccessError, HandleAccessesError, PathBeneathError, PathFdError, + PrivateAccess, PrivateRule, Rule, Ruleset, RulesetCreated, RulesetError, TailoredCompatLevel, TryCompat, ABI, }; use enumflags2::{bitflags, make_bitflags, BitFlags}; @@ -143,7 +143,11 @@ impl PrivateAccess for AccessFs { // We need to record the requested accesses for PrivateRule::check_consistency(). ruleset.requested_handled_fs |= access; ruleset.actual_handled_fs |= match access - .try_compat(&mut ruleset.compat) + .try_compat( + ruleset.compat.abi(), + ruleset.compat.level, + &mut ruleset.compat.state, + ) .map_err(HandleAccessError::Compat)? { Some(a) => a, @@ -196,7 +200,7 @@ pub struct PathBeneath { // Ties the lifetime of a file descriptor to this object. parent_fd: F, allowed_access: BitFlags, - compat_level: CompatLevel, + compat_level: Option, } impl PathBeneath @@ -218,15 +222,48 @@ where }, parent_fd: parent, allowed_access: access.into(), - compat_level: CompatLevel::default(), + compat_level: None, } } - // Check with our own support requirement. - fn filter_access( + fn sync_attr(mut self) -> Self { + // Synchronizes rule attributes. + self.attr.allowed_access = self.allowed_access.bits(); + self + } +} + +impl TryCompat for PathBeneath +where + F: AsFd, +{ + fn try_compat_children( + mut self, + abi: ABI, + parent_level: L, + compat_state: &mut CompatState, + ) -> Result, CompatError> + where + L: Into, + { + // Checks with our own compatibility level, if any. + self.allowed_access = match self.allowed_access.try_compat( + abi, + self.tailored_compat_level(parent_level), + compat_state, + )? { + Some(a) => a, + None => return Ok(None), + }; + Ok(Some(self)) + } + + fn try_compat_inner( mut self, - compat: &mut Compatibility, - ) -> Result, PathBeneathError> { + _abi: ABI, + ) -> Result, CompatError> { + // self.attr.allowed_access was updated with try_compat_children(), called by try_compat(). + // Gets subset of valid accesses according the FD type. let valid_access = if is_file(&self.parent_fd).map_err(|e| PathBeneathError::StatCall { source: e })? { @@ -236,39 +273,17 @@ where }; if self.allowed_access != valid_access { - self.allowed_access = match self.compat_level { - CompatLevel::BestEffort => valid_access, - CompatLevel::SoftRequirement => { - compat.update(CompatState::Dummy); - return Ok(None); - } - CompatLevel::HardRequirement => { - // Linux would return EINVAL. - return Err(PathBeneathError::DirectoryAccess { - access: self.allowed_access, - incompatible: self.allowed_access ^ valid_access, - }); - } + let error = PathBeneathError::DirectoryAccess { + access: self.allowed_access, + incompatible: self.allowed_access ^ valid_access, } + .into(); + self.allowed_access = valid_access; + // Linux would return EINVAL. + Ok(CompatResult::Partial(self.sync_attr(), error)) + } else { + Ok(CompatResult::Full(self.sync_attr())) } - Ok(Some(self)) - } -} - -impl TryCompat for PathBeneath -where - F: AsFd, -{ - fn try_compat(self, compat: &mut Compatibility) -> Result, CompatError> { - let mut filtered = match self.filter_access(compat)? { - Some(f) => f, - None => return Ok(None), - }; - filtered.attr.allowed_access = match filtered.allowed_access.try_compat(compat)? { - Some(f) => f.bits(), - None => return Ok(None), - }; - Ok(Some(filtered)) } } @@ -276,29 +291,28 @@ where fn path_beneath_try_compat() { use crate::*; - let compat: Compatibility = ABI::V1.into(); + let abi = ABI::V1; for file in &["/etc/passwd", "/dev/null"] { - let mut compat_copy = compat.clone(); + // TODO: test try_compat_children + + let mut compat_state = CompatState::Init; let ro_access = AccessFs::ReadDir | AccessFs::ReadFile; assert!(matches!( PathBeneath::new(PathFd::new(file).unwrap(), ro_access) - .set_compatibility(CompatLevel::HardRequirement) - .try_compat(&mut compat_copy) + .try_compat(abi, CompatLevel::HardRequirement, &mut compat_state) .unwrap_err(), CompatError::PathBeneath(PathBeneathError::DirectoryAccess { access, incompatible }) if access == ro_access && incompatible == AccessFs::ReadDir )); - // compat_copy.state is not consistent when an error occurs. - let mut compat_copy = compat.clone(); + let mut compat_state = CompatState::Init; assert!(matches!( PathBeneath::new(PathFd::new(file).unwrap(), BitFlags::EMPTY) - .try_compat(&mut compat_copy) + .try_compat(abi, CompatLevel::BestEffort, &mut compat_state) .unwrap_err(), CompatError::Access(AccessError::Empty) )); - // compat_copy.state is not consistent when an error occurs. } let full_access = AccessFs::from_all(ABI::V1); @@ -307,21 +321,20 @@ fn path_beneath_try_compat() { CompatLevel::SoftRequirement, CompatLevel::HardRequirement, ] { - let mut compat_copy = compat.clone(); + let mut compat_state = CompatState::Init; let raw_access = PathBeneath::new(PathFd::new("/").unwrap(), full_access) - .set_compatibility(*compat_level) - .try_compat(&mut compat_copy) + .try_compat(abi, *compat_level, &mut compat_state) .unwrap() .unwrap() .attr .allowed_access; assert_eq!(raw_access, full_access.bits()); - assert_eq!(compat_copy.state, CompatState::Full); + assert_eq!(compat_state, CompatState::Full); } } -impl AsMut for PathBeneath { - fn as_mut(&mut self) -> &mut CompatLevel { +impl AsMut> for PathBeneath { + fn as_mut(&mut self) -> &mut Option { &mut self.compat_level } } @@ -335,10 +348,15 @@ fn path_beneath_compatibility() { let mut path = PathBeneath::new(PathFd::new("/").unwrap(), AccessFs::from_all(ABI::V1)); let path_ref = &mut path; - assert_eq!(path_ref.as_mut(), &CompatLevel::BestEffort); + let level = path_ref.as_mut(); + assert_eq!(level, &None); + assert_eq!( + as Into>::into(*level), + CompatLevel::BestEffort + ); path_ref.set_compatibility(CompatLevel::SoftRequirement); - assert_eq!(path_ref.as_mut(), &CompatLevel::SoftRequirement); + assert_eq!(path_ref.as_mut(), &Some(CompatLevel::SoftRequirement)); path.set_compatibility(CompatLevel::HardRequirement); } diff --git a/src/lib.rs b/src/lib.rs index 120e0ff..c0a37cd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -93,7 +93,7 @@ pub use ruleset::{ }; use access::PrivateAccess; -use compat::{CompatState, Compatibility, TryCompat}; +use compat::{CompatResult, CompatState, Compatibility, TailoredCompatLevel, TryCompat}; use ruleset::PrivateRule; #[cfg(test)] @@ -203,15 +203,31 @@ mod tests { // Same code as allow_root_compat() but with /etc/passwd instead of / .add_rule(PathBeneath::new( PathFd::new("/etc/passwd")?, - // TODO: Only allow legitimate access rights on a file. - AccessFs::from_all(abi), + // Only allow legitimate access rights on a file. + AccessFs::from_file(abi), ))? .restrict_self()?) }, false, ); - // TODO: Fix and test partial compatibility with never-fully-supported ruleset. + check_ruleset_support( + abi, + None, + |ruleset: Ruleset| -> _ { + Ok(ruleset + .handle_access(AccessFs::from_all(abi))? + .create()? + // Same code as allow_root_compat() but with /etc/passwd instead of / + .add_rule(PathBeneath::new( + PathFd::new("/etc/passwd")?, + // Tries to allow all access rights on a file. + AccessFs::from_all(abi), + ))? + .restrict_self()?) + }, + false, + ); } #[test] diff --git a/src/ruleset.rs b/src/ruleset.rs index 3faedbc..6f7cff9 100644 --- a/src/ruleset.rs +++ b/src/ruleset.rs @@ -18,8 +18,9 @@ where } // PrivateRule is not public outside this crate. -pub trait PrivateRule: TryCompat +pub trait PrivateRule where + Self: TryCompat + Compatible, T: Access, { fn as_ptr(&self) -> *const libc::c_void; @@ -242,7 +243,7 @@ impl Ruleset { // Checks that the ruleset handles at least one access. if self.actual_handled_fs.is_empty() { - match self.compat.level { + match self.compat.level.into() { CompatLevel::BestEffort => { self.compat.update(CompatState::No); } @@ -277,8 +278,8 @@ impl Ruleset { } } -impl AsMut for Ruleset { - fn as_mut(&mut self) -> &mut CompatLevel { +impl AsMut> for Ruleset { + fn as_mut(&mut self) -> &mut Option { &mut self.compat.level } } @@ -365,8 +366,8 @@ fn ruleset_created_handle_access_or() { )); } -impl AsMut for RulesetCreated { - fn as_mut(&mut self) -> &mut CompatLevel { +impl AsMut> for RulesetCreated { + fn as_mut(&mut self) -> &mut Option { &mut self.compat.level } } @@ -388,7 +389,11 @@ pub trait RulesetCreatedAttr: Sized + AsMut + Compatible { let self_ref = self.as_mut(); rule.check_consistency(self_ref)?; let compat_rule = match rule - .try_compat(&mut self_ref.compat) + .try_compat( + self_ref.compat.abi(), + self_ref.compat.level, + &mut self_ref.compat.state, + ) .map_err(AddRuleError::Compat)? { Some(r) => r, @@ -546,11 +551,15 @@ impl RulesetCreated { /// On error, returns a wrapped [`RestrictSelfError`]. pub fn restrict_self(mut self) -> Result { let mut body = || -> Result { + // FIXME: Enforce no_new_privs even if something failed with SoftRequirement. The + // rationale is that no_new_privs should not be an issue on its own if it is not + // explicitly deactivated. + // // Ignores prctl_set_no_new_privs() if an error was encountered with // CompatLevel::SoftRequirement set. let enforced_nnp = if self.compat.state != CompatState::Dummy && self.no_new_privs { if let Err(e) = prctl_set_no_new_privs() { - match self.compat.level { + match self.compat.level.into() { CompatLevel::BestEffort => {} CompatLevel::SoftRequirement => { self.compat.update(CompatState::Dummy);