[PATCH] libsemanage: fsync before rename

[PATCH] libsemanage: fsync before rename

[Smalley]

From: Stephen Smalley <stephen.smalley.work@gmail.com>

Prior to rename(2)'ing new files into place, fsync(2) them to ensure
the contents will be fully written prior to rename.  While we are here,
also fix checking of write(2) to detect short writes.  This code could
be more generally improved but keeping to the minimal changes required
to fix this bug.

Fixes: https://github.com/SELinuxProject/selinux/issues/237
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 libsemanage/src/semanage_store.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 859c0a22..3cac36d4 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -735,7 +735,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode)
 	}
 	umask(mask);
 	while (retval == 0 && (amount_read = read(in, buf, sizeof(buf))) > 0) {
-		if (write(out, buf, amount_read) < 0) {
+		if (write(out, buf, amount_read) != amount_read) {
 			errsv = errno;
 			retval = -1;
 		}
@@ -745,6 +745,10 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode)
 		retval = -1;
 	}
 	close(in);
+	if (fsync(out) < 0) {
+		errsv = errno;
+		retval = -1;
+	}
 	if (close(out) < 0) {
 		errsv = errno;
 		retval = -1;
-- 
2.23.3

Re: [PATCH] libsemanage: fsync before rename

[Nicolas Iooss]

On Wed, May 13, 2020 at 8:46 PM Smalley <stephen.smalley.work@gmail.com> wrote:
>
> From: Stephen Smalley <stephen.smalley.work@gmail.com>
>
> Prior to rename(2)'ing new files into place, fsync(2) them to ensure
> the contents will be fully written prior to rename.  While we are here,
> also fix checking of write(2) to detect short writes.  This code could
> be more generally improved but keeping to the minimal changes required
> to fix this bug.
>
> Fixes: https://github.com/SELinuxProject/selinux/issues/237
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
>  libsemanage/src/semanage_store.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 859c0a22..3cac36d4 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -735,7 +735,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode)
>         }
>         umask(mask);
>         while (retval == 0 && (amount_read = read(in, buf, sizeof(buf))) > 0) {
> -               if (write(out, buf, amount_read) < 0) {
> +               if (write(out, buf, amount_read) != amount_read) {
>                         errsv = errno;
>                         retval = -1;
>                 }

If I remember correctly, errno is not defined if a short write
happens. If this is confirmed and if you want to keep the patch short,
you could for example use errsv = EIO if write() returned a value
different from -1 and from amount_read.

Thanks,
Nicolas

> @@ -745,6 +745,10 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode)
>                 retval = -1;
>         }
>         close(in);
> +       if (fsync(out) < 0) {
> +               errsv = errno;
> +               retval = -1;
> +       }
>         if (close(out) < 0) {
>                 errsv = errno;
>                 retval = -1;
> --
> 2.23.3
>

Re: [PATCH] libsemanage: fsync before rename

[Stephen Smalley]

On Wed, May 13, 2020 at 2:52 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Wed, May 13, 2020 at 8:46 PM Smalley <stephen.smalley.work@gmail.com> wrote:
> >
> > From: Stephen Smalley <stephen.smalley.work@gmail.com>
> >
> > Prior to rename(2)'ing new files into place, fsync(2) them to ensure
> > the contents will be fully written prior to rename.  While we are here,
> > also fix checking of write(2) to detect short writes.  This code could
> > be more generally improved but keeping to the minimal changes required
> > to fix this bug.
> >
> > Fixes: https://github.com/SELinuxProject/selinux/issues/237
> > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > ---
> >  libsemanage/src/semanage_store.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> > index 859c0a22..3cac36d4 100644
> > --- a/libsemanage/src/semanage_store.c
> > +++ b/libsemanage/src/semanage_store.c
> > @@ -735,7 +735,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode)
> >         }
> >         umask(mask);
> >         while (retval == 0 && (amount_read = read(in, buf, sizeof(buf))) > 0) {
> > -               if (write(out, buf, amount_read) < 0) {
> > +               if (write(out, buf, amount_read) != amount_read) {
> >                         errsv = errno;
> >                         retval = -1;
> >                 }
>
> If I remember correctly, errno is not defined if a short write
> happens. If this is confirmed and if you want to keep the patch short,
> you could for example use errsv = EIO if write() returned a value
> different from -1 and from amount_read.

True.  It also occurred to me that this is too heavyweight given how
widely semanage_copy_file() is used within libsemanage; performing a
fsync(2) for every one of these file copies will be very expensive.
We only really care about it when copying the final files to
/etc/selinux, not for the copying under /var/lib/selinux IIUC.  So I
guess I need a bool argument or similar to semanage_copy_file() to
indicate when a fsync is required.  Even with this change, installing
the final SELinux policy files under /etc/selinux won't be fully
atomic; one can still end up with a mix of old and new (e.g. new
policy.32 with old file_contexts).  But that's out of scope for this
particular bug.