From cc09794fbdd3fe311027ae2ceb9f441a30a6075a Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Fri, 8 Feb 2019 11:32:16 -0800 Subject: [PATCH] parser: determine xmatch priority based on smallest DFA match The length of a xmatch is used to prioritize multiple profiles that match the same path, with the intent that the more specific match wins. Currently, the length of a xmatch is computed by the position of the first regex character. While trying to work around issues with no_new_privs by combining profiles, we noticed that the xmatch length computation doesn't work as expected for multiple regexs. Consider the following two profiles: profile all /** { } profile bins /{,usr/,usr/local/}bin/** { } xmatch_len is currently computed as "1" for both profiles, even though "bins" is clearly more specific. When determining the length of a regex, compute the smallest possible match and use that for xmatch priority instead of the position of the first regex character. --- parser/libapparmor_re/aare_rules.cc | 6 +- parser/libapparmor_re/aare_rules.h | 2 +- parser/libapparmor_re/expr-tree.h | 95 +++++++++++++++++++++++++++++ parser/parser_regex.c | 13 ++-- 4 files changed, 106 insertions(+), 10 deletions(-) diff --git a/parser/libapparmor_re/aare_rules.cc b/parser/libapparmor_re/aare_rules.cc index 02aa80fa0..fd719db48 100644 --- a/parser/libapparmor_re/aare_rules.cc +++ b/parser/libapparmor_re/aare_rules.cc @@ -126,9 +126,10 @@ bool aare_rules::add_rule_vec(int deny, uint32_t perms, uint32_t audit, /* create a dfa from the ruleset * returns: buffer contain dfa tables, @size set to the size of the tables - * else NULL on failure + * else NULL on failure, @min_match_len set to the shortest string + * that can match the dfa for determining xmatch priority. */ -void *aare_rules::create_dfa(size_t *size, dfaflags_t flags) +void *aare_rules::create_dfa(size_t *size, int *min_match_len, dfaflags_t flags) { char *buffer = NULL; @@ -150,6 +151,7 @@ void *aare_rules::create_dfa(size_t *size, dfaflags_t flags) root = new AltNode(root, new CatNode(tmp, i->first)); } } + *min_match_len = root->min_match_len(); /* dumping of the none simplified tree without -O no-expr-simplify * is broken because we need to build the tree above first, and diff --git a/parser/libapparmor_re/aare_rules.h b/parser/libapparmor_re/aare_rules.h index 87b79f590..3cdfa0963 100644 --- a/parser/libapparmor_re/aare_rules.h +++ b/parser/libapparmor_re/aare_rules.h @@ -104,7 +104,7 @@ class aare_rules { uint32_t audit, dfaflags_t flags); bool add_rule_vec(int deny, uint32_t perms, uint32_t audit, int count, const char **rulev, dfaflags_t flags); - void *create_dfa(size_t *size, dfaflags_t flags); + void *create_dfa(size_t *size, int *min_match_len, dfaflags_t flags); }; #endif /* __LIBAA_RE_RULES_H */ diff --git a/parser/libapparmor_re/expr-tree.h b/parser/libapparmor_re/expr-tree.h index 9100db32f..6990cdef6 100644 --- a/parser/libapparmor_re/expr-tree.h +++ b/parser/libapparmor_re/expr-tree.h @@ -144,6 +144,18 @@ public: virtual void compute_lastpos() = 0; virtual void compute_followpos() { } + /* + * min_match_len determines the smallest string that can match the + * syntax tree. This is used to determine the priority of a regex. + */ + virtual int min_match_len() { return 0; } + /* + * contains_null returns if the expression tree contains a null character. + * Null characters indicate that the rest of the DFA matches the xattrs and + * not the path. This is used to compute min_match_len. + */ + virtual bool contains_null() { return false; } + virtual int eq(Node *other) = 0; virtual ostream &dump(ostream &os) = 0; void dump_syntax_tree(ostream &os); @@ -278,6 +290,17 @@ public: return os << c; } + int min_match_len() + { + if (c == 0) { + // Null character indicates end of string. + return 0; + } + return 1; + } + + bool contains_null() { return c == 0; } + uchar c; }; @@ -319,6 +342,24 @@ public: return os << ']'; } + int min_match_len() + { + if (contains_null()) { + return 0; + } + return 1; + } + + bool contains_null() + { + for (Chars::iterator i = chars.begin(); i != chars.end(); i++) { + if (*i == 0) { + return true; + } + } + return false; + } + Chars chars; }; @@ -367,6 +408,24 @@ public: return os << ']'; } + int min_match_len() + { + if (contains_null()) { + return 0; + } + return 1; + } + + bool contains_null() + { + for (Chars::iterator i = chars.begin(); i != chars.end(); i++) { + if (*i == 0) { + return false; + } + } + return true; + } + Chars chars; }; @@ -390,6 +449,8 @@ public: return 0; } ostream &dump(ostream &os) { return os << "."; } + + bool contains_null() { return true; } }; /* Match a node zero or more times. (This is a unary operator.) */ @@ -417,6 +478,8 @@ public: child[0]->dump(os); return os << ")*"; } + + bool contains_null() { return child[0]->contains_null(); } }; /* Match a node one or more times. (This is a unary operator.) */ @@ -444,6 +507,8 @@ public: child[0]->dump(os); return os << ")+"; } + int min_match_len() { return child[0]->min_match_len(); } + bool contains_null() { return child[0]->contains_null(); } }; /* Match a pair of consecutive nodes. */ @@ -491,6 +556,22 @@ public: return os; } void normalize(int dir); + int min_match_len() + { + int len = child[0]->min_match_len(); + if (child[0]->contains_null()) { + // Null characters are used to indicate when the DFA transitions + // from matching the path to matching the xattrs. If the left child + // contains a null character, the right side doesn't contribute to + // the path match. + return len; + } + return len + child[1]->min_match_len(); + } + bool contains_null() + { + return child[0]->contains_null() || child[1]->contains_null(); + } }; /* Match one of two alternative nodes. */ @@ -528,6 +609,20 @@ public: return os; } void normalize(int dir); + int min_match_len() + { + int m1, m2; + m1 = child[0]->min_match_len(); + m2 = child[1]->min_match_len(); + if (m1 < m2) { + return m1; + } + return m2; + } + bool contains_null() + { + return child[0]->contains_null() || child[1]->contains_null(); + } }; class SharedNode: public ImportantNode { diff --git a/parser/parser_regex.c b/parser/parser_regex.c index bdc3a5866..72df37aa8 100644 --- a/parser/parser_regex.c +++ b/parser/parser_regex.c @@ -473,17 +473,13 @@ static int process_profile_name_xmatch(Profile *prof) ptype = convert_aaregex_to_pcre(alt->name, 0, glob_default, tbuf, &len); - if (ptype == ePatternBasic) - len = strlen(alt->name); - if (len < prof->xmatch_len) - prof->xmatch_len = len; if (!rules->add_rule(tbuf.c_str(), 0, AA_MAY_EXEC, 0, dfaflags)) { delete rules; return FALSE; } } } - prof->xmatch = rules->create_dfa(&prof->xmatch_size, dfaflags); + prof->xmatch = rules->create_dfa(&prof->xmatch_size, &prof->xmatch_len, dfaflags); delete rules; if (!prof->xmatch) return FALSE; @@ -679,8 +675,9 @@ int process_profile_regex(Profile *prof) goto out; if (prof->dfa.rules->rule_count > 0) { + int xmatch_len = 0; prof->dfa.dfa = prof->dfa.rules->create_dfa(&prof->dfa.size, - dfaflags); + &xmatch_len, dfaflags); delete prof->dfa.rules; prof->dfa.rules = NULL; if (!prof->dfa.dfa) @@ -815,7 +812,9 @@ int process_profile_policydb(Profile *prof) goto out; if (prof->policy.rules->rule_count > 0) { - prof->policy.dfa = prof->policy.rules->create_dfa(&prof->policy.size, dfaflags); + int xmatch_len = 0; + prof->policy.dfa = prof->policy.rules->create_dfa(&prof->policy.size, + &xmatch_len, dfaflags); delete prof->policy.rules; prof->policy.rules = NULL;