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

@@ -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

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 */

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 {

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;