BUG? in mkswap (Re: The current status of sebusybox project)

[KaiGai Kohei]

Karel,

Can I consider that you are the most appropriate person to report
about the following matter?

The changelog in util-linux-ng.spec says:
| * Wed Mar  8 2006 Karel Zak  2.13-0.17
| - fix #181782 - mkswap selinux relabeling (fix util-linux-2.13-mkswap-selinux.patch)
> /sbin/mkswap (should be ported later.)
> It enables to relabel the target file as "swapfile_t", when we use
> a regular file as a swap.

In util-linux-ng-2.13-1.fc8.src.rpm, this feature is implemented
as follows:

at util-linux-ng-2.13/disk-utils/mkswap.c
-------------------------------------------------------
    75  #define SELINUX_SWAPFILE_TYPE   "swapfile_t"
    :                :
   735  #ifdef HAVE_LIBSELINUX
   736          if (S_ISREG(statbuf.st_mode) && is_selinux_enabled()) {
   737                  security_context_t context_string;
   738                  security_context_t oldcontext;
   739                  context_t newcontext;
   740
   741                  if ((fgetfilecon(DEV, &oldcontext) < 0) &&
   742                      (errno != ENODATA)) {
   743                          fprintf(stderr, _("%s: %s: unable to obtain selinux file label: %s\n"),
   744                                          program_name, device_name,
   745                                          strerror(errno));
   746                          exit(1);
   747                  }
   748                  if (!(newcontext = context_new(oldcontext)))
   749                          die(_("unable to create new selinux context"));
   750                  if (context_type_set(newcontext, SELINUX_SWAPFILE_TYPE))
   751                          die(_("couldn't compute selinux context"));
   752
   753                  context_string = context_str(newcontext);
   754
   755                  if (strcmp(context_string, oldcontext)!=0) {
   756                          if (fsetfilecon(DEV, context_string)) {
   757                                  fprintf(stderr, _("%s: unable to relabel %s to %s: %s\n"),
   758                                                  program_name, device_name,
   759                                                  context_string,
   760                                                  strerror(errno));
   761                                  exit(1);
   762                          }
   763                  }
   764                  context_free(newcontext);
   765                  freecon(oldcontext);
   766          }
   767  #endif
-------------------------------------------------------

Pay attention around line 741.
If fgetfilecon() fails and returns -ENODATA, context_new() will be
called with uninitialized oldcontext in the next. Then, it cause
a segmentation fault.

If you don't want to exit immediately, I think this logic can be
changed as follows:
-------------------------------------------------------
    if (fgetfilecon(DEV, &oldcontext) < 0) {
        if (errno != ENODATA) {
            fprintf(stderr, _("%s: %s: unable to obtain selinux file label: %s\n"),
                            program_name, device_name,
                            strerror(errno));
            exit(1);
        }
        if (matchpathcon(devide_name, statbuf.st_mode, &oldcontext))
            die(_("unable to create new selinux context"));
    }
    if (!(newcontext = context_new(oldcontext)))
        die(_("unable to create new selinux context"));
-------------------------------------------------------

Thanks,

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

KaiGai Kohei wrote:
> Karel,
>
> Can I consider that you are the most appropriate person to report
> about the following matter?
>
> The changelog in util-linux-ng.spec says:
> | * Wed Mar  8 2006 Karel Zak  2.13-0.17
> | - fix #181782 - mkswap selinux relabeling (fix util-linux-2.13-mkswap-selinux.patch)
> > /sbin/mkswap (should be ported later.)
> > It enables to relabel the target file as "swapfile_t", when we use
> > a regular file as a swap.
>
> In util-linux-ng-2.13-1.fc8.src.rpm, this feature is implemented
> as follows:
> at util-linux-ng-2.13/disk-utils/mkswap.c
> 75  #define SELINUX_SWAPFILE_TYPE   "swapfile_t"
> :                :
>
> 735  #ifdef HAVE_LIBSELINUX
>    736          if (S_ISREG(statbuf.st_mode) && is_selinux_enabled()) {
>    737                  security_context_t context_string;
>    738                  security_context_t oldcontext;
>    739                  context_t newcontext;
>    740
>    741                  if ((fgetfilecon(DEV, &oldcontext) < 0) &&
>    742                      (errno != ENODATA)) {
>    743                          fprintf(stderr, _("%s: %s: unable to obtain selinux file label: %s\n"),
>    744                                          program_name, device_name,
>    745                                          strerror(errno));
>    746                          exit(1);
>    747                  }
>    748                  if (!(newcontext = context_new(oldcontext)))
>    749                          die(_("unable to create new selinux context"));
>    750                  if (context_type_set(newcontext, SELINUX_SWAPFILE_TYPE))
>    751                          die(_("couldn't compute selinux context"));
>    752
>    753                  context_string = context_str(newcontext);
>    754
>    755                  if (strcmp(context_string, oldcontext)!=0) {
>    756                          if (fsetfilecon(DEV, context_string)) {
>    757                                  fprintf(stderr, _("%s: unable to relabel %s to %s: %s\n"),
>    758                                                  program_name, device_name,
>    759                                                  context_string,
>    760                                                  strerror(errno));
>    761                                  exit(1);
>    762                          }
>    763                  }
>    764                  context_free(newcontext);
>    765                  freecon(oldcontext);
>    766          }
>    767  #endif
>
> Pay attention around line 741.
> If fgetfilecon() fails and returns -ENODATA, context_new() will be
> called with uninitialized oldcontext in the next. Then, it cause
> a segmentation fault.
>
> If you don't want to exit immediately, I think this logic can be
> changed as follows:
> if (fgetfilecon(DEV, &oldcontext) < 0) {
>     if (errno != ENODATA) {
>         fprintf(stderr, _("%s: %s: unable to obtain selinux file label: %s\n"),
>                         program_name, device_name,
>                         strerror(errno));
>         exit(1);
>     }
>     if (matchpathcon(devide_name, statbuf.st_mode, &oldcontext))
>         die(_("unable to create new selinux context"));
> }
> if (!(newcontext = context_new(oldcontext)))
>     die(_("unable to create new selinux context"));
>
> Thanks,

Please open a bugzilla.  Too easy to lease in email.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.7 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org
>
> iD8DBQFHD3lJrlYvE4MpobMRAuEsAJ47goFYNYHeuFgqES9Tjns+5/J29gCdE5j+
> hUHoowS2WpBJsNU09/4ZaDA=
> =kC64
> -----END PGP SIGNATURE-----

[busybox:01238] Re: BUG? in mkswap (Re: The current status of sebusybox project)

[KaiGai Kohei]

- snip -
> > If you don't want to exit immediately, I think this logic can be
> > changed as follows:
> > if (fgetfilecon(DEV, &oldcontext) < 0) {
> >     if (errno != ENODATA) {
> >         fprintf(stderr, _("%s: %s: unable to obtain selinux file label: %s\n"),
> >                         program_name, device_name,
> >                         strerror(errno));
> >         exit(1);
> >     }
> >     if (matchpathcon(devide_name, statbuf.st_mode, &oldcontext))
> >         die(_("unable to create new selinux context"));
> > }
> > if (!(newcontext = context_new(oldcontext)))
> >     die(_("unable to create new selinux context"));
> >
> > Thanks,
>
> Please open a bugzilla.  Too easy to lease in email.

OK, I opened a report at bugzilla.
https://bugzilla.redhat.com/show_bug.cgi?id=329641