[PATCH] libselpol: Sort portcon rules consistently

[PATCH] libselpol: Sort portcon rules consistently

[James Carter]

The comparison function, portcon_data_cmp(), only made use of the
protocol to put tcp before udp, dccp, and sctp. Rules that have
the same port range, but with different protocols would be considered
equal unless one of the protocols was tcp. When generating a CIL or
conf source policy from a binary or using the "-S" option in
checkpolicy the non-tcp portcon rules with the same port range would
not be consistently sorted.

Changed portcon_data_cmp() to sort portcon rules like the CIL function
cil_post_portcon_compare().

Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/src/kernel_to_common.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libsepol/src/kernel_to_common.c b/libsepol/src/kernel_to_common.c
index 7b53c92f..a7453d3c 100644
--- a/libsepol/src/kernel_to_common.c
+++ b/libsepol/src/kernel_to_common.c
@@ -470,11 +470,9 @@ static int portcon_data_cmp(const void *a, const void *b)
 	rc = compare_ranges((*aa)->u.port.low_port, (*aa)->u.port.high_port,
 			    (*bb)->u.port.low_port, (*bb)->u.port.high_port);
 	if (rc == 0) {
-		if ((*aa)->u.port.protocol == (*bb)->u.port.protocol) {
-			rc = 0;
-		} else if ((*aa)->u.port.protocol == IPPROTO_TCP) {
+		if ((*aa)->u.port.protocol < (*bb)->u.port.protocol) {
 			rc = -1;
-		} else {
+		} else if ((*aa)->u.port.protocol > (*bb)->u.port.protocol) {
 			rc = 1;
 		}
 	}
-- 
2.25.4

Re: [PATCH] libselpol: Sort portcon rules consistently

[Stephen Smalley]

On Thu, May 28, 2020 at 2:41 PM James Carter <jwcart2@gmail.com> wrote:
>
> The comparison function, portcon_data_cmp(), only made use of the
> protocol to put tcp before udp, dccp, and sctp. Rules that have
> the same port range, but with different protocols would be considered
> equal unless one of the protocols was tcp. When generating a CIL or
> conf source policy from a binary or using the "-S" option in
> checkpolicy the non-tcp portcon rules with the same port range would
> not be consistently sorted.
>
> Changed portcon_data_cmp() to sort portcon rules like the CIL function
> cil_post_portcon_compare().
>
> Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Signed-off-by: James Carter <jwcart2@gmail.com>

Any idea why it used that logic previously?  And how does this compare
with sepol_port_compare/compare2() used by libsemanage?
Regardless,
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Re: [PATCH] libselpol: Sort portcon rules consistently

[James Carter]

On Fri, May 29, 2020 at 10:57 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, May 28, 2020 at 2:41 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > The comparison function, portcon_data_cmp(), only made use of the
> > protocol to put tcp before udp, dccp, and sctp. Rules that have
> > the same port range, but with different protocols would be considered
> > equal unless one of the protocols was tcp. When generating a CIL or
> > conf source policy from a binary or using the "-S" option in
> > checkpolicy the non-tcp portcon rules with the same port range would
> > not be consistently sorted.
> >
> > Changed portcon_data_cmp() to sort portcon rules like the CIL function
> > cil_post_portcon_compare().
> >
> > Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > Signed-off-by: James Carter <jwcart2@gmail.com>
>
> Any idea why it used that logic previously?  And how does this compare
> with sepol_port_compare/compare2() used by libsemanage?

It originally followed the logic in CIL. I updated the CIL logic in
2018 (see commit 4ba19b541 ("libsepol/cil: Improve processing of
context rules"), but failed to update the logic in kernel_to_common.

The logic is similar, but slightly different. The logic in CIL and in
kernel_to_common puts smaller port ranges before larger ones, but
sepol_port_compare/compare2() do not take into account the port range.
Other than that they are the same (with this patch). I am not sure
where the CIL ordering logic came from, Steve Lawrence might remember.

Jim

> Regardless,
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Re: [PATCH] libselpol: Sort portcon rules consistently

[Stephen Smalley]

On Fri, May 29, 2020 at 11:35 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Fri, May 29, 2020 at 10:57 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Thu, May 28, 2020 at 2:41 PM James Carter <jwcart2@gmail.com> wrote:
> > >
> > > The comparison function, portcon_data_cmp(), only made use of the
> > > protocol to put tcp before udp, dccp, and sctp. Rules that have
> > > the same port range, but with different protocols would be considered
> > > equal unless one of the protocols was tcp. When generating a CIL or
> > > conf source policy from a binary or using the "-S" option in
> > > checkpolicy the non-tcp portcon rules with the same port range would
> > > not be consistently sorted.
> > >
> > > Changed portcon_data_cmp() to sort portcon rules like the CIL function
> > > cil_post_portcon_compare().
> > >
> > > Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > Signed-off-by: James Carter <jwcart2@gmail.com>
> >
> > Any idea why it used that logic previously?  And how does this compare
> > with sepol_port_compare/compare2() used by libsemanage?
>
> It originally followed the logic in CIL. I updated the CIL logic in
> 2018 (see commit 4ba19b541 ("libsepol/cil: Improve processing of
> context rules"), but failed to update the logic in kernel_to_common.
>
> The logic is similar, but slightly different. The logic in CIL and in
> kernel_to_common puts smaller port ranges before larger ones, but
> sepol_port_compare/compare2() do not take into account the port range.
> Other than that they are the same (with this patch). I am not sure
> where the CIL ordering logic came from, Steve Lawrence might remember.
>
> Jim
>
> > Regardless,
> > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Applied.