| Message ID | 20230309143741.346749-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a171ba62bbba |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [userspace] libsemanage: include more parameters in the module checksum | expand |
On Thu, Mar 9, 2023 at 9:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > The check_ext_changes option currently assumes that as long as the > module content is unchanged, it is safe to assume that the policy.linked > file doesn't need to be rebuilt. However, there are some additional > parameters that can affect the content of this policy file, namely: > * the disable_dontaudit and preserve_tunables flags > * the target_platform and policyvers configuration values > > Include these in the checksum so that the option works correctly when > only some of these input values are changed versus the current state. > > Fixes: 286a679fadc4 ("libsemanage: optionally rebuild policy when modules are changed externally") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> Feel free to merge it. I was wondering if we ought to somehow unify the logic around do_rebuild and check_ext_changes to ensure that an update to one is also reflected in the other but that can be done later. I don't think do_rebuild currently is set based on target_platform or policyvers, likely because we don't ever change the former and we only change the latter for libsepol upgrades that support newer kernel policy versions and the kernel will cheerfully accept the older policy versions (and the new policy version likely won't be leveraged until there is an actual change to a policy module to use some new feature). > --- > libsemanage/src/direct_api.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > index 7aa081ab..d740070d 100644 > --- a/libsemanage/src/direct_api.c > +++ b/libsemanage/src/direct_api.c > @@ -863,6 +863,14 @@ static void update_checksum_with_len(Sha256Context *context, size_t s) > Sha256Update(context, buffer, 8); > } > > +static void update_checksum_with_bool(Sha256Context *context, bool b) > +{ > + uint8_t byte; > + > + byte = b ? UINT8_C(1) : UINT8_C(0); > + Sha256Update(context, &byte, 1); > +} > + > static int semanage_compile_module(semanage_handle_t *sh, > semanage_module_info_t *modinfo, > Sha256Context *context) > @@ -977,13 +985,21 @@ static int modinfo_cmp(const void *a, const void *b) > return strcmp(ma->name, mb->name); > } > > +struct extra_checksum_params { > + int disable_dontaudit; > + int preserve_tunables; > + int target_platform; > + int policyvers; > +}; > + > static int semanage_compile_hll_modules(semanage_handle_t *sh, > semanage_module_info_t *modinfos, > int num_modinfos, > + const struct extra_checksum_params *extra, > char *cil_checksum) > { > /* to be incremented when checksum input data format changes */ > - static const size_t CHECKSUM_EPOCH = 1; > + static const size_t CHECKSUM_EPOCH = 2; > > int i, status = 0; > char cil_path[PATH_MAX]; > @@ -1000,6 +1016,10 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh, > > Sha256Initialise(&context); > update_checksum_with_len(&context, CHECKSUM_EPOCH); > + update_checksum_with_bool(&context, !!extra->disable_dontaudit); > + update_checksum_with_bool(&context, !!extra->preserve_tunables); > + update_checksum_with_len(&context, (size_t)extra->target_platform); > + update_checksum_with_len(&context, (size_t)extra->policyvers); > > /* prefix with module count to avoid collisions */ > update_checksum_with_len(&context, num_modinfos); > @@ -1134,6 +1154,7 @@ static int semanage_direct_commit(semanage_handle_t * sh) > mode_t mask = umask(0077); > struct stat sb; > char modules_checksum[CHECKSUM_CONTENT_SIZE + 1 /* '\0' */]; > + struct extra_checksum_params extra; > > int do_rebuild, do_write_kernel, do_install; > int fcontexts_modified, ports_modified, seusers_modified, > @@ -1274,8 +1295,14 @@ static int semanage_direct_commit(semanage_handle_t * sh) > goto cleanup; > } > > + extra = (struct extra_checksum_params){ > + .disable_dontaudit = sepol_get_disable_dontaudit(sh->sepolh), > + .preserve_tunables = sepol_get_preserve_tunables(sh->sepolh), > + .target_platform = sh->conf->target_platform, > + .policyvers = sh->conf->policyvers, > + }; > retval = semanage_compile_hll_modules(sh, modinfos, num_modinfos, > - modules_checksum); > + &extra, modules_checksum); > if (retval < 0) { > ERR(sh, "Failed to compile hll files into cil files.\n"); > goto cleanup; > -- > 2.39.2 >
On Fri, Mar 17, 2023 at 2:02 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Mar 9, 2023 at 9:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > The check_ext_changes option currently assumes that as long as the > > module content is unchanged, it is safe to assume that the policy.linked > > file doesn't need to be rebuilt. However, there are some additional > > parameters that can affect the content of this policy file, namely: > > * the disable_dontaudit and preserve_tunables flags > > * the target_platform and policyvers configuration values > > > > Include these in the checksum so that the option works correctly when > > only some of these input values are changed versus the current state. > > > > Fixes: 286a679fadc4 ("libsemanage: optionally rebuild policy when modules are changed externally") > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > Feel free to merge it. Thanks, merged: https://github.com/SELinuxProject/selinux/commit/a171ba62bbba891a8dce2239327b1d905f695b82 > I was wondering if we ought to somehow unify > the logic around do_rebuild and check_ext_changes to ensure that an > update to one is also reflected in the other but that can be done > later. I don't think do_rebuild currently is set based on > target_platform or policyvers, likely because we don't ever change the > former and we only change the latter for libsepol upgrades that > support newer kernel policy versions and the kernel will cheerfully > accept the older policy versions (and the new policy version likely > won't be leveraged until there is an actual change to a policy module > to use some new feature). Yeah, I may have gone a bit overboard with those two parameters, but I wanted to be safe rather than sorry.
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c index 7aa081ab..d740070d 100644 --- a/libsemanage/src/direct_api.c +++ b/libsemanage/src/direct_api.c @@ -863,6 +863,14 @@ static void update_checksum_with_len(Sha256Context *context, size_t s) Sha256Update(context, buffer, 8); } +static void update_checksum_with_bool(Sha256Context *context, bool b) +{ + uint8_t byte; + + byte = b ? UINT8_C(1) : UINT8_C(0); + Sha256Update(context, &byte, 1); +} + static int semanage_compile_module(semanage_handle_t *sh, semanage_module_info_t *modinfo, Sha256Context *context) @@ -977,13 +985,21 @@ static int modinfo_cmp(const void *a, const void *b) return strcmp(ma->name, mb->name); } +struct extra_checksum_params { + int disable_dontaudit; + int preserve_tunables; + int target_platform; + int policyvers; +}; + static int semanage_compile_hll_modules(semanage_handle_t *sh, semanage_module_info_t *modinfos, int num_modinfos, + const struct extra_checksum_params *extra, char *cil_checksum) { /* to be incremented when checksum input data format changes */ - static const size_t CHECKSUM_EPOCH = 1; + static const size_t CHECKSUM_EPOCH = 2; int i, status = 0; char cil_path[PATH_MAX]; @@ -1000,6 +1016,10 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh, Sha256Initialise(&context); update_checksum_with_len(&context, CHECKSUM_EPOCH); + update_checksum_with_bool(&context, !!extra->disable_dontaudit); + update_checksum_with_bool(&context, !!extra->preserve_tunables); + update_checksum_with_len(&context, (size_t)extra->target_platform); + update_checksum_with_len(&context, (size_t)extra->policyvers); /* prefix with module count to avoid collisions */ update_checksum_with_len(&context, num_modinfos); @@ -1134,6 +1154,7 @@ static int semanage_direct_commit(semanage_handle_t * sh) mode_t mask = umask(0077); struct stat sb; char modules_checksum[CHECKSUM_CONTENT_SIZE + 1 /* '\0' */]; + struct extra_checksum_params extra; int do_rebuild, do_write_kernel, do_install; int fcontexts_modified, ports_modified, seusers_modified, @@ -1274,8 +1295,14 @@ static int semanage_direct_commit(semanage_handle_t * sh) goto cleanup; } + extra = (struct extra_checksum_params){ + .disable_dontaudit = sepol_get_disable_dontaudit(sh->sepolh), + .preserve_tunables = sepol_get_preserve_tunables(sh->sepolh), + .target_platform = sh->conf->target_platform, + .policyvers = sh->conf->policyvers, + }; retval = semanage_compile_hll_modules(sh, modinfos, num_modinfos, - modules_checksum); + &extra, modules_checksum); if (retval < 0) { ERR(sh, "Failed to compile hll files into cil files.\n"); goto cleanup;
The check_ext_changes option currently assumes that as long as the module content is unchanged, it is safe to assume that the policy.linked file doesn't need to be rebuilt. However, there are some additional parameters that can affect the content of this policy file, namely: * the disable_dontaudit and preserve_tunables flags * the target_platform and policyvers configuration values Include these in the checksum so that the option works correctly when only some of these input values are changed versus the current state. Fixes: 286a679fadc4 ("libsemanage: optionally rebuild policy when modules are changed externally") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- libsemanage/src/direct_api.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)