From e1cc239b51901695ec05356263fc77c46dfdd20d Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <56512186+LocalSpook@users.noreply.github.com> Date: Mon, 25 Dec 2023 01:07:52 -0700 Subject: [PATCH] Use final matching rule when determining if a command is permitted Before, a command was permitted only if there were no other rules denying it *anywhere* in the file. This is inconsistent with the original doas. As a side effect of the changes, this commit fixes another inconsistency where the original doas did not allow using the `nopass` option with `deny` rules, but rsudoas did. --- src/lib.rs | 112 +++++++++++++++++----------------------------------- src/main.rs | 44 ++++++++++++--------- 2 files changed, 63 insertions(+), 93 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 2a1dfdb..3551057 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,14 +159,14 @@ pub mod command { pub mod config { use std::collections::HashMap; - #[derive(Clone, Debug)] + #[derive(Clone, Debug, PartialEq, Eq)] pub enum RuleAction { - Permit, Deny, + PermitWithPassword, + PermitWithoutPassword, } #[derive(Clone, Debug)] pub struct RuleOpts { - pub nopass: bool, pub nolog: bool, pub persist: bool, pub keepenv: bool, @@ -188,70 +188,45 @@ pub mod config { pub args: Option>, } - #[derive(Clone, Debug)] - pub struct Rules { - pub allowed: Vec, - pub denied: Vec, - } - - impl Rules { - pub fn r#match<'a>(&self, + impl Rule { + pub fn matches(&self, user: &str, - groups: &'a [impl AsRef], + groups: &[impl AsRef], cmd: &str, - args: &'a [impl AsRef], + args: &[impl AsRef], target: &str, - ) -> Option { - let match_rules = |rules: &[Rule]| -> Option { - let mut matched: Option<&Rule> = None; - for rule in rules { - match &rule.identity { - RuleIdentity::User(rule_user) => if rule_user != user { - continue; - }, - RuleIdentity::Group(group) => if !groups.iter().any(|x| x.as_ref() == group) { - continue; - }, - } - if let Some(rule_cmd) = &rule.command { - if rule_cmd != cmd { - continue; - } - } - if let Some(rule_args) = &rule.args { - if rule_args.iter().zip(args).all(|(x, y)| x == y.as_ref()) { - continue; - } - } - if let Some(rule_target) = &rule.target { - if rule_target != target { - continue; - } - } - matched = Some(rule); + ) -> bool { + match &self.identity { + RuleIdentity::User(rule_user) => if rule_user != user { + return false; + }, + RuleIdentity::Group(group) => if !groups.iter().any(|x| x.as_ref() == group) { + return false; + }, + } + if let Some(rule_cmd) = &self.command { + if rule_cmd != cmd { + return false; } - match matched { - Some(rule) => Some(rule.options.clone()), - None => None, + } + if let Some(rule_args) = &self.args { + if rule_args.iter().zip(args).all(|(x, y)| x == y.as_ref()) { + return false; + } + } + if let Some(rule_target) = &self.target { + if rule_target != target { + return false; } - }; - let matched = match_rules(&self.allowed); - if matched.is_some() && match_rules(&self.denied).is_none() { - matched - } else { - None } + true } } - impl TryFrom<&str> for Rules { - type Error = String; - - fn try_from(config: &str) -> Result { + pub fn parse_into_rules(config: &str) -> Result, String> { const DEFAULT_RULE: Rule = Rule { action: RuleAction::Deny, options: RuleOpts { - nopass: false, nolog: false, persist: false, keepenv: false, @@ -263,10 +238,7 @@ pub mod config { args: None, }; - let mut rules = Rules { - allowed: Vec::new(), - denied: Vec::new(), - }; + let mut rules = Vec::new(); enum Expect { Permission, @@ -278,13 +250,6 @@ pub mod config { let mut state = Expect::Permission; let mut new_rule = false; let mut rule: Rule = DEFAULT_RULE.clone(); - let mut push_rule = |rule: &mut Rule| { - let value = std::mem::replace(rule, DEFAULT_RULE.clone()); - match value.action { - RuleAction::Permit => rules.allowed.push(value), - RuleAction::Deny => rules.denied.push(value), - }; - }; while let Some((token, pure)) = tokens.next() { if new_rule { rule = DEFAULT_RULE.clone(); @@ -295,7 +260,7 @@ pub mod config { Expect::Permission => { if pure { if token == "permit" { - rule.action = RuleAction::Permit; + rule.action = RuleAction::PermitWithPassword; } else if token == "deny" { rule.action = RuleAction::Deny; } else { @@ -308,7 +273,10 @@ pub mod config { }, Expect::OptionOrIdentity => { match (&*token, pure) { - ("nopass", true) => rule.options.nopass = true, + ("nopass", true) => match rule.action { + RuleAction::Deny => return Err("The nopass option cannot be used with the deny action".to_string()), + _ => rule.action = RuleAction::PermitWithoutPassword, + }, ("nolog", true) => rule.options.nolog = true, ("persist", true) => rule.options.persist = true, ("keepenv", true) => rule.options.keepenv = true, @@ -352,16 +320,11 @@ pub mod config { }, } if tokens.line_ended { - push_rule(&mut rule); + rules.push(std::mem::replace(&mut rule, DEFAULT_RULE.clone())); new_rule = true; } } - if !new_rule { - // Push the leftover rule (happens when config doesn't end with a NL) - push_rule(&mut rule); - } - return Ok(rules); fn parse_env<'a>(tokens: &mut Tokenizer<'a>) -> Result, String> { @@ -470,5 +433,4 @@ pub mod config { } } } - } } diff --git a/src/main.rs b/src/main.rs index b3998e6..a9b52ae 100644 --- a/src/main.rs +++ b/src/main.rs @@ -57,8 +57,9 @@ fn execute(opts: Execute) { config_file = file; }, } - let config = std::fs::read_to_string(config_file).expect("Failed to read config"); - let rules = match Rules::try_from(&*config) { + // Make sure the config file ends with a newline. + let config = std::fs::read_to_string(config_file).expect("Failed to read config") + "\n"; + let rules = match config::parse_into_rules(&config) { Ok(x) => x, Err(_) => print_error_and_exit("Error parsing config", 1), }; @@ -79,32 +80,39 @@ fn execute(opts: Execute) { Some(x) => x, None => passwd_target.shell.clone(), }; - let matched = rules.r#match(user, &groups, &*cmd, &opts.args, &opts.user); + let matching_rule = rules.into_iter().rev().find(|rule| + rule.matches(user, &groups, &cmd, &opts.args, &opts.user) + ); + let action = match matching_rule { + None => RuleAction::Deny, + Some(ref rule) => rule.action.clone(), + }; + if only_check { - match matched { - None => println!("deny"), - Some(rule_opts) => { - println!("permit{}", if rule_opts.nopass {" nopass"} else {""}); - }, + match action { + RuleAction::Deny => println!("deny"), + RuleAction::PermitWithPassword => println!("permit"), + RuleAction::PermitWithoutPassword => println!("permit nopass"), } return; } + + if action == RuleAction::Deny { + let cmdline = get_cmdline(&cmd, &opts.args); + let msg = format!("command not permitted for {}: {}", &user, &cmdline); + syslog(libc::LOG_AUTHPRIV | libc::LOG_NOTICE, &msg); + print_error_and_exit("Not permitted", 1); + } - let rule_opts = match matched { - None => { - let cmdline = get_cmdline(&cmd, &opts.args); - let msg = format!("command not permitted for {}: {}", &user, &cmdline); - syslog(libc::LOG_AUTHPRIV | libc::LOG_NOTICE, &msg); - print_error_and_exit("Not permitted", 1); - }, - Some(match_opts) => match_opts, - }; - if !rule_opts.nopass { + if action == RuleAction::PermitWithPassword { if !challenge_user(&passwd) { eprintln!("Authentication failed"); return; } } + + let rule_opts = matching_rule.unwrap().options; + if !rule_opts.nolog { let cmdline = get_cmdline(&cmd, &opts.args); let cwd = env::current_dir();