From b6d9d9d8b6701237f3cc913bee0998de16aad307 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 5 Jun 2024 18:38:29 -0700 Subject: [PATCH] parser: fix Normalizatin infinite loop Expression simplification can get into an infinite loop due to eps pairs hiding behind and alternation that can't be caught by normalize_eps() (which exists in the first place to stop a similar loop). The loop in question happens in AltNode::normalize when a subtree has the following structure. 1. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too alt /\ / \ / \ eps alt /\ / \ / \ alt eps /\ / \ / \ eps eps 2. if (normalize_eps(dir)) results in alt /\ / \ / \ alt eps /\ / \ / \ alt eps /\ / \ / \ eps eps 3. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too alt /\ / \ / \ alt alt /\ /\ / \ / \ / \ / \ eps eps eps eps 4. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too alt /\ / \ / \ eps alt /\ / \ / \ eps alt /\ / \ / \ eps eps 5. if (normalize_eps(dir)) results in alt /\ / \ / \ alt eps /\ / \ / \ eps alt /\ / \ / \ eps eps 6. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too alt /\ / \ / \ eps alt /\ / \ / \ alt eps /\ / \ / \ eps eps back to beginning of cycle Fix this by detecting the creation of an eps_pair in rotate_node(), that pair can be immediately eliminated by simplifying the tree in that step. In the above cycle the pair creation is caught at step 3 resulting in 3. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too alt /\ / \ / \ alt eps /\ / \ / \ eps eps 4. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too alt /\ / \ / \ eps alt /\ / \ / \ eps eps whch gets reduces to alt /\ / \ / \ eps eps breaking the normalization loop. The degenerate alt node will be caught in turn when its parent is dealt with. This needs to be backported to all releases Closes: https://gitlab.com/apparmor/apparmor/-/issues/398 Fixes: 846cee506 ("Split out parsing and expression trees from regexp.y") Reported-by: Christian Boltz Signed-off-by: John Johansen --- parser/libapparmor_re/expr-tree.cc | 25 +++++++++++++------ .../simple_tests/regressions/ok_normalize.sd | 25 +++++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) create mode 100644 parser/tst/simple_tests/regressions/ok_normalize.sd diff --git a/parser/libapparmor_re/expr-tree.cc b/parser/libapparmor_re/expr-tree.cc index 53c640d4e..184d51515 100644 --- a/parser/libapparmor_re/expr-tree.cc +++ b/parser/libapparmor_re/expr-tree.cc @@ -189,6 +189,19 @@ void Node::dump_syntax_tree(ostream &os) * a b c T * */ + + +static Node *simplify_eps_pair(Node *t) +{ + if (t->is_type(NODE_TYPE_TWOCHILD) && + t->child[0] == &epsnode && + t->child[1] == &epsnode) { + t->release(); + return &epsnode; + } + return t; +} + static void rotate_node(Node *t, int dir) { // (a | b) | c -> a | (b | c) @@ -197,7 +210,9 @@ static void rotate_node(Node *t, int dir) t->child[dir] = left->child[dir]; left->child[dir] = left->child[!dir]; left->child[!dir] = t->child[!dir]; - t->child[!dir] = left; + + // check that rotation didn't create (E | E) + t->child[!dir] = simplify_eps_pair(left); } /* return False if no work done */ @@ -209,13 +224,7 @@ int TwoChildNode::normalize_eps(int dir) // Ea -> aE // Test for E | (E | E) and E . (E . E) which will // result in an infinite loop - Node *c = child[!dir]; - if (c->is_type(NODE_TYPE_TWOCHILD) && - &epsnode == c->child[dir] && - &epsnode == c->child[!dir]) { - c->release(); - c = &epsnode; - } + Node *c = simplify_eps_pair(child[!dir]); child[!dir] = child[dir]; child[dir] = c; return 1; diff --git a/parser/tst/simple_tests/regressions/ok_normalize.sd b/parser/tst/simple_tests/regressions/ok_normalize.sd new file mode 100644 index 000000000..d810b0c50 --- /dev/null +++ b/parser/tst/simple_tests/regressions/ok_normalize.sd @@ -0,0 +1,25 @@ +# +#=Description caused an infinite loop in expr normalization +#=EXRESULT PASS + +# This test triggers an infinite loop bug in expr normalization +# Note: this test might be able to be reduced more but, each element appears +# to be required to trigger the bug. +# that is the initial var assignment, += with the "comment" at the end +# (which is a separate bug), the expansion in the 2nd variable and then +# the use of the 2nd variable. +# This seems to be due to difference in consistency check between expansion +# at parse time and variable expansion. +# eg. expanding @{exec_path} manually will result in a failure to parse +# see: https://gitlab.com/apparmor/apparmor/-/issues/398 + +@{var}=*-linux-gnu* +@{var}+=*-suse-linux* #aa:only opensuse + +@{exec_path} = /{,@{var}/}t + +profile test { + + + @{exec_path} mr, +}