Fix recursion-check in alias table (#993)

src/sudoers/entry.rs

@@ -3,6 +3,7 @@ use core::fmt;
 use crate::sudoers::{
     ast::{Identifier, Qualified, UserSpecifier},
     tokens::{ChDir, Meta},
+    VecOrd,
 };
 use crate::{
     common::{resolve::CurrentUser, SudoString},
@@ -21,14 +22,14 @@ mod verbose;
 pub struct Entry<'a> {
     run_as: Option<&'a RunAs>,
     cmd_specs: Vec<(Tag, &'a Qualified<Meta<Command>>)>,
-    cmd_alias: &'a [Def<Command>],
+    cmd_alias: &'a VecOrd<Def<Command>>,
 }
 
 impl<'a> Entry<'a> {
     pub(super) fn new(
         run_as: Option<&'a RunAs>,
         cmd_specs: Vec<(Tag, &'a Qualified<Meta<Command>>)>,
-        cmd_alias: &'a [Def<Command>],
+        cmd_alias: &'a VecOrd<Def<Command>>,
     ) -> Self {
         debug_assert!(!cmd_specs.is_empty());
 
@@ -89,7 +90,10 @@ impl fmt::Display for Entry<'_> {
 
             write_tag(f, tag, last_tag)?;
             last_tag = Some(tag);
-            write_spec(f, spec, cmd_alias, true, ", ")?;
+
+            // cmd_alias is to be topologically sorted (dependencies come before dependents),
+            // the argument to write_spec needs to have dependents before dependencies.
+            write_spec(f, spec, cmd_alias.iter().rev(), true, ", ")?;
         }
 
         Ok(())
@@ -218,10 +222,10 @@ fn write_tag(f: &mut fmt::Formatter, tag: &Tag, last_tag: Option<&Tag>) -> fmt::
     Ok(())
 }
 
-fn write_spec(
+fn write_spec<'a>(
     f: &mut fmt::Formatter,
     spec: &Qualified<Meta<Command>>,
-    alias_list: &[Def<Command>],
+    mut alias_list: impl Iterator<Item = &'a Def<Command>> + Clone,
     mut sign: bool,
     separator: &str,
 ) -> fmt::Result {
@@ -250,14 +254,17 @@ fn write_spec(
             }
         }
         Meta::Alias(alias) => {
-            // this will terminate, since AliasTable has been checked by sanitize_alias_table
-            if let Some(Def(_, spec_list)) = alias_list.iter().find(|Def(id, _)| id == alias) {
+            if let Some(Def(_, spec_list)) = alias_list.find(|Def(id, _)| id == alias) {
                 let mut is_first_iteration = true;
                 for spec in spec_list {
                     if !is_first_iteration {
                         f.write_str(separator)?;
                     }
-                    write_spec(f, spec, alias_list, sign, separator)?;
+                    // 1) this recursion will terminate, since "alias_list" has become smaller
+                    //    by the "alias_list.find()" above
+                    // 2) to get the correct macro expansion, alias_list has to be (reverse-)topologically
+                    //    sorted so that "later" definitions do not refer back to "earlier" definitons.
+                    write_spec(f, spec, alias_list.clone(), sign, separator)?;
                     is_first_iteration = false;
                 }
             } else {

src/sudoers/entry/verbose.rs

@@ -35,7 +35,7 @@ impl fmt::Display for Verbose<'_> {
             last_tag = Some(tag);
 
             f.write_str("\n\t")?;
-            super::write_spec(f, cmd_spec, cmd_alias, true, "\n\t")?;
+            super::write_spec(f, cmd_spec, cmd_alias.iter().rev(), true, "\n\t")?;
         }
 
         Ok(())

src/sudoers/mod.rs

@@ -178,7 +178,7 @@ impl Sudoers {
     ) -> impl Iterator<Item = Entry<'a>> {
         let user_specs = self.matching_user_specs(invoking_user, hostname);
 
-        user_specs.flat_map(|cmd_specs| group_cmd_specs_per_runas(cmd_specs, &self.aliases.cmnd.1))
+        user_specs.flat_map(|cmd_specs| group_cmd_specs_per_runas(cmd_specs, &self.aliases.cmnd))
     }
 
     pub(crate) fn solve_editor_path(&self) -> Option<PathBuf> {
@@ -214,7 +214,7 @@ fn peeking_take_while<'a, T>(
 
 fn group_cmd_specs_per_runas<'a>(
     cmnd_specs: impl Iterator<Item = (Option<&'a RunAs>, (Tag, &'a Spec<Command>))>,
-    cmnd_aliases: &'a [Def<Command>],
+    cmnd_aliases: &'a VecOrd<Def<Command>>,
 ) -> impl Iterator<Item = Entry<'a>> {
     // `distribute_tags` will have given every spec a reference to the "runas specification"
     // that applies to it. The output of sudo --list splits the CmndSpec list based on that:
@@ -282,7 +282,7 @@ impl<T> Default for VecOrd<T> {
 }
 
 impl<T> VecOrd<T> {
-    fn iter(&self) -> impl Iterator<Item = &T> {
+    fn iter(&self) -> impl DoubleEndedIterator<Item = &T> + Clone {
         self.0.iter().map(|&i| &self.1[i])
     }
 }
@@ -707,7 +707,7 @@ fn sanitize_alias_table<T>(table: &Vec<Def<T>>, diagnostics: &mut Vec<Error>) ->
                 let Def(_, members) = &self.table[pos];
                 for elem in members {
                     let Meta::Alias(name) = remqualify(elem) else {
-                        break;
+                        continue;
                     };
                     let Some(dependency) = self.table.iter().position(|Def(id, _)| id == name)
                     else {

src/sudoers/test/mod.rs

@@ -431,6 +431,16 @@ fn useralias_underscore_regression() {
         .is_alias());
 }
 
+#[test]
+fn regression_check_recursion() {
+    let (_, error) = analyze(
+        Path::new("/etc/fakesudoers"),
+        sudoer!["User_Alias A=user, B", "User_Alias B=A"],
+    );
+
+    assert!(!error.is_empty());
+}
+
 fn test_topo_sort(n: usize) {
     let alias = |s: &str| Qualified::Allow(Meta::<UserSpecifier>::Alias(s.to_string()));
     let stop = || Qualified::Allow(Meta::<UserSpecifier>::All);

src/sudoers/tokens.rs

@@ -142,11 +142,11 @@ impl Token for AliasName {
     }
 
     fn accept_1st(c: char) -> bool {
-        c.is_ascii_uppercase() || c.is_ascii_digit()
+        c.is_ascii_uppercase()
     }
 
     fn accept(c: char) -> bool {
-        Self::accept_1st(c) || c == '_'
+        Self::accept_1st(c) || c.is_ascii_digit() || c == '_'
     }
 }