Correct inclusion of parentheses when outputing constraints

setools/policyrep/constraint.pxi

@@ -1,5 +1,6 @@
 # Copyright 2014-2016, Tresys Technology, LLC
 # Copyright 2016-2018, Chris PeBenito <[email protected]>
+# Copyright 2024, Sealing Technologies, Inc.
 #
 # SPDX-License-Identifier: LGPL-2.1-only
 #
@@ -192,40 +193,52 @@ cdef class ConstraintExpression(PolicyObject):
 
             # sepol representation is in postfix notation.  This code
             # converts it to infix notation.  Parentheses are added
-            # to ensure correct expressions, though they may end up
-            # being overused.  Set previous operator at start to the
-            # highest precedence (op) so if there is a single binary
-            # operator, no parentheses are output
+            # to ensure correct expressions. Parentheses are needed
+            # whenever an operation involves a subexpression with
+            # lower precedence.
             stack = []
-            prev_op_precedence = _max_precedence
+
+            @dataclasses.dataclass(repr=False, eq=False, frozen=True)
+            class StackObj:
+                precedence: int
+                expression: List[str]
+
             for op in self._postfix:
                 if isinstance(op, frozenset) or op in _operands:
                     # operands
-                    stack.append(op)
+                    stack.append(StackObj(_max_precedence, op))
                 else:
                     # operators
+                    op_precedence = _precedence[op]
                     if op == "not":
                         # unary operator
                         operator = op
-                        operand = stack.pop()
-                        op_precedence = _precedence[op]
-                        stack.append([operator, "(", operand, ")"])
+                        operand_info = stack.pop()
+                        if operand_info.precedence < op_precedence:
+                            e = [operator, "(", operand_info.expression, ")"]
+                        else:
+                            e = [operator, operand_info.expression]
                     else:
                         # binary operators
-                        operand2 = stack.pop()
-                        operand1 = stack.pop()
+                        operand2_info = stack.pop()
+                        operand1_info = stack.pop()
                         operator = op
 
-                        # if previous operator is of higher precedence
-                        # no parentheses are needed.
-                        if _precedence[op] < prev_op_precedence:
-                            stack.append([operand1, operator, operand2])
+                        if operand1_info.precedence < op_precedence:
+                            operand1 = ["(", operand1_info.expression, ")"]
                         else:
-                            stack.append(["(", operand1, operator, operand2, ")"])
+                            operand1 = [operand1_info.expression]
+
+                        if operand2_info.precedence < op_precedence:
+                            operand2 = ["(", operand2_info.expression, ")"]
+                        else:
+                            operand2 = [operand2_info.expression]
+
+                        e = operand1 + [operator] + operand2
 
-                    prev_op_precedence = _precedence[op]
+                    stack.append(StackObj(op_precedence, e))
 
-            self._infix = flatten_list(stack)
+            self._infix = flatten_list(map(lambda x:x.expression, stack))
 
         return self._infix
 

tests/library/constraintquery.conf

@@ -21,6 +21,8 @@ class test41b
 class test50
 class test51a
 class test51b
+class test52a
+class test52b
 
 sid kernel
 sid security
@@ -123,6 +125,12 @@ inherits test
 class test51b
 inherits test
 
+class test52a
+inherits test
+
+class test52b
+inherits test
+
 sensitivity low_s;
 sensitivity medium_s alias med;
 sensitivity high_s;
@@ -277,6 +285,16 @@ constrain test50 hi_w (u1 == u2 or u1 == test50u);
 constrain test51a hi_w (u1 == u2 or u1 == test51u1);
 constrain test51b hi_w (u1 == u2 or u2 == test51u2);
 
+# test 52:
+# ruletype: unset
+# tclass: unset
+# perms: unset
+# role: unset
+# type: unset
+# user: unset
+constrain test52a hi_w ((r1 == system or r2 == system) and u1 == u2);
+constrain test52b hi_w (r1 == system or r2 == system and u1 == u2);
+
 #isids
 sid kernel system:system:system:medium_s:here
 sid security system:system:system:high_s:lost

tests/library/test_constraintquery.py

@@ -1,4 +1,5 @@
 # Copyright 2015, Tresys Technology, LLC
+# Copyright 2024, Sealing Technologies, Inc.
 #
 # SPDX-License-Identifier: GPL-2.0-only
 #
@@ -94,3 +95,17 @@ def test_user_match_regex(self, compiled_policy: setools.SELinuxPolicy) -> None:
 
         constraint = sorted(c.tclass for c in q.results())
         assert ["test51a", "test51b"] == constraint
+
+    def test_or_and_parens(self, compiled_policy: setools.SELinuxPolicy) -> None:
+        """Constraint with an or expression anded with another expression"""
+        q = setools.ConstraintQuery(compiled_policy, tclass=["test52a"])
+
+        constraint = sorted(str(c.expression) for c in q.results())
+        assert ["( r1 == system or r2 == system ) and u1 == u2"] == constraint
+
+    def test_or_and_no_parens(self, compiled_policy: setools.SELinuxPolicy) -> None:
+        """Constraint with an or expression anded with another expression"""
+        q = setools.ConstraintQuery(compiled_policy, tclass=["test52b"])
+
+        constraint = sorted(str(c.expression) for c in q.results())
+        assert ["r1 == system or r2 == system and u1 == u2"] == constraint