From d69d4d3ddf37d3a997deba58cbccee64f4a6869b Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 11 Nov 2024 20:52:19 +0000 Subject: [PATCH] Merge parser: bug fix do not change auditing information when applying deny The parser recently changed how/where deny information is applied. commit 1fa45b7c1 ("parser: dfa minimization prepare for extended permissions") removed the implicit filtering of explicit denies during the minimization pass. The implicit clear allowed the explicit information to be carried into the minimization pass and merged with implicit denies. The end result being a minimized dfa with the explicit deny information available to be applied post minimization, and then dropped later at permission encoding in the accept entries. Extended permission however enable carrying explicit deny information into the kernel to fix certain bugs like complain mode not being able to distinguish between implicit and explicit deny rules (ie. deny rules get ignored in complain mode). However keeping explicit deny information when unnecessary result in a larger state machine than necessary and slower compiles. commit 179c1c1ba ("parser: fix minimization check for filtering_deny") Moved the explicit apply_and_clear_deny() pass to before minimization to restore mnimization's ability to create a minimized dfa with explicit and implicit deny information merged but this also cleared the explicit deny information that used to be carried through minimization. This meant that when the deny information was applied post minimization it resulted in the audit and quiet information being cleared. This resulted in the query_label tests failing as they are checking for the expected audit infomation in the permissions. Fixes: 179c1c1ba ("parser: fix minimization check for filtering_deny") Bug: https://gitlab.com/apparmor/apparmor/-/issues/461 Signed-off-by: John Johansen MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1408 Approved-by: Ryan Lee Approved-by: John Johansen Merged-by: John Johansen (cherry picked from commit eb365b374df7b85c13a0119063a807743771bf5a) Signed-off-by: John Johansen --- parser/libapparmor_re/aare_rules.cc | 1 - parser/libapparmor_re/hfa.h | 17 ++++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/parser/libapparmor_re/aare_rules.cc b/parser/libapparmor_re/aare_rules.cc index 96b46e16a..c0aea3898 100644 --- a/parser/libapparmor_re/aare_rules.cc +++ b/parser/libapparmor_re/aare_rules.cc @@ -270,7 +270,6 @@ CHFA *aare_rules::create_chfa(int *min_match_len, if (opts.control & CONTROL_DFA_MINIMIZE) { dfa.minimize(opts); - if (opts.dump & DUMP_DFA_MIN_UNIQ_PERMS) dfa.dump_uniq_perms("minimized dfa"); } diff --git a/parser/libapparmor_re/hfa.h b/parser/libapparmor_re/hfa.h index 1ba0f2cb2..512071be0 100644 --- a/parser/libapparmor_re/hfa.h +++ b/parser/libapparmor_re/hfa.h @@ -131,12 +131,23 @@ public: */ } + + /* returns true if perm is no longer accept */ int apply_and_clear_deny(void) { if (deny) { allow &= ~deny; + exact &= ~deny; prompt &= ~deny; - quiet &= deny; + /* don't change audit or quiet based on clearing + * deny at this stage. This was made unique in + * accept_perms, and the info about whether + * we are auditing or quieting based on the explicit + * deny has been discarded and can only be inferred. + * But we know it is correct from accept_perms() + * audit &= deny; + * quiet &= deny; + */ deny = 0; return !is_accept(); } @@ -282,9 +293,9 @@ public: { accept1 = perms.allow; if (prompt && prompt_compat_mode == PROMPT_COMPAT_DEV) - accept2 = PACK_AUDIT_CTL(perms.prompt, perms.quiet & perms.deny); + accept2 = PACK_AUDIT_CTL(perms.prompt, perms.quiet); else - accept2 = PACK_AUDIT_CTL(perms.audit, perms.quiet & perms.deny); + accept2 = PACK_AUDIT_CTL(perms.audit, perms.quiet); accept3 = perms.prompt; }