[PATCH] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[PATCH] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Rahul Sandhu]

In package build/install environments, when semodule(8) is passed the
`--path` option, it is expected that it creates the entire directory
tree for the policy root.

Some package managers warn or error if permissions do not align between
the tree on the existing system and the build environment about to be
merged. To make sure this is a non-issue, create the tree of the policy
root with 0755 permissions (in line with standards for `/var/lib`) and
then chmod the final path to the more restrictive 0700 permissions. As
the contents being placed in the policy root are security sensitive,
erorr instead of warning if we fail to chown the policy root to 0700.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libsemanage/src/semanage_store.c | 58 ++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 1731c5e8..c1425f15 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -491,6 +491,44 @@ char *semanage_conf_path(void)
 	return semanage_conf;
 }
 
+/* Recursively create a directory from a path string.
+ * Returns 0 on success, -errno on failure.
+ */
+static int mkdir_recursive(const char *path, mode_t mode)
+{
+	if (!path || !*path) {
+		return -EINVAL;
+	}
+
+	char path_buffer[PATH_MAX] = {0};
+	size_t len = strlen(path);
+	/* + 1 for nullterm.  */
+	if (len + 1 >= sizeof(path_buffer)) {
+		return -ENAMETOOLONG;
+	}
+
+	strncpy(path_buffer, path, sizeof(path_buffer) - 1);
+
+	/* trim possible trailing slashes, except if '/' is the entire path.  */
+	while (len > 1 && path_buffer[len - 1] == '/') {
+		path_buffer[--len] = '\0';
+	}
+
+	for (char *pos = path_buffer + 1, *slash; (slash = strchr(pos, '/')); pos = slash + 1) {
+		*slash = '\0';
+		if (mkdir(path_buffer, mode) != 0 && errno != EEXIST) {
+			return -errno;
+		}
+		*slash = '/';
+	}
+
+	if (mkdir(path_buffer, mode) != 0 && errno != EEXIST) {
+		return -errno;
+	}
+
+	return 0;
+}
+
 /**************** functions that create module store ***************/
 
 /* Check that the semanage store exists.  If 'create' is non-zero then
@@ -506,14 +544,20 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
-			mask = umask(0077);
-			if (mkdir(path, S_IRWXU) == -1) {
-				umask(mask);
-				ERR(sh, "Could not create module store at %s.",
-				    path);
+			/* First we create directories recursively with standard permissions so that
+			   we don't screw up ownership of toplevel dirs such as `/var` in pkgmgr
+			   environments.  */
+			const int r = mkdir_recursive(path, (mode_t)0755);
+			if (r != 0) {
+				ERR(sh, "Could not create module store at %s: %s.", path, strerror(-r));
+				return -2;
+			}
+			/* Now that we've created the directory tree, we set the permissions of the
+			   target path to 0700. */
+			if (chmod(path, (mode_t)0700) != 0) {
+				ERR(sh, "Failed to chown module store at %s: %s.", path, strerror(errno));
 				return -2;
 			}
-			umask(mask);
 		} else {
 			if (create)
 				ERR(sh,
@@ -529,6 +573,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 			return -1;
 		}
 	}
+	/* We no longer need to use mkdir_recursive at this point: the toplevel
+	   directory heirachy has been created by now.  */
 	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_TOPLEVEL);
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
-- 
2.51.0

Re: [PATCH] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Stephen Smalley]

On Sat, Oct 18, 2025 at 1:20 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> In package build/install environments, when semodule(8) is passed the
> `--path` option, it is expected that it creates the entire directory
> tree for the policy root.
>
> Some package managers warn or error if permissions do not align between
> the tree on the existing system and the build environment about to be
> merged. To make sure this is a non-issue, create the tree of the policy
> root with 0755 permissions (in line with standards for `/var/lib`) and
> then chmod the final path to the more restrictive 0700 permissions. As
> the contents being placed in the policy root are security sensitive,
> erorr instead of warning if we fail to chown the policy root to 0700.

error

>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
>  libsemanage/src/semanage_store.c | 58 ++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 1731c5e8..c1425f15 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -491,6 +491,44 @@ char *semanage_conf_path(void)
>         return semanage_conf;
>  }
>
> +/* Recursively create a directory from a path string.
> + * Returns 0 on success, -errno on failure.
> + */
> +static int mkdir_recursive(const char *path, mode_t mode)
> +{
> +       if (!path || !*path) {
> +               return -EINVAL;
> +       }
> +
> +       char path_buffer[PATH_MAX] = {0};
> +       size_t len = strlen(path);
> +       /* + 1 for nullterm.  */
> +       if (len + 1 >= sizeof(path_buffer)) {

if len == sizeof(path_buffer) - 1, then len + 1 == sizeof(path_buffer)
and this condition will evaluate to true even though the path + NUL
terminator will fit into the buffer, right?

> +               return -ENAMETOOLONG;
> +       }
> +
> +       strncpy(path_buffer, path, sizeof(path_buffer) - 1);

Not sure why "sizeof(path_buffer) - 1" is used as "n" here or why we
even need to use strncpy() at this point. We already know that path
has length len and that len < sizeof(path_buffer), right?

> +
> +       /* trim possible trailing slashes, except if '/' is the entire path.  */
> +       while (len > 1 && path_buffer[len - 1] == '/') {
> +               path_buffer[--len] = '\0';
> +       }
> +
> +       for (char *pos = path_buffer + 1, *slash; (slash = strchr(pos, '/')); pos = slash + 1) {

Assumes that path_buffer originally starts with a "/"? Likely always
true but noting it.

> @@ -529,6 +573,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
>                         return -1;
>                 }
>         }
> +       /* We no longer need to use mkdir_recursive at this point: the toplevel
> +          directory heirachy has been created by now.  */

hierarchy

Re: [PATCH] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Rahul Sandhu]

On Mon Oct 20, 2025 at 2:15 PM BST, Stephen Smalley wrote:
> On Sat, Oct 18, 2025 at 1:20 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>>
>> In package build/install environments, when semodule(8) is passed the
>> `--path` option, it is expected that it creates the entire directory
>> tree for the policy root.
>>
>> Some package managers warn or error if permissions do not align between
>> the tree on the existing system and the build environment about to be
>> merged. To make sure this is a non-issue, create the tree of the policy
>> root with 0755 permissions (in line with standards for `/var/lib`) and
>> then chmod the final path to the more restrictive 0700 permissions. As
>> the contents being placed in the policy root are security sensitive,
>> erorr instead of warning if we fail to chown the policy root to 0700.
>
> error
>
>>
>> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>> ---
>>  libsemanage/src/semanage_store.c | 58 ++++++++++++++++++++++++++++----
>>  1 file changed, 52 insertions(+), 6 deletions(-)
>>
>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>> index 1731c5e8..c1425f15 100644
>> --- a/libsemanage/src/semanage_store.c
>> +++ b/libsemanage/src/semanage_store.c
>> @@ -491,6 +491,44 @@ char *semanage_conf_path(void)
>>         return semanage_conf;
>>  }
>>
>> +/* Recursively create a directory from a path string.
>> + * Returns 0 on success, -errno on failure.
>> + */
>> +static int mkdir_recursive(const char *path, mode_t mode)
>> +{
>> +       if (!path || !*path) {
>> +               return -EINVAL;
>> +       }
>> +
>> +       char path_buffer[PATH_MAX] = {0};
>> +       size_t len = strlen(path);
>> +       /* + 1 for nullterm.  */
>> +       if (len + 1 >= sizeof(path_buffer)) {
>
> if len == sizeof(path_buffer) - 1, then len + 1 == sizeof(path_buffer)
> and this condition will evaluate to true even though the path + NUL
> terminator will fit into the buffer, right?
>

Yea, forgot to change the check from `>=` to `>` when I added the +1 to
make the nullterm more clear. Will update.

>> +               return -ENAMETOOLONG;
>> +       }
>> +
>> +       strncpy(path_buffer, path, sizeof(path_buffer) - 1);
>
> Not sure why "sizeof(path_buffer) - 1" is used as "n" here or why we
> even need to use strncpy() at this point. We already know that path
> has length len and that len < sizeof(path_buffer), right?
>

Should be fine as I initalise with `= {0}`, but it isn't great agreed,
will change this.

>> +
>> +       /* trim possible trailing slashes, except if '/' is the entire path.  */
>> +       while (len > 1 && path_buffer[len - 1] == '/') {
>> +               path_buffer[--len] = '\0';
>> +       }
>> +
>> +       for (char *pos = path_buffer + 1, *slash; (slash = strchr(pos, '/')); pos = slash + 1) {
>
> Assumes that path_buffer originally starts with a "/"? Likely always
> true but noting it.
>

I don't think this is the case, there are two cases here:

1. We start with a /, so `/foo/bar`. In this case, we have the + 1, so
   the `/` is skipped over and we start searching from the `f` character
   and find the next `/` before `bar`. In this case, we set that to '\0'
   so mkdir doesn't read past that, only creating `/foo` (because we did
   not actually modify the first slash in `path_buffer`. We modify pos,
   which is how we calculate where '/' is to set it to '\0'.

2. We don't start with a /, so `foo/bar`. in this case, we have the + 1
   again, but this doesn't actually matter! Sure, pos advances, but we
   never remove or change the initial character in the `path_buffer`, we
   only advance past it when searching for the `/`, which then continues
   the case like we did above, setting the `/` to '\0'.

I might have had some oversight though, please do correct me if I have
missed something.

>> @@ -529,6 +573,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
>>                         return -1;
>>                 }
>>         }
>> +       /* We no longer need to use mkdir_recursive at this point: the toplevel
>> +          directory heirachy has been created by now.  */
>
> hierarchy

Will fix - whoops.

Regards,
Rahul

Re: [PATCH] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Stephen Smalley]

On Mon, Oct 20, 2025 at 9:48 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> On Mon Oct 20, 2025 at 2:15 PM BST, Stephen Smalley wrote:
> > On Sat, Oct 18, 2025 at 1:20 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >>
> >> In package build/install environments, when semodule(8) is passed the
> >> `--path` option, it is expected that it creates the entire directory
> >> tree for the policy root.
> >>
> >> Some package managers warn or error if permissions do not align between
> >> the tree on the existing system and the build environment about to be
> >> merged. To make sure this is a non-issue, create the tree of the policy
> >> root with 0755 permissions (in line with standards for `/var/lib`) and
> >> then chmod the final path to the more restrictive 0700 permissions. As
> >> the contents being placed in the policy root are security sensitive,
> >> erorr instead of warning if we fail to chown the policy root to 0700.
> >
> > error
> >
> >>
> >> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> >> ---
> >>  libsemanage/src/semanage_store.c | 58 ++++++++++++++++++++++++++++----
> >>  1 file changed, 52 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> >> index 1731c5e8..c1425f15 100644
> >> --- a/libsemanage/src/semanage_store.c
> >> +++ b/libsemanage/src/semanage_store.c
> >> @@ -491,6 +491,44 @@ char *semanage_conf_path(void)
> >>         return semanage_conf;
> >>  }
> >>
> >> +/* Recursively create a directory from a path string.
> >> + * Returns 0 on success, -errno on failure.
> >> + */
> >> +static int mkdir_recursive(const char *path, mode_t mode)
> >> +{
> >> +       if (!path || !*path) {
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       char path_buffer[PATH_MAX] = {0};
> >> +       size_t len = strlen(path);
> >> +       /* + 1 for nullterm.  */
> >> +       if (len + 1 >= sizeof(path_buffer)) {
> >
> > if len == sizeof(path_buffer) - 1, then len + 1 == sizeof(path_buffer)
> > and this condition will evaluate to true even though the path + NUL
> > terminator will fit into the buffer, right?
> >
>
> Yea, forgot to change the check from `>=` to `>` when I added the +1 to
> make the nullterm more clear. Will update.
>
> >> +               return -ENAMETOOLONG;
> >> +       }
> >> +
> >> +       strncpy(path_buffer, path, sizeof(path_buffer) - 1);
> >
> > Not sure why "sizeof(path_buffer) - 1" is used as "n" here or why we
> > even need to use strncpy() at this point. We already know that path
> > has length len and that len < sizeof(path_buffer), right?
> >
>
> Should be fine as I initalise with `= {0}`, but it isn't great agreed,
> will change this.
>
> >> +
> >> +       /* trim possible trailing slashes, except if '/' is the entire path.  */
> >> +       while (len > 1 && path_buffer[len - 1] == '/') {
> >> +               path_buffer[--len] = '\0';
> >> +       }
> >> +
> >> +       for (char *pos = path_buffer + 1, *slash; (slash = strchr(pos, '/')); pos = slash + 1) {
> >
> > Assumes that path_buffer originally starts with a "/"? Likely always
> > true but noting it.
> >
>
> I don't think this is the case, there are two cases here:
>
> 1. We start with a /, so `/foo/bar`. In this case, we have the + 1, so
>    the `/` is skipped over and we start searching from the `f` character
>    and find the next `/` before `bar`. In this case, we set that to '\0'
>    so mkdir doesn't read past that, only creating `/foo` (because we did
>    not actually modify the first slash in `path_buffer`. We modify pos,
>    which is how we calculate where '/' is to set it to '\0'.
>
> 2. We don't start with a /, so `foo/bar`. in this case, we have the + 1
>    again, but this doesn't actually matter! Sure, pos advances, but we
>    never remove or change the initial character in the `path_buffer`, we
>    only advance past it when searching for the `/`, which then continues
>    the case like we did above, setting the `/` to '\0'.
>
> I might have had some oversight though, please do correct me if I have
> missed something.

I assumed incorrectly that you are calling mkdir() with pos to create
each directory component in turn. On 2nd look though I see you are
just passing path_buffer each time. But that won't work if you have a
multi-component pathname with more than one directory level, yes?

>
> >> @@ -529,6 +573,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
> >>                         return -1;
> >>                 }
> >>         }
> >> +       /* We no longer need to use mkdir_recursive at this point: the toplevel
> >> +          directory heirachy has been created by now.  */
> >
> > hierarchy
>
> Will fix - whoops.
>
> Regards,
> Rahul

Re: [PATCH] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Stephen Smalley]

On Mon, Oct 20, 2025 at 12:27 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Oct 20, 2025 at 9:48 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > On Mon Oct 20, 2025 at 2:15 PM BST, Stephen Smalley wrote:
> > > On Sat, Oct 18, 2025 at 1:20 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> > >>
> > >> In package build/install environments, when semodule(8) is passed the
> > >> `--path` option, it is expected that it creates the entire directory
> > >> tree for the policy root.
> > >>
> > >> Some package managers warn or error if permissions do not align between
> > >> the tree on the existing system and the build environment about to be
> > >> merged. To make sure this is a non-issue, create the tree of the policy
> > >> root with 0755 permissions (in line with standards for `/var/lib`) and
> > >> then chmod the final path to the more restrictive 0700 permissions. As
> > >> the contents being placed in the policy root are security sensitive,
> > >> erorr instead of warning if we fail to chown the policy root to 0700.
> > >
> > > error
> > >
> > >>
> > >> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> > >> ---
> > >>  libsemanage/src/semanage_store.c | 58 ++++++++++++++++++++++++++++----
> > >>  1 file changed, 52 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> > >> index 1731c5e8..c1425f15 100644
> > >> --- a/libsemanage/src/semanage_store.c
> > >> +++ b/libsemanage/src/semanage_store.c
> > >> @@ -491,6 +491,44 @@ char *semanage_conf_path(void)
> > >>         return semanage_conf;
> > >>  }
> > >>
> > >> +/* Recursively create a directory from a path string.
> > >> + * Returns 0 on success, -errno on failure.
> > >> + */
> > >> +static int mkdir_recursive(const char *path, mode_t mode)
> > >> +{
> > >> +       if (!path || !*path) {
> > >> +               return -EINVAL;
> > >> +       }
> > >> +
> > >> +       char path_buffer[PATH_MAX] = {0};
> > >> +       size_t len = strlen(path);
> > >> +       /* + 1 for nullterm.  */
> > >> +       if (len + 1 >= sizeof(path_buffer)) {
> > >
> > > if len == sizeof(path_buffer) - 1, then len + 1 == sizeof(path_buffer)
> > > and this condition will evaluate to true even though the path + NUL
> > > terminator will fit into the buffer, right?
> > >
> >
> > Yea, forgot to change the check from `>=` to `>` when I added the +1 to
> > make the nullterm more clear. Will update.
> >
> > >> +               return -ENAMETOOLONG;
> > >> +       }
> > >> +
> > >> +       strncpy(path_buffer, path, sizeof(path_buffer) - 1);
> > >
> > > Not sure why "sizeof(path_buffer) - 1" is used as "n" here or why we
> > > even need to use strncpy() at this point. We already know that path
> > > has length len and that len < sizeof(path_buffer), right?
> > >
> >
> > Should be fine as I initalise with `= {0}`, but it isn't great agreed,
> > will change this.
> >
> > >> +
> > >> +       /* trim possible trailing slashes, except if '/' is the entire path.  */
> > >> +       while (len > 1 && path_buffer[len - 1] == '/') {
> > >> +               path_buffer[--len] = '\0';
> > >> +       }
> > >> +
> > >> +       for (char *pos = path_buffer + 1, *slash; (slash = strchr(pos, '/')); pos = slash + 1) {
> > >
> > > Assumes that path_buffer originally starts with a "/"? Likely always
> > > true but noting it.
> > >
> >
> > I don't think this is the case, there are two cases here:
> >
> > 1. We start with a /, so `/foo/bar`. In this case, we have the + 1, so
> >    the `/` is skipped over and we start searching from the `f` character
> >    and find the next `/` before `bar`. In this case, we set that to '\0'
> >    so mkdir doesn't read past that, only creating `/foo` (because we did
> >    not actually modify the first slash in `path_buffer`. We modify pos,
> >    which is how we calculate where '/' is to set it to '\0'.
> >
> > 2. We don't start with a /, so `foo/bar`. in this case, we have the + 1
> >    again, but this doesn't actually matter! Sure, pos advances, but we
> >    never remove or change the initial character in the `path_buffer`, we
> >    only advance past it when searching for the `/`, which then continues
> >    the case like we did above, setting the `/` to '\0'.
> >
> > I might have had some oversight though, please do correct me if I have
> > missed something.
>
> I assumed incorrectly that you are calling mkdir() with pos to create
> each directory component in turn. On 2nd look though I see you are
> just passing path_buffer each time. But that won't work if you have a
> multi-component pathname with more than one directory level, yes?

Oh, never mind - I just misread the code again.

>
> >
> > >> @@ -529,6 +573,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
> > >>                         return -1;
> > >>                 }
> > >>         }
> > >> +       /* We no longer need to use mkdir_recursive at this point: the toplevel
> > >> +          directory heirachy has been created by now.  */
> > >
> > > hierarchy
> >
> > Will fix - whoops.
> >
> > Regards,
> > Rahul

[PATCH v2] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Rahul Sandhu]

In package build/install environments, when semodule(8) is passed the
`--path` option, it is expected that it creates the entire directory
tree for the policy root.

Some package managers warn or error if permissions do not align between
the tree on the existing system and the build environment about to be
merged. To make sure this is a non-issue, create the tree of the policy
root with 0755 permissions (in line with standards for `/var/lib`) and
then chmod the final path to the more restrictive 0700 permissions. As
the contents being placed in the policy root are security sensitive,
error instead of warning if we fail to chown the policy root to 0700.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libsemanage/src/semanage_store.c | 59 ++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 6 deletions(-)

v2: fix-up buffer length check, use memcpy instead of strncpy: we check
    the size already.

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 1731c5e8..e3048c08 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -491,6 +491,45 @@ char *semanage_conf_path(void)
 	return semanage_conf;
 }
 
+/* Recursively create a directory from a path string.
+ * Returns 0 on success, -errno on failure.
+ */
+static int mkdir_recursive(const char *path, mode_t mode)
+{
+	if (!path || !*path) {
+		return -EINVAL;
+	}
+
+	char path_buffer[PATH_MAX] = {0};
+	size_t len = strlen(path);
+	/* + 1 for nullterm.  */
+	if (len + 1 > sizeof(path_buffer)) {
+		return -ENAMETOOLONG;
+	}
+
+	/* + 1 for nullterm.  */
+	memcpy(path_buffer, path, len + 1);
+
+	/* trim possible trailing slashes, except if '/' is the entire path.  */
+	while (len > 1 && path_buffer[len - 1] == '/') {
+		path_buffer[--len] = '\0';
+	}
+
+	for (char *pos = path_buffer + 1, *slash; (slash = strchr(pos, '/')); pos = slash + 1) {
+		*slash = '\0';
+		if (mkdir(path_buffer, mode) != 0 && errno != EEXIST) {
+			return -errno;
+		}
+		*slash = '/';
+	}
+
+	if (mkdir(path_buffer, mode) != 0 && errno != EEXIST) {
+		return -errno;
+	}
+
+	return 0;
+}
+
 /**************** functions that create module store ***************/
 
 /* Check that the semanage store exists.  If 'create' is non-zero then
@@ -506,14 +545,20 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
-			mask = umask(0077);
-			if (mkdir(path, S_IRWXU) == -1) {
-				umask(mask);
-				ERR(sh, "Could not create module store at %s.",
-				    path);
+			/* First we create directories recursively with standard permissions so that
+			   we don't screw up ownership of toplevel dirs such as `/var` in pkgmgr
+			   environments.  */
+			const int r = mkdir_recursive(path, (mode_t)0755);
+			if (r != 0) {
+				ERR(sh, "Could not create module store at %s: %s.", path, strerror(-r));
+				return -2;
+			}
+			/* Now that we've created the directory tree, we set the permissions of the
+			   target path to 0700. */
+			if (chmod(path, (mode_t)0700) != 0) {
+				ERR(sh, "Failed to chown module store at %s: %s.", path, strerror(errno));
 				return -2;
 			}
-			umask(mask);
 		} else {
 			if (create)
 				ERR(sh,
@@ -529,6 +574,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 			return -1;
 		}
 	}
+	/* We no longer need to use mkdir_recursive at this point: the toplevel
+	   directory hierarchy has been created by now.  */
 	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_TOPLEVEL);
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
-- 
2.51.0

Re: [PATCH v2] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Stephen Smalley]

On Mon, Oct 20, 2025 at 1:42 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> In package build/install environments, when semodule(8) is passed the
> `--path` option, it is expected that it creates the entire directory
> tree for the policy root.
>
> Some package managers warn or error if permissions do not align between
> the tree on the existing system and the build environment about to be
> merged. To make sure this is a non-issue, create the tree of the policy
> root with 0755 permissions (in line with standards for `/var/lib`) and
> then chmod the final path to the more restrictive 0700 permissions. As
> the contents being placed in the policy root are security sensitive,
> error instead of warning if we fail to chown the policy root to 0700.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>

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

> ---
>  libsemanage/src/semanage_store.c | 59 ++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 6 deletions(-)
>
> v2: fix-up buffer length check, use memcpy instead of strncpy: we check
>     the size already.
>
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 1731c5e8..e3048c08 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -491,6 +491,45 @@ char *semanage_conf_path(void)
>         return semanage_conf;
>  }
>
> +/* Recursively create a directory from a path string.
> + * Returns 0 on success, -errno on failure.
> + */
> +static int mkdir_recursive(const char *path, mode_t mode)
> +{
> +       if (!path || !*path) {
> +               return -EINVAL;
> +       }
> +
> +       char path_buffer[PATH_MAX] = {0};
> +       size_t len = strlen(path);
> +       /* + 1 for nullterm.  */
> +       if (len + 1 > sizeof(path_buffer)) {
> +               return -ENAMETOOLONG;
> +       }
> +
> +       /* + 1 for nullterm.  */
> +       memcpy(path_buffer, path, len + 1);
> +
> +       /* trim possible trailing slashes, except if '/' is the entire path.  */
> +       while (len > 1 && path_buffer[len - 1] == '/') {
> +               path_buffer[--len] = '\0';
> +       }
> +
> +       for (char *pos = path_buffer + 1, *slash; (slash = strchr(pos, '/')); pos = slash + 1) {
> +               *slash = '\0';
> +               if (mkdir(path_buffer, mode) != 0 && errno != EEXIST) {
> +                       return -errno;
> +               }
> +               *slash = '/';
> +       }
> +
> +       if (mkdir(path_buffer, mode) != 0 && errno != EEXIST) {
> +               return -errno;
> +       }
> +
> +       return 0;
> +}
> +
>  /**************** functions that create module store ***************/
>
>  /* Check that the semanage store exists.  If 'create' is non-zero then
> @@ -506,14 +545,20 @@ int semanage_create_store(semanage_handle_t * sh, int create)
>
>         if (stat(path, &sb) == -1) {
>                 if (errno == ENOENT && create) {
> -                       mask = umask(0077);
> -                       if (mkdir(path, S_IRWXU) == -1) {
> -                               umask(mask);
> -                               ERR(sh, "Could not create module store at %s.",
> -                                   path);
> +                       /* First we create directories recursively with standard permissions so that
> +                          we don't screw up ownership of toplevel dirs such as `/var` in pkgmgr
> +                          environments.  */
> +                       const int r = mkdir_recursive(path, (mode_t)0755);
> +                       if (r != 0) {
> +                               ERR(sh, "Could not create module store at %s: %s.", path, strerror(-r));
> +                               return -2;
> +                       }
> +                       /* Now that we've created the directory tree, we set the permissions of the
> +                          target path to 0700. */
> +                       if (chmod(path, (mode_t)0700) != 0) {
> +                               ERR(sh, "Failed to chown module store at %s: %s.", path, strerror(errno));
>                                 return -2;
>                         }
> -                       umask(mask);
>                 } else {
>                         if (create)
>                                 ERR(sh,
> @@ -529,6 +574,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
>                         return -1;
>                 }
>         }
> +       /* We no longer need to use mkdir_recursive at this point: the toplevel
> +          directory hierarchy has been created by now.  */
>         path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_TOPLEVEL);
>         if (stat(path, &sb) == -1) {
>                 if (errno == ENOENT && create) {
> --
> 2.51.0
>

Re: [PATCH v2] libsemanage: semanage_store: recursively create SEMANAGE_ROOT

[Stephen Smalley]

On Tue, Oct 21, 2025 at 8:59 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Oct 20, 2025 at 1:42 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > In package build/install environments, when semodule(8) is passed the
> > `--path` option, it is expected that it creates the entire directory
> > tree for the policy root.
> >
> > Some package managers warn or error if permissions do not align between
> > the tree on the existing system and the build environment about to be
> > merged. To make sure this is a non-issue, create the tree of the policy
> > root with 0755 permissions (in line with standards for `/var/lib`) and
> > then chmod the final path to the more restrictive 0700 permissions. As
> > the contents being placed in the policy root are security sensitive,
> > error instead of warning if we fail to chown the policy root to 0700.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Thanks, merged.