From 6e0b7ca20a77aa1faf759c793a2a725120de5bd6 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 24 Dec 2024 01:27:10 -0800 Subject: [PATCH] parser: change priority so that it accumulates based on permissions The current behavior of priority rules can be non-intuitive with higher priority rules completely overriding lower priority rules even in permissions not held in common. This behavior does have use cases but its can be very confusing, and does not normal policy behavior Eg. priority=0 allow r /**, priority=1 deny w /**, will result in no allowed permissions even though the deny rule is only removing the w permission, beause the higher priority rule completely over ride lower priority permissions sets (including none shared permissions). Instead move to tracking the priority at a per permission level. This allows the w permission to still override at priority 1, while the read permission is allowed at priority 0. The final constructed state will still drop priority for the final permission set on the state. Note: this patch updates the equality tests for the cases where the complete override behavior was being tested for. The complete override behavior will be reintroduced in a future patch with a keyword extension, enabling that behavior to be used for ordered blocks etc. Signed-off-by: John Johansen --- parser/common_optarg.c | 1 + parser/immunix.h | 6 + parser/libapparmor_re/apparmor_re.h | 1 + parser/libapparmor_re/hfa.cc | 239 ++++++++++++++++++++-------- parser/libapparmor_re/hfa.h | 27 +++- parser/tst/equality.sh | 4 +- 6 files changed, 202 insertions(+), 76 deletions(-) diff --git a/parser/common_optarg.c b/parser/common_optarg.c index a9dc13fdb..a9bd20b8d 100644 --- a/parser/common_optarg.c +++ b/parser/common_optarg.c @@ -49,6 +49,7 @@ optflag_table_t dumpflag_table[] = { { 1, "dfa-states-post-filter", "Dump dfa state immediately after filtering deny", DUMP_DFA_STATES_POST_FILTER }, { 1, "dfa-states-post-minimize", "Dump dfa state immediately after initial build", DUMP_DFA_STATES_POST_MINIMIZE }, { 1, "dfa-states-post-unreachable", "Dump dfa state immediately after filtering deny", DUMP_DFA_STATES_POST_UNREACHABLE }, + { 1, "dfa-perms-build", "Dump permission being built from accept node", DUMP_DFA_PERMS }, { 1, "dfa-graph", "Dump dfa dot (graphviz) graph", DUMP_DFA_GRAPH }, { 1, "dfa-minimize", "Dump dfa minimization", DUMP_DFA_MINIMIZE }, { 1, "dfa-unreachable", "Dump dfa unreachable states", diff --git a/parser/immunix.h b/parser/immunix.h index 319723c52..11af2ced6 100644 --- a/parser/immunix.h +++ b/parser/immunix.h @@ -95,6 +95,12 @@ #define ALL_USER_EXEC (AA_USER_EXEC | AA_USER_EXEC_TYPE) #define ALL_OTHER_EXEC (AA_OTHER_EXEC | AA_OTHER_EXEC_TYPE) +#define AA_USER_EXEC_INHERIT (AA_EXEC_INHERIT << AA_USER_SHIFT) +#define AA_OTHER_EXEC_INHERIT (AA_EXEC_INHERIT << AA_OTHER_SHIFT) + +#define AA_USER_EXEC_MMAP (AA_OLD_EXEC_MMAP << AA_USER_SHIFT) +#define AA_OTHER_EXEC_MMAP (AA_OLD_EXEC_MMAP << AA_OTHER_SHIFT) + #define AA_LINK_BITS ((AA_OLD_MAY_LINK << AA_USER_SHIFT) | \ (AA_OLD_MAY_LINK << AA_OTHER_SHIFT)) diff --git a/parser/libapparmor_re/apparmor_re.h b/parser/libapparmor_re/apparmor_re.h index edd29aafe..2d8765263 100644 --- a/parser/libapparmor_re/apparmor_re.h +++ b/parser/libapparmor_re/apparmor_re.h @@ -65,5 +65,6 @@ #define DUMP_DFA_STATES_POST_MINIMIZE (1 << 27) #define DUMP_DFA_STATES_POST_UNREACHABLE (1 << 28) #define DUMP_DFA_COMPTRESSED_STATES (1 << 29) +#define DUMP_DFA_PERMS (1 << 30) #endif /* APPARMOR_RE_H */ diff --git a/parser/libapparmor_re/hfa.cc b/parser/libapparmor_re/hfa.cc index e77851733..263859979 100644 --- a/parser/libapparmor_re/hfa.cc +++ b/parser/libapparmor_re/hfa.cc @@ -317,7 +317,8 @@ static void split_node_types(NodeSet *nodes, NodeSet **anodes, NodeSet **nnodes *nnodes = nodes; } -State *DFA::add_new_state(NodeSet *anodes, NodeSet *nnodes, State *other) +State *DFA::add_new_state(optflags const &opts, NodeSet *anodes, + NodeSet *nnodes, State *other) { NodeVec *nnodev, *anodev; nnodev = nnodes_cache.insert(nnodes); @@ -325,7 +326,7 @@ State *DFA::add_new_state(NodeSet *anodes, NodeSet *nnodes, State *other) ProtoState proto; proto.init(nnodev, anodev); - State *state = new State(node_map.size(), proto, other, filedfa); + State *state = new State(opts, node_map.size(), proto, other, filedfa); pair x = node_map.insert(proto, state); if (x.second == false) { delete state; @@ -337,7 +338,7 @@ State *DFA::add_new_state(NodeSet *anodes, NodeSet *nnodes, State *other) return x.first->second; } -State *DFA::add_new_state(NodeSet *nodes, State *other) +State *DFA::add_new_state(optflags const &opts, NodeSet *nodes, State *other) { /* The splitting of nodes should probably get pushed down into * follow(), ie. put in separate lists from the start @@ -345,12 +346,12 @@ State *DFA::add_new_state(NodeSet *nodes, State *other) NodeSet *anodes, *nnodes; split_node_types(nodes, &anodes, &nnodes); - State *state = add_new_state(anodes, nnodes, other); + State *state = add_new_state(opts, anodes, nnodes, other); return state; } -void DFA::update_state_transitions(State *state) +void DFA::update_state_transitions(optflags const &opts, State *state) { /* Compute possible transitions for state->nodes. This is done by * iterating over all the nodes in state->nodes and combining the @@ -373,7 +374,8 @@ void DFA::update_state_transitions(State *state) /* check the default transition first */ if (cases.otherwise) - state->otherwise = add_new_state(cases.otherwise, nonmatching); + state->otherwise = add_new_state(opts, cases.otherwise, + nonmatching); else state->otherwise = nonmatching; @@ -382,7 +384,7 @@ void DFA::update_state_transitions(State *state) */ for (Cases::iterator j = cases.begin(); j != cases.end(); j++) { State *target; - target = add_new_state(j->second, nonmatching); + target = add_new_state(opts, j->second, nonmatching); /* Don't insert transition that the otherwise transition * already covers @@ -429,7 +431,7 @@ void DFA::process_work_queue(const char *header, optflags const &opts) /* Update 'from's transitions, and if it transitions to any * unknown State create it and add it to the work_queue */ - update_state_transitions(from); + update_state_transitions(opts, from); } /* while (!work_queue.empty()) */ } @@ -459,8 +461,8 @@ DFA::DFA(Node *root, optflags const &opts, bool buildfiledfa): root(root), filed (*i)->compute_followpos(); } - nonmatching = add_new_state(new NodeSet, NULL); - start = add_new_state(new NodeSet(root->firstpos), nonmatching); + nonmatching = add_new_state(opts, new NodeSet, NULL); + start = add_new_state(opts, new NodeSet(root->firstpos), nonmatching); /* the work_queue contains the states that need to have their * transitions computed. This could be done with a recursive @@ -1391,85 +1393,186 @@ static inline int diff_qualifiers(perm32_t perm1, perm32_t perm2) (perm1 & AA_EXEC_TYPE) != (perm2 & AA_EXEC_TYPE)); } +/* update a single permission based on priority - only called if match->perm | match-> audit bit set */ +static int pri_update_perm(optflags const &opts, vector &priority, int i, + MatchFlag *match, perms_t &perms, perms_t &exact, + bool filedfa) +{ + if (priority[i] > match->priority) { + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " > " << match->priority << " SKIPPING " << hex << (match->perms) << "/" << (match->audit) << dec << "\n"; + return 0; + } + + perm32_t xmask = 0; + perm32_t mask = 1 << i; + perm32_t amask = mask; + + // drop once we move the xindex out of the perms in the front end + if (filedfa) { + if (mask & AA_USER_EXEC) { + xmask = AA_USER_EXEC_TYPE; + // ix implies EXEC_MMAP + if (match->perms & AA_EXEC_INHERIT) { + xmask |= AA_USER_EXEC_MMAP; + //USER_EXEC_MAP = 6 + if (priority[6] < match->priority) + priority[6] = match->priority; + } + amask = mask | xmask; + } else if (mask & AA_OTHER_EXEC) { + xmask = AA_OTHER_EXEC_TYPE; + // ix implies EXEC_MMAP + if (match->perms & AA_OTHER_EXEC_INHERIT) { + xmask |= AA_OTHER_EXEC_MMAP; + //OTHER_EXEC_MAP = 20 + if (priority[20] < match->priority) + priority[20] = match->priority; + } + amask = mask | xmask; + } else if (((mask & AA_USER_EXEC_MMAP) && + (match->perms & AA_USER_EXEC_INHERIT)) || + ((mask & AA_OTHER_EXEC_MMAP) && + (match->perms & AA_OTHER_EXEC_INHERIT))) { + // if exec && ix we handled mmp above + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " SKIPPING mmap unmasked " << hex << match->perms << "/" << match->audit << " masked " << (match->perms & amask) << "/" << (match->audit & amask) << " data " << (perms.allow & mask) << "/" << (perms.audit & mask) << " exact " << (exact.allow & mask) << "/" << (exact.audit & mask) << dec << "\n"; + return 0; + } + } + + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " vs. " << match->priority << " mask: " << hex << mask << " xmask: " << xmask << " amask: " << amask << dec << "\n"; + if (priority[i] < match->priority) { + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " < " << match->priority << " clearing " << hex << (perms.allow & amask) << "/" << (perms.audit & amask) << " -> " << dec; + priority[i] = match->priority; + perms.clear_bits(amask); + exact.clear_bits(amask); + if (opts.dump & DUMP_DFA_PERMS) + cerr << hex << (perms.allow & amask) << "/" << (perms.audit & amask) << dec << "\n"; + } + + // the if conditions in order of permission priority + if (match->is_type(NODE_TYPE_DENYMATCHFLAG)) { + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " deny " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n"; + perms.deny |= match->perms & amask; + perms.quiet |= match->audit & amask; + + perms.allow &= ~amask; + perms.audit &= ~amask; + perms.prompt &= ~amask; + } else if (match->is_type(NODE_TYPE_EXACTMATCHFLAG)) { + /* exact match only asserts dominance on the XTYPE */ + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " exact " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n"; + if (filedfa && + !is_merged_x_consistent(exact.allow, match->perms & amask)) { + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " exact match conflict" << "\n"; + return 1; + } + exact.allow |= match->perms & amask; + exact.audit |= match->audit & amask; + + // dominance is only done for XTYPE so only clear that + // note xmask only set if setting x perm bit, so this + // won't clear for other bit types + perms.allow &= ~xmask; + perms.audit &= ~xmask; + perms.prompt &= ~xmask; + + perms.allow |= match->perms & amask; + perms.audit |= match->audit & amask; + // can't specify exact prompt atm + + } else if (!match->is_type(NODE_TYPE_PROMPTMATCHFLAG)) { + // allow perms, if exact has been encountered will already be set + // if overlaps x here, don't conflict, because exact will override + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " allow " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n"; + if (filedfa && !(exact.allow & mask) && + !is_merged_x_consistent(perms.allow, match->perms & amask)) { + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " allow match conflict" << "\n"; + return 1; + } + // mask off if XTYPE in xmatch + if ((exact.allow | exact.audit) & mask) { + // mask == amask & ~xmask + perms.allow |= match->perms & mask; + perms.audit |= match->audit & mask; + } else { + perms.allow |= match->perms & amask; + perms.audit |= match->audit & amask; + } + } else { // if (match->is_type(NODE_TYPE_PROMPTMATCHFLAG)) { + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " promt " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n"; + if (filedfa && !((exact.allow | perms.allow) & mask) && + !is_merged_x_consistent(perms.allow, match->perms & amask)) { + if (opts.dump & DUMP_DFA_PERMS) + cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " prompt match conflict" << "\n"; + return 1; + } + if ((exact.allow | exact.audit | perms.allow | perms.audit) & mask) { + // mask == amask & ~xmask + perms.prompt |= match->perms & mask; + perms.audit |= match->audit & mask; + } else { + perms.prompt |= match->perms & amask; + perms.audit |= match->audit & amask; + } + } + + return 0; +} + /** * Compute the permission flags that this state corresponds to. If we * have any exact matches, then they override the execute and safe * execute flags. */ -int accept_perms(NodeVec *state, perms_t &perms, bool filedfa) +int accept_perms(optflags const &opts, NodeVec *state, perms_t &perms, + bool filedfa) { int error = 0; perms_t exact; - int priority = MIN_INTERNAL_PRIORITY; + // size of vector needs to be number of bits in the data type + // being used for the permission set. + std::vector priority(sizeof(perm32_t)*8, MIN_INTERNAL_PRIORITY); perms.clear(); if (!state) return error; - + if (opts.dump & DUMP_DFA_PERMS) + cerr << "Building\n"; for (NodeVec::iterator i = state->begin(); i != state->end(); i++) { if (!(*i)->is_type(NODE_TYPE_MATCHFLAG)) continue; MatchFlag *match = static_cast(*i); - if (priority > match->priority) - continue; - if (priority < match->priority) { - priority = match->priority; - perms.clear(); - exact.clear(); - } - if (match->is_type(NODE_TYPE_EXACTMATCHFLAG)) { - /* exact match only ever happens with x */ - if (filedfa && - !is_merged_x_consistent(exact.allow, match->perms)) - error = 1; - exact.allow |= match->perms; - exact.audit |= match->audit; - } else if (match->is_type(NODE_TYPE_DENYMATCHFLAG)) { - perms.deny |= match->perms; - perms.quiet |= match->audit; - } else if (match->is_type(NODE_TYPE_PROMPTMATCHFLAG)) { - perms.prompt |= match->perms; - perms.audit |= match->audit; - } else { - if (filedfa && - !is_merged_x_consistent(perms.allow, match->perms)) - error = 1; - perms.allow |= match->perms; - perms.audit |= match->audit; + perm32_t bit = 1; + perm32_t check = match->perms | match->audit; + if (filedfa) + check &= ~ALL_AA_EXEC_TYPE; + + for (int i = 0; check; i++) { + if (check & bit) { + error = pri_update_perm(opts, priority, i, match, perms, exact, filedfa); + if (error) + goto out; + } + check &= ~bit; + bit <<= 1; } } - - if (filedfa) { - perms.allow |= exact.allow & ~(ALL_AA_EXEC_TYPE); - perms.prompt |= exact.prompt & ~(ALL_AA_EXEC_TYPE); - perms.audit |= exact.audit & ~(ALL_AA_EXEC_TYPE); - } else { - perms.allow |= exact.allow; - perms.prompt |= exact.prompt; - perms.audit |= exact.audit; + if (opts.dump & DUMP_DFA_PERMS) { + cerr << " computed: "; perms.dump(cerr); cerr << "\n"; } - if (exact.allow & AA_USER_EXEC) { - perms.allow = (exact.allow & AA_USER_EXEC_TYPE) | - (perms.allow & ~AA_USER_EXEC_TYPE); - perms.exact = AA_USER_EXEC_TYPE; - } - if (exact.allow & AA_OTHER_EXEC) { - perms.allow = (exact.allow & AA_OTHER_EXEC_TYPE) | - (perms.allow & ~AA_OTHER_EXEC_TYPE); - perms.exact |= AA_OTHER_EXEC_TYPE; - } - if (filedfa && (AA_USER_EXEC & perms.deny)) - perms.deny |= AA_USER_EXEC_TYPE; - - if (filedfa && (AA_OTHER_EXEC & perms.deny)) - perms.deny |= AA_OTHER_EXEC_TYPE; - - perms.allow &= ~perms.deny; - perms.quiet &= perms.deny; - perms.prompt &= ~perms.deny; - perms.prompt &= ~perms.allow; +out: if (error) fprintf(stderr, "profile has merged rule with conflicting x modifiers\n"); diff --git a/parser/libapparmor_re/hfa.h b/parser/libapparmor_re/hfa.h index 6049ab0c4..f66d880e2 100644 --- a/parser/libapparmor_re/hfa.h +++ b/parser/libapparmor_re/hfa.h @@ -70,6 +70,17 @@ public: void clear(void) { allow = deny = prompt = audit = quiet = exact = 0; } + + void clear_bits(perm32_t bits) + { + allow &= ~bits; + deny &= ~bits; + prompt &= ~bits; + audit &= ~bits; + quiet &= ~bits; + exact &= ~bits; + } + void add(perms_t &rhs, bool filedfa) { deny |= rhs.deny; @@ -159,7 +170,8 @@ public: perm32_t allow, deny, prompt, audit, quiet, exact; }; -int accept_perms(NodeVec *state, perms_t &perms, bool filedfa); +int accept_perms(optflags const &opts, NodeVec *state, perms_t &perms, + bool filedfa); /* * ProtoState - NodeSet and ancillery information used to create a state @@ -223,7 +235,8 @@ struct DiffDag { */ class State { public: - State(int l, ProtoState &n, State *other, bool filedfa): + State(optflags const &opts, int l, ProtoState &n, State *other, + bool filedfa): label(l), flags(0), idx(0), perms(), trans() { int error; @@ -236,7 +249,7 @@ public: proto = n; /* Compute permissions associated with the State. */ - error = accept_perms(n.anodes, perms, filedfa); + error = accept_perms(opts, n.anodes, perms, filedfa); if (error) { //cerr << "Failing on accept perms " << error << "\n"; throw error; @@ -340,9 +353,11 @@ typedef map Renumber_Map; /* Transitions in the DFA. */ class DFA { void dump_node_to_dfa(void); - State *add_new_state(NodeSet *nodes, State *other); - State *add_new_state(NodeSet *anodes, NodeSet *nnodes, State *other); - void update_state_transitions(State *state); + State *add_new_state(optflags const &opts, NodeSet *nodes, + State *other); + State *add_new_state(optflags const &opts,NodeSet *anodes, + NodeSet *nnodes, State *other); + void update_state_transitions(optflags const &opts, State *state); void process_work_queue(const char *header, optflags const &); void dump_diff_chain(ostream &os, map &relmap, Partition &chain, State *state, diff --git a/parser/tst/equality.sh b/parser/tst/equality.sh index c06c7ba38..5ee69b079 100755 --- a/parser/tst/equality.sh +++ b/parser/tst/equality.sh @@ -725,7 +725,7 @@ else #this one may not be true in the future depending on if the compiled profile #is explicitly including deny permissions for dynamic composition - verify_binary_inequality "'$p1'x'$p2' Deny of ungranted perm" \ + verify_binary_equality "'$p1'x'$p2' Deny of ungranted perm" \ "/t { $p1 /foo/[abc] r, audit deny /foo/b w, }" \ "/t { $p2 /foo/[abc] r, }" fi @@ -823,7 +823,7 @@ if { priority_lt "$p1" "" && priority_lt "$p2" "" ; } || "/t { $p1 owner /proc/[0-9]*/attr/{apparmor/,}current a, ^test { $p1 owner /proc/[0-9]*/attr/{apparmor/,}current a, /f r, }}" \ "/t { $p2 owner /proc/[0-9]*/attr/{apparmor/,}current w, ^test { $p2 owner /proc/[0-9]*/attr/{apparmor/,}current w, /f r, }}" else - verify_binary_inequality "'$p1'x'$p2' change_hat rules automatically inserted"\ + verify_binary_equality "'$p1'x'$p2' change_hat rules automatically inserted"\ "/t { $p1 owner /proc/[0-9]*/attr/{apparmor/,}current a, ^test { $p1 owner /proc/[0-9]*/attr/{apparmor/,}current a, /f r, }}" \ "/t { $p2 owner /proc/[0-9]*/attr/{apparmor/,}current w, ^test { $p2 owner /proc/[0-9]*/attr/{apparmor/,}current w, /f r, }}" fi