diff mbox series

semodule: avoid toctou on output module

Message ID 20220520131952.12286-1-cgzones@googlemail.com (mailing list archive)
State New
Headers show
Series semodule: avoid toctou on output module | expand

Commit Message

Christian Göttsche May 20, 2022, 1:19 p.m. UTC
Do not check for file existence and open afterwards, open with the
exclusive flag (supported in Glibc and musl 0.9.6 and also standardized
in C11).

Found by GitHub CodeQL.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 policycoreutils/semodule/semodule.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Nicolas Iooss May 29, 2022, 10:52 p.m. UTC | #1
On Fri, May 20, 2022 at 3:20 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Do not check for file existence and open afterwards, open with the
> exclusive flag (supported in Glibc and musl 0.9.6 and also standardized
> in C11).
>
> Found by GitHub CodeQL.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

This looks good to me.

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

Thanks!

> ---
>  policycoreutils/semodule/semodule.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/policycoreutils/semodule/semodule.c b/policycoreutils/semodule/semodule.c
> index 1ed8e690..48bc28dd 100644
> --- a/policycoreutils/semodule/semodule.c
> +++ b/policycoreutils/semodule/semodule.c
> @@ -550,15 +550,12 @@ int main(int argc, char *argv[])
>                                         goto cleanup_extract;
>                                 }
>
> -                               if (access(output_path, F_OK) == 0) {
> -                                       fprintf(stderr, "%s: %s is already extracted with extension %s.\n", argv[0], mode_arg, lang_ext);
> -                                       result = -1;
> -                                       goto cleanup_extract;
> -                               }
> -
> -                               output_fd = fopen(output_path, "w");
> +                               output_fd = fopen(output_path, "wx");
>                                 if (output_fd == NULL) {
> -                                       fprintf(stderr, "%s: Unable to open %s\n", argv[0], output_path);
> +                                       if (errno == EEXIST)
> +                                               fprintf(stderr, "%s: %s is already extracted with extension %s.\n", argv[0], mode_arg, lang_ext);
> +                                       else
> +                                               fprintf(stderr, "%s: Unable to open %s:  %s\n", argv[0], output_path, strerror(errno));
>                                         result = -1;
>                                         goto cleanup_extract;
>                                 }
> --
> 2.36.1
>
James Carter June 2, 2022, 1:04 p.m. UTC | #2
On Mon, May 30, 2022 at 1:00 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Fri, May 20, 2022 at 3:20 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Do not check for file existence and open afterwards, open with the
> > exclusive flag (supported in Glibc and musl 0.9.6 and also standardized
> > in C11).
> >
> > Found by GitHub CodeQL.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> This looks good to me.
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>

Merged.
Thanks,
Jim

> Thanks!
>
> > ---
> >  policycoreutils/semodule/semodule.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/policycoreutils/semodule/semodule.c b/policycoreutils/semodule/semodule.c
> > index 1ed8e690..48bc28dd 100644
> > --- a/policycoreutils/semodule/semodule.c
> > +++ b/policycoreutils/semodule/semodule.c
> > @@ -550,15 +550,12 @@ int main(int argc, char *argv[])
> >                                         goto cleanup_extract;
> >                                 }
> >
> > -                               if (access(output_path, F_OK) == 0) {
> > -                                       fprintf(stderr, "%s: %s is already extracted with extension %s.\n", argv[0], mode_arg, lang_ext);
> > -                                       result = -1;
> > -                                       goto cleanup_extract;
> > -                               }
> > -
> > -                               output_fd = fopen(output_path, "w");
> > +                               output_fd = fopen(output_path, "wx");
> >                                 if (output_fd == NULL) {
> > -                                       fprintf(stderr, "%s: Unable to open %s\n", argv[0], output_path);
> > +                                       if (errno == EEXIST)
> > +                                               fprintf(stderr, "%s: %s is already extracted with extension %s.\n", argv[0], mode_arg, lang_ext);
> > +                                       else
> > +                                               fprintf(stderr, "%s: Unable to open %s:  %s\n", argv[0], output_path, strerror(errno));
> >                                         result = -1;
> >                                         goto cleanup_extract;
> >                                 }
> > --
> > 2.36.1
> >
>
diff mbox series

Patch

diff --git a/policycoreutils/semodule/semodule.c b/policycoreutils/semodule/semodule.c
index 1ed8e690..48bc28dd 100644
--- a/policycoreutils/semodule/semodule.c
+++ b/policycoreutils/semodule/semodule.c
@@ -550,15 +550,12 @@  int main(int argc, char *argv[])
 					goto cleanup_extract;
 				}
 
-				if (access(output_path, F_OK) == 0) {
-					fprintf(stderr, "%s: %s is already extracted with extension %s.\n", argv[0], mode_arg, lang_ext);
-					result = -1;
-					goto cleanup_extract;
-				}
-
-				output_fd = fopen(output_path, "w");
+				output_fd = fopen(output_path, "wx");
 				if (output_fd == NULL) {
-					fprintf(stderr, "%s: Unable to open %s\n", argv[0], output_path);
+					if (errno == EEXIST)
+						fprintf(stderr, "%s: %s is already extracted with extension %s.\n", argv[0], mode_arg, lang_ext);
+					else
+						fprintf(stderr, "%s: Unable to open %s:  %s\n", argv[0], output_path, strerror(errno));
 					result = -1;
 					goto cleanup_extract;
 				}