Skip to content

Commit

Permalink
Merge pull request #843 from memorysafety/visudo-trunc
Browse files Browse the repository at this point in the history 
Fix visudo bugs; update the CI
  • Loading branch information
@pvdrz
pvdrz authored Jun 25, 2024
2 parents 90e2385 + 13e7dcc commit 3b09b4c
Show file tree
Hide file tree
Showing 9 changed files with 153 additions and 171 deletions.
3 changes: 3 additions & 0 deletions src/defaults/settings_dsl.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
macro_rules! add_from {
($ctor:ident, $type:ty) => {
#[allow(non_local_definitions)]

Check warning on line 3 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`

Check warning on line 3 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`

Check warning on line 3 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`

Check warning on line 3 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`
impl From<$type> for $crate::defaults::SudoDefault {
fn from(value: $type) -> Self {
$crate::defaults::SudoDefault::$ctor(value.into())
Expand All @@ -8,6 +9,7 @@ macro_rules! add_from {
};

($ctor:ident, $type:ty, negatable$(, $vetting_function:expr)?) => {
#[allow(non_local_definitions)]

Check warning on line 12 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`

Check warning on line 12 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`
impl From<$type> for $crate::defaults::SudoDefault {
fn from(value: $type) -> Self {
$crate::defaults::SudoDefault::$ctor(OptTuple {
Expand All @@ -17,6 +19,7 @@ macro_rules! add_from {
}
}

#[allow(non_local_definitions)]

Check warning on line 22 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`

Check warning on line 22 in src/defaults/settings_dsl.rs

View workflow job for this annotation

GitHub Actions / build-and-test-msrv 

unknown lint: `non_local_definitions`
impl From<($type, $type)> for $crate::defaults::SudoDefault {
fn from((value, neg): ($type, $type)) -> Self {
$crate::defaults::SudoDefault::$ctor(OptTuple {
Expand Down
3 changes: 3 additions & 0 deletions src/sudo/cli/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ pub mod help;
#[cfg(test)]
mod tests;

// remove dead_code when sudoedit has been implemented
#[allow(dead_code)]
pub enum SudoAction {
Edit(SudoEditOptions),
Help(SudoHelpOptions),
Expand Down Expand Up @@ -205,6 +207,7 @@ impl TryFrom<SudoOptions> for SudoValidateOptions {
}

// sudo -e [-ABkNnS] [-r role] [-t type] [-C num] [-D directory] [-g group] [-h host] [-p prompt] [-R directory] [-T timeout] [-u user] file ...
#[allow(dead_code)]
pub struct SudoEditOptions {
// -k
pub reset_timestamp: bool,
Expand Down
3 changes: 3 additions & 0 deletions src/sudo/env/environment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ fn is_safe_tz(value: &[u8]) -> bool {
};

if check_value.starts_with(&[b'/']) {
// clippy 1.79 wants to us to optimise this check away; but we don't know what this will always
// be possible; and the compiler is clever enough to do that for us anyway if it can be.
#[allow(clippy::const_is_empty)]
if !PATH_ZONEINFO.is_empty() {
if !check_value.starts_with(PATH_ZONEINFO.as_bytes())
|| check_value.get(PATH_ZONEINFO.len()) != Some(&b'/')
Expand Down
4 changes: 0 additions & 4 deletions src/sudo/env/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,6 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
let current_group = Group {
gid: 1000,
name: "test".to_string(),
passwd: String::new(),
members: Vec::new(),
};

let root_user = User {
Expand All @@ -114,8 +112,6 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
let root_group = Group {
gid: 0,
name: "root".to_string(),
passwd: String::new(),
members: Vec::new(),
};

Context {
Expand Down
2 changes: 1 addition & 1 deletion src/sudo/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use pipeline::{Pipeline, PolicyPlugin};
use std::path::Path;

mod cli;
mod diagnostic;
pub mod diagnostic;
mod env;
mod pam;
mod pipeline;
Expand Down
259 changes: 129 additions & 130 deletions src/sudoers/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,160 +590,159 @@ fn analyze(
}
}

impl Sudoers {
fn include(
&mut self,
parent: &Path,
path: &Path,
diagnostics: &mut Vec<Error>,
count: &mut u8,
) {
if *count >= INCLUDE_LIMIT {
diagnostics.push(Error {
source: Some(parent.to_owned()),
location: None,
message: format!("include file limit reached opening '{}'", path.display()),
})
// FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
// that includes another non-privileged sudoer files.
} else {
match open_subsudoers(path) {
Ok(subsudoer) => {
*count += 1;
self.process(path, subsudoer, diagnostics, count)
}
Err(e) => {
let message = if e.kind() == io::ErrorKind::NotFound {
// improve the error message in this case
format!("cannot open sudoers file '{}'", path.display())
} else {
e.to_string()
};
fn include(
cfg: &mut Sudoers,
parent: &Path,
path: &Path,
diagnostics: &mut Vec<Error>,
count: &mut u8,
) {
if *count >= INCLUDE_LIMIT {
diagnostics.push(Error {
source: Some(parent.to_owned()),
location: None,
message: format!("include file limit reached opening '{}'", path.display()),
})
// FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
// that includes another non-privileged sudoer files.
} else {
match open_subsudoers(path) {
Ok(subsudoer) => {
*count += 1;
process(cfg, path, subsudoer, diagnostics, count)
}
Err(e) => {
let message = if e.kind() == io::ErrorKind::NotFound {
// improve the error message in this case
format!("cannot open sudoers file '{}'", path.display())
} else {
e.to_string()
};

diagnostics.push(Error {
source: Some(parent.to_owned()),
location: None,
message,
})
}
diagnostics.push(Error {
source: Some(parent.to_owned()),
location: None,
message,
})
}
}
}
}

fn process(
&mut self,
cur_path: &Path,
sudoers: impl IntoIterator<Item = basic_parser::Parsed<Sudo>>,
diagnostics: &mut Vec<Error>,
safety_count: &mut u8,
) {
for item in sudoers {
match item {
Ok(line) => match line {
Sudo::LineComment => {}

Sudo::Spec(permission) => self.rules.push(permission),

Sudo::Decl(UserAlias(mut def)) => self.aliases.user.1.append(&mut def),
Sudo::Decl(HostAlias(mut def)) => self.aliases.host.1.append(&mut def),
Sudo::Decl(CmndAlias(mut def)) => self.aliases.cmnd.1.append(&mut def),
Sudo::Decl(RunasAlias(mut def)) => self.aliases.runas.1.append(&mut def),

Sudo::Decl(Defaults(params)) => {
for (name, value) in params {
self.set_default(name, value)
}
fn process(
cfg: &mut Sudoers,
cur_path: &Path,
sudoers: impl IntoIterator<Item = basic_parser::Parsed<Sudo>>,
diagnostics: &mut Vec<Error>,
safety_count: &mut u8,
) {
for item in sudoers {
match item {
Ok(line) => match line {
Sudo::LineComment => {}

Sudo::Spec(permission) => cfg.rules.push(permission),

Sudo::Decl(UserAlias(mut def)) => cfg.aliases.user.1.append(&mut def),
Sudo::Decl(HostAlias(mut def)) => cfg.aliases.host.1.append(&mut def),
Sudo::Decl(CmndAlias(mut def)) => cfg.aliases.cmnd.1.append(&mut def),
Sudo::Decl(RunasAlias(mut def)) => cfg.aliases.runas.1.append(&mut def),

Sudo::Decl(Defaults(params)) => {
for (name, value) in params {
set_default(cfg, name, value)
}
}

Sudo::Include(path) => self.include(
cur_path,
&resolve_relative(cur_path, path),
diagnostics,
safety_count,
),

Sudo::IncludeDir(path) => {
if path.contains("%h") {
diagnostics.push(Error{
Sudo::Include(path) => include(
cfg,
cur_path,
&resolve_relative(cur_path, path),
diagnostics,
safety_count,
),

Sudo::IncludeDir(path) => {
if path.contains("%h") {
diagnostics.push(Error{
source: Some(cur_path.to_owned()),
location: None,
message: format!("cannot open sudoers file {path}: percent escape %h in includedir is unsupported")});
continue;
}

let path = resolve_relative(cur_path, path);
let Ok(files) = std::fs::read_dir(&path) else {
diagnostics.push(Error {
source: Some(cur_path.to_owned()),
location: None,
message: format!("cannot open sudoers file {}", path.display()),
});
continue;
};
let mut safe_files = files
.filter_map(|direntry| {
let path = direntry.ok()?.path();
let text = path.file_name()?.to_str()?;
if text.ends_with('~') || text.contains('.') {
None
} else {
Some(path)
}
})
.collect::<Vec<_>>();
safe_files.sort();
for file in safe_files {
self.include(cur_path, file.as_ref(), diagnostics, safety_count)
}
continue;
}
},

Err(basic_parser::Status::Fatal(pos, message)) => diagnostics.push(Error {
source: Some(cur_path.to_owned()),
location: Some(pos),
message,
}),
Err(_) => panic!("internal parser error"),
}
let path = resolve_relative(cur_path, path);
let Ok(files) = std::fs::read_dir(&path) else {
diagnostics.push(Error {
source: Some(cur_path.to_owned()),
location: None,
message: format!("cannot open sudoers file {}", path.display()),
});
continue;
};
let mut safe_files = files
.filter_map(|direntry| {
let path = direntry.ok()?.path();
let text = path.file_name()?.to_str()?;
if text.ends_with('~') || text.contains('.') {
None
} else {
Some(path)
}
})
.collect::<Vec<_>>();
safe_files.sort();
for file in safe_files {
include(cfg, cur_path, file.as_ref(), diagnostics, safety_count)
}
}
},

Err(basic_parser::Status::Fatal(pos, message)) => diagnostics.push(Error {
source: Some(cur_path.to_owned()),
location: Some(pos),
message,
}),
Err(_) => panic!("internal parser error"),
}
}
}

fn set_default(&mut self, name: String, value: ConfigValue) {
match value {
Flag(value) => {
if value {
self.settings.flags.insert(name);
} else {
self.settings.flags.remove(&name);
}
fn set_default(cfg: &mut Sudoers, name: String, value: ConfigValue) {
match value {
Flag(value) => {
if value {
cfg.settings.flags.insert(name);
} else {
cfg.settings.flags.remove(&name);
}
List(mode, values) => {
let slot: &mut _ = self.settings.list.entry(name).or_default();
match mode {
Mode::Set => *slot = values.into_iter().collect(),
Mode::Add => slot.extend(values),
Mode::Del => {
for key in values {
slot.remove(&key);
}
}
List(mode, values) => {
let slot: &mut _ = cfg.settings.list.entry(name).or_default();
match mode {
Mode::Set => *slot = values.into_iter().collect(),
Mode::Add => slot.extend(values),
Mode::Del => {
for key in values {
slot.remove(&key);
}
}
}
Text(value) => {
self.settings.str_value.insert(name, value);
}
Enum(value) => {
self.settings.enum_value.insert(name, value);
}
Num(value) => {
self.settings.int_value.insert(name, value);
}
}
Text(value) => {
cfg.settings.str_value.insert(name, value);
}
Enum(value) => {
cfg.settings.enum_value.insert(name, value);
}
Num(value) => {
cfg.settings.int_value.insert(name, value);
}
}
}

let mut diagnostics = vec![];
result.process(path, sudoers, &mut diagnostics, &mut 0);
process(&mut result, path, sudoers, &mut diagnostics, &mut 0);

let alias = &mut result.aliases;
alias.user.0 = sanitize_alias_table(&alias.user.1, &mut diagnostics);
Expand Down
Loading

0 comments on commit 3b09b4c

Please sign in to comment.