parser: fix integer overflow bug in rule priority comparisons

There is an integer overflow when comparing priorities when cmp is
used because it uses subtraction to find lessthan, equal, and greater
than in one operation.

But INT_MAX and INT_MIN are being used by priorities and this results
in INT_MAX - INT_MIN and INT_MIN - INT_MAX which are both overflows
causing an incorrect comparison result and selection of the wrong
rule permission.

Closes: https://gitlab.com/apparmor/apparmor/-/issues/452
Fixes: e3fca60d1 ("parser: add the ability to specify a priority prefix to rules")
Signed-off-by: John Johansen <john.johansen@canonical.com>

parser/immunix.h

@@ -175,6 +175,28 @@ static inline int is_merged_x_consistent(int a, int b)
 	return 1;
 }
 
+/* Arbitrary max and minimum priority that userspace can specify,
+ * internally we handle up to MAX_INTERNAL_PRIORITY and
+ * MIN_INTERNAL_PRIORITY. Do not ever allow INT_MAX, or INT_MIN
+ * because cmp uses subtraction and it can cause overflow.  Ensure we
+ * don't over/underflow make internal max/min one more than allowed on
+ * rules.
+ *
+ * see
+ * note on mediates_priority
+ */
+#define MIN_POLICY_PRIORITY (-1000)
+#define MAX_POLICY_PRIORITY (1000)
+
+/* internally we need a priority that any policy based rule can override
+ * and a priority that no policy based rule can override. These are
+ * used on rules encoding what abi/classes are supported by the
+ * compiled policy.
+ */
+#define MIN_INTERNAL_PRIORITY (MIN_POLICY_PRIORITY - 1)
+#define MAX_INTERNAL_PRIORITY (MAX_POLICY_PRIORITY + 1)
+
+
 #endif				/* ! _IMMUNIX_H */
 
 /*  LocalWords:  MMAP

parser/libapparmor_re/hfa.h

@@ -52,7 +52,7 @@ ostream &operator<<(ostream &os, State &state);
 
 class perms_t {
 public:
-	perms_t(void): priority(INT_MIN), allow(0), deny(0), prompt(0), audit(0), quiet(0), exact(0) { };
+	perms_t(void): priority(MIN_INTERNAL_PRIORITY), allow(0), deny(0), prompt(0), audit(0), quiet(0), exact(0) { };
 
 	bool is_accept(void) { return (allow | deny | prompt | audit | quiet); }
 
@@ -68,7 +68,7 @@ public:
 	}
 
 	void clear(void) {
-		priority = INT_MIN;
+		priority = MIN_INTERNAL_PRIORITY;
 		allow = deny = prompt = audit = quiet = exact = 0;
 	}
 	void clear(int p) {

parser/parser.h

@@ -53,12 +53,6 @@ using namespace std;
  */
 extern int parser_token;
 
-/* Arbitrary max and minimum priority that userspace can specify, internally
- * we handle up to INT_MAX and INT_MIN. Do not ever allow INT_MAX, see
- * note on mediates_priority
- */
-#define MAX_PRIORITY 1000
-#define MIN_PRIORITY -1000
 
 #define WARN_RULE_NOT_ENFORCED	0x1
 #define WARN_RULE_DOWNGRADED	0x2

parser/parser_regex.c

@@ -1093,9 +1093,10 @@ static const char *deny_file = ".*";
  *
  * Note: it turns out the above bug does exist for dbus rules in parsers
  * that do not support priority, and we don't have a way to fix it.
- * We fix it here by capping user specified priority to be < INT_MAX.
+ * We fix it here by capping user specified priority to be less than
+ * MAX_INTERNAL_PRIORITY.
  */
-static int mediates_priority = INT_MAX;
+static int mediates_priority = MAX_INTERNAL_PRIORITY;
 
 /* some rule types unfortunately encoded permissions on the class byte
  * to fix the above bug, they need a different solution. The generic
@@ -1106,7 +1107,7 @@ static int mediates_priority = INT_MAX;
  * and it is guaranteed to have the same priority as the highest priority
  * rule.
  */
-static int perms_onclass_mediates_priority = INT_MIN;
+static int perms_onclass_mediates_priority = MIN_INTERNAL_PRIORITY;
 
 int process_profile_policydb(Profile *prof)
 {

parser/parser_yacc.y

@@ -640,10 +640,10 @@ opt_priority: { $$ = 0; }
 			yyerror("invalid priority %s", $3);
 		free($3);
 		/* see note on mediates_priority */
-		if (tmp > MAX_PRIORITY)
-			yyerror("invalid priority %l > %d", tmp, MAX_PRIORITY);
-		if (tmp < MIN_PRIORITY)
-			yyerror("invalid priority %l > %d", tmp, MIN_PRIORITY);
+		if (tmp > MAX_POLICY_PRIORITY)
+			yyerror("invalid priority %l > %d", tmp, MAX_POLICY_PRIORITY);
+		if (tmp < MIN_POLICY_PRIORITY)
+			yyerror("invalid priority %l > %d", tmp, MIN_POLICY_PRIORITY);
 		$$ = tmp;
 	}
 

parser/tst/simple_tests/file/priority/bad_1.sd

@@ -0,0 +1,7 @@
+#
+#=Description to rule priority out of low end of range
+#=EXRESULT FAIL
+#
+/usr/bin/foo {
+  priority=-1001 /usr/bin/foo r,
+}

parser/tst/simple_tests/file/priority/bad_2.sd

@@ -0,0 +1,7 @@
+#
+#=Description basic file rule priority outside high end of range.
+#=EXRESULT FAIL
+#
+/usr/bin/foo {
+  priority=1001 /usr/bin/foo r,
+}