From e2d55844a21656a42397d669f82ce368c8993d13 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 15 Aug 2024 13:22:19 -0700 Subject: [PATCH] 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 --- parser/immunix.h | 22 +++++++++++++++++++ parser/libapparmor_re/hfa.h | 4 ++-- parser/parser.h | 6 ----- parser/parser_regex.c | 7 +++--- parser/parser_yacc.y | 8 +++---- .../tst/simple_tests/file/priority/bad_1.sd | 7 ++++++ .../tst/simple_tests/file/priority/bad_2.sd | 7 ++++++ 7 files changed, 46 insertions(+), 15 deletions(-) create mode 100644 parser/tst/simple_tests/file/priority/bad_1.sd create mode 100644 parser/tst/simple_tests/file/priority/bad_2.sd diff --git a/parser/immunix.h b/parser/immunix.h index 357a2d16a..319723c52 100644 --- a/parser/immunix.h +++ b/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 diff --git a/parser/libapparmor_re/hfa.h b/parser/libapparmor_re/hfa.h index 3c6afb071..1ba0f2cb2 100644 --- a/parser/libapparmor_re/hfa.h +++ b/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) { diff --git a/parser/parser.h b/parser/parser.h index 6f0425c81..ca7274f25 100644 --- a/parser/parser.h +++ b/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 diff --git a/parser/parser_regex.c b/parser/parser_regex.c index 7810458d5..e4d6e012d 100644 --- a/parser/parser_regex.c +++ b/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) { diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y index 7bc5067ef..d1fe72d2e 100644 --- a/parser/parser_yacc.y +++ b/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; } diff --git a/parser/tst/simple_tests/file/priority/bad_1.sd b/parser/tst/simple_tests/file/priority/bad_1.sd new file mode 100644 index 000000000..26cf4d553 --- /dev/null +++ b/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, +} diff --git a/parser/tst/simple_tests/file/priority/bad_2.sd b/parser/tst/simple_tests/file/priority/bad_2.sd new file mode 100644 index 000000000..84ab2df60 --- /dev/null +++ b/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, +}