[PATCH 1/2 v2] checkpolicy: Do not automatically upgrade when using "-b" flag

[PATCH 1/2 v2] checkpolicy: Do not automatically upgrade when using "-b" flag

[James Carter]

When reading a binary policy, do not automatically change the version
to the max policy version supported by libsepol or, if specified, the
value given using the "-c" flag.

If the binary policy version is less than or equal to version 23
(POLICYDB_VERSION_PERMISSIVE) than do not automatically upgrade the
policy and if a policy version is specified by the "-c" flag, only set
the binary policy to the specified version if it is lower than the
current version.

If the binary policy version is greater than version 23 than it should
be set to the maximum version supported by libsepol or, if specified,
the value given by the "-c" flag.

The reason for this change is that policy versions 20
(POLICYDB_VERSION_AVTAB) to 23 have a more primitive support for type
attributes where the datums are not written out, but they exist in the
type_attr_map. This means that when the binary policy is read by
libsepol, there will be gaps in the type_val_to_struct and
p_type_val_to_name arrays and policy rules can refer to those gaps.
Certain libsepol functions like sepol_kernel_policydb_to_conf() and
sepol_kernel_policydb_to_cil() do not support this behavior and need
to be able to identify these policies. Policies before version 20 do not
support attributes at all and can be handled by all libsepol functions.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
v2 - Give the proper value when printing the compatibility range

 checkpolicy/checkpolicy.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
index 5841c5c4..acf1eac4 100644
--- a/checkpolicy/checkpolicy.c
+++ b/checkpolicy/checkpolicy.c
@@ -106,7 +106,7 @@ static int handle_unknown = SEPOL_DENY_UNKNOWN;
 static const char *txtfile = "policy.conf";
 static const char *binfile = "policy";
 
-unsigned int policyvers = POLICYDB_VERSION_MAX;
+unsigned int policyvers = 0;
 
 static __attribute__((__noreturn__)) void usage(const char *progname)
 {
@@ -515,7 +515,8 @@ int main(int argc, char **argv)
 	}
 
 	if (show_version) {
-		printf("%d (compatibility range %d-%d)\n", policyvers,
+		printf("%d (compatibility range %d-%d)\n",
+			   policyvers ? policyvers : POLICYDB_VERSION_MAX ,
 		       POLICYDB_VERSION_MAX, POLICYDB_VERSION_MIN);
 		exit(0);
 	}
@@ -588,6 +589,16 @@ int main(int argc, char **argv)
 				exit(1);
 			}
 		}
+
+		if (policydbp->policyvers <= POLICYDB_VERSION_PERMISSIVE) {
+			if (policyvers > policydbp->policyvers) {
+				fprintf(stderr, "Binary policies with version <= %u cannot be upgraded\n", POLICYDB_VERSION_PERMISSIVE);
+			} else if (policyvers) {
+				policydbp->policyvers = policyvers;
+			}
+		} else {
+			policydbp->policyvers = policyvers ? policyvers : POLICYDB_VERSION_MAX;
+		}
 	} else {
 		if (conf) {
 			fprintf(stderr, "Can only generate policy.conf from binary policy\n");
@@ -629,6 +640,8 @@ int main(int argc, char **argv)
 			policydb_destroy(policydbp);
 			policydbp = &policydb;
 		}
+
+		policydbp->policyvers = policyvers ? policyvers : POLICYDB_VERSION_MAX;
 	}
 
 	if (policydb_load_isids(&policydb, &sidtab))
@@ -654,8 +667,6 @@ int main(int argc, char **argv)
 			}
 		}
 
-		policydb.policyvers = policyvers;
-
 		if (!cil) {
 			if (!conf) {
 				policydb.policy_type = POLICY_KERN;
-- 
2.26.2

[PATCH 2/2 v2] libsepol: Check kernel to CIL and Conf functions for supported versions

[James Carter]

For policy versions between 20 and 23, attributes exist in the
policy, but only in the type_attr_map. This means that there are
gaps in both the type_val_to_struct and p_type_val_to_name arrays
and policy rules can refer to those gaps which can lead to NULL
dereferences when using sepol_kernel_policydb_to_conf() and
sepol_kernel_policydb_to_cil().

This can be seen with the following policy:
  class CLASS1
  sid SID1
  class CLASS1 { PERM1 }
  attribute TYPE_ATTR1;
  type TYPE1;
  typeattribute TYPE1 TYPE_ATTR1;
  allow TYPE_ATTR1 self : CLASS1 PERM1;
  role ROLE1;
  role ROLE1 types TYPE1;
  user USER1 roles ROLE1;
  sid SID1 USER1:ROLE1:TYPE1

Compile the policy:
  checkpolicy -c 23 -o policy.bin policy.conf
Converting back to a policy.conf causes a segfault:
  checkpolicy -F -b -o policy.bin.conf policy.bin

Have both sepol_kernel_policydb_to_conf() and
sepol_kernel_policydb_to_cil() exit with an error if the kernel
policy version is between 20 and 23.

Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
v2 - No changes

 libsepol/src/kernel_to_cil.c  | 12 ++++++++++++
 libsepol/src/kernel_to_conf.c | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index a146ac51..edfebeaf 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -3164,6 +3164,18 @@ int sepol_kernel_policydb_to_cil(FILE *out, struct policydb *pdb)
 		goto exit;
 	}
 
+	if (pdb->policyvers >= POLICYDB_VERSION_AVTAB && pdb->policyvers <= POLICYDB_VERSION_PERMISSIVE) {
+		/*
+		 * For policy versions between 20 and 23, attributes exist in the policy,
+		 * but only in the type_attr_map. This means that there are gaps in both
+		 * the type_val_to_struct and p_type_val_to_name arrays and policy rules
+		 * can refer to those gaps.
+		 */
+		sepol_log_err("Writing policy versions between 20 and 23 as CIL is not supported");
+		rc = -1;
+		goto exit;
+	}
+
 	rc = constraint_rules_to_strs(pdb, mls_constraints, non_mls_constraints);
 	if (rc != 0) {
 		goto exit;
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index a22f196d..ea58a026 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -3041,6 +3041,18 @@ int sepol_kernel_policydb_to_conf(FILE *out, struct policydb *pdb)
 		goto exit;
 	}
 
+	if (pdb->policyvers >= POLICYDB_VERSION_AVTAB && pdb->policyvers <= POLICYDB_VERSION_PERMISSIVE) {
+		/*
+		 * For policy versions between 20 and 23, attributes exist in the policy,
+		 * but only in the type_attr_map. This means that there are gaps in both
+		 * the type_val_to_struct and p_type_val_to_name arrays and policy rules
+		 * can refer to those gaps.
+		 */
+		sepol_log_err("Writing policy versions between 20 and 23 as a policy.conf is not supported");
+		rc = -1;
+		goto exit;
+	}
+
 	rc = constraint_rules_to_strs(pdb, mls_constraints, non_mls_constraints);
 	if (rc != 0) {
 		goto exit;
-- 
2.26.2

Re: [PATCH 1/2 v2] checkpolicy: Do not automatically upgrade when using "-b" flag

[Nicolas Iooss]

On Mon, Mar 15, 2021 at 4:10 PM James Carter <jwcart2@gmail.com> wrote:
>
> When reading a binary policy, do not automatically change the version
> to the max policy version supported by libsepol or, if specified, the
> value given using the "-c" flag.
>
> If the binary policy version is less than or equal to version 23
> (POLICYDB_VERSION_PERMISSIVE) than do not automatically upgrade the
> policy and if a policy version is specified by the "-c" flag, only set
> the binary policy to the specified version if it is lower than the
> current version.
>
> If the binary policy version is greater than version 23 than it should
> be set to the maximum version supported by libsepol or, if specified,
> the value given by the "-c" flag.
>
> The reason for this change is that policy versions 20
> (POLICYDB_VERSION_AVTAB) to 23 have a more primitive support for type
> attributes where the datums are not written out, but they exist in the
> type_attr_map. This means that when the binary policy is read by
> libsepol, there will be gaps in the type_val_to_struct and
> p_type_val_to_name arrays and policy rules can refer to those gaps.
> Certain libsepol functions like sepol_kernel_policydb_to_conf() and
> sepol_kernel_policydb_to_cil() do not support this behavior and need
> to be able to identify these policies. Policies before version 20 do not
> support attributes at all and can be handled by all libsepol functions.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
> v2 - Give the proper value when printing the compatibility range

For both patches:
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Thanks!
Nicolas

>  checkpolicy/checkpolicy.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
> index 5841c5c4..acf1eac4 100644
> --- a/checkpolicy/checkpolicy.c
> +++ b/checkpolicy/checkpolicy.c
> @@ -106,7 +106,7 @@ static int handle_unknown = SEPOL_DENY_UNKNOWN;
>  static const char *txtfile = "policy.conf";
>  static const char *binfile = "policy";
>
> -unsigned int policyvers = POLICYDB_VERSION_MAX;
> +unsigned int policyvers = 0;
>
>  static __attribute__((__noreturn__)) void usage(const char *progname)
>  {
> @@ -515,7 +515,8 @@ int main(int argc, char **argv)
>         }
>
>         if (show_version) {
> -               printf("%d (compatibility range %d-%d)\n", policyvers,
> +               printf("%d (compatibility range %d-%d)\n",
> +                          policyvers ? policyvers : POLICYDB_VERSION_MAX ,
>                        POLICYDB_VERSION_MAX, POLICYDB_VERSION_MIN);
>                 exit(0);
>         }
> @@ -588,6 +589,16 @@ int main(int argc, char **argv)
>                                 exit(1);
>                         }
>                 }
> +
> +               if (policydbp->policyvers <= POLICYDB_VERSION_PERMISSIVE) {
> +                       if (policyvers > policydbp->policyvers) {
> +                               fprintf(stderr, "Binary policies with version <= %u cannot be upgraded\n", POLICYDB_VERSION_PERMISSIVE);
> +                       } else if (policyvers) {
> +                               policydbp->policyvers = policyvers;
> +                       }
> +               } else {
> +                       policydbp->policyvers = policyvers ? policyvers : POLICYDB_VERSION_MAX;
> +               }
>         } else {
>                 if (conf) {
>                         fprintf(stderr, "Can only generate policy.conf from binary policy\n");
> @@ -629,6 +640,8 @@ int main(int argc, char **argv)
>                         policydb_destroy(policydbp);
>                         policydbp = &policydb;
>                 }
> +
> +               policydbp->policyvers = policyvers ? policyvers : POLICYDB_VERSION_MAX;
>         }
>
>         if (policydb_load_isids(&policydb, &sidtab))
> @@ -654,8 +667,6 @@ int main(int argc, char **argv)
>                         }
>                 }
>
> -               policydb.policyvers = policyvers;
> -
>                 if (!cil) {
>                         if (!conf) {
>                                 policydb.policy_type = POLICY_KERN;
> --
> 2.26.2
>

Re: [PATCH 1/2 v2] checkpolicy: Do not automatically upgrade when using "-b" flag

[Nicolas Iooss]

On Mon, Mar 15, 2021 at 10:10 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Mon, Mar 15, 2021 at 4:10 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > When reading a binary policy, do not automatically change the version
> > to the max policy version supported by libsepol or, if specified, the
> > value given using the "-c" flag.
> >
> > If the binary policy version is less than or equal to version 23
> > (POLICYDB_VERSION_PERMISSIVE) than do not automatically upgrade the
> > policy and if a policy version is specified by the "-c" flag, only set
> > the binary policy to the specified version if it is lower than the
> > current version.
> >
> > If the binary policy version is greater than version 23 than it should
> > be set to the maximum version supported by libsepol or, if specified,
> > the value given by the "-c" flag.
> >
> > The reason for this change is that policy versions 20
> > (POLICYDB_VERSION_AVTAB) to 23 have a more primitive support for type
> > attributes where the datums are not written out, but they exist in the
> > type_attr_map. This means that when the binary policy is read by
> > libsepol, there will be gaps in the type_val_to_struct and
> > p_type_val_to_name arrays and policy rules can refer to those gaps.
> > Certain libsepol functions like sepol_kernel_policydb_to_conf() and
> > sepol_kernel_policydb_to_cil() do not support this behavior and need
> > to be able to identify these policies. Policies before version 20 do not
> > support attributes at all and can be handled by all libsepol functions.
> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> > v2 - Give the proper value when printing the compatibility range
>
> For both patches:
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Merged.

Thanks!
Nicolas

> >  checkpolicy/checkpolicy.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
> > index 5841c5c4..acf1eac4 100644
> > --- a/checkpolicy/checkpolicy.c
> > +++ b/checkpolicy/checkpolicy.c
> > @@ -106,7 +106,7 @@ static int handle_unknown = SEPOL_DENY_UNKNOWN;
> >  static const char *txtfile = "policy.conf";
> >  static const char *binfile = "policy";
> >
> > -unsigned int policyvers = POLICYDB_VERSION_MAX;
> > +unsigned int policyvers = 0;
> >
> >  static __attribute__((__noreturn__)) void usage(const char *progname)
> >  {
> > @@ -515,7 +515,8 @@ int main(int argc, char **argv)
> >         }
> >
> >         if (show_version) {
> > -               printf("%d (compatibility range %d-%d)\n", policyvers,
> > +               printf("%d (compatibility range %d-%d)\n",
> > +                          policyvers ? policyvers : POLICYDB_VERSION_MAX ,
> >                        POLICYDB_VERSION_MAX, POLICYDB_VERSION_MIN);
> >                 exit(0);
> >         }
> > @@ -588,6 +589,16 @@ int main(int argc, char **argv)
> >                                 exit(1);
> >                         }
> >                 }
> > +
> > +               if (policydbp->policyvers <= POLICYDB_VERSION_PERMISSIVE) {
> > +                       if (policyvers > policydbp->policyvers) {
> > +                               fprintf(stderr, "Binary policies with version <= %u cannot be upgraded\n", POLICYDB_VERSION_PERMISSIVE);
> > +                       } else if (policyvers) {
> > +                               policydbp->policyvers = policyvers;
> > +                       }
> > +               } else {
> > +                       policydbp->policyvers = policyvers ? policyvers : POLICYDB_VERSION_MAX;
> > +               }
> >         } else {
> >                 if (conf) {
> >                         fprintf(stderr, "Can only generate policy.conf from binary policy\n");
> > @@ -629,6 +640,8 @@ int main(int argc, char **argv)
> >                         policydb_destroy(policydbp);
> >                         policydbp = &policydb;
> >                 }
> > +
> > +               policydbp->policyvers = policyvers ? policyvers : POLICYDB_VERSION_MAX;
> >         }
> >
> >         if (policydb_load_isids(&policydb, &sidtab))
> > @@ -654,8 +667,6 @@ int main(int argc, char **argv)
> >                         }
> >                 }
> >
> > -               policydb.policyvers = policyvers;
> > -
> >                 if (!cil) {
> >                         if (!conf) {
> >                                 policydb.policy_type = POLICY_KERN;
> > --
> > 2.26.2
> >