[PATCH] write SELinux contexts when extracting a tar file

[Denys Vlasenko]

On Sat, Mar 13, 2010 at 1:20 AM, J. Tang <tang at jtang.org> wrote:
> While doing some experiments with the current version of busybox out of the
> git repo, I discovered that tar does ignores SELinux file contexts while
> extracting a tarball.  The following patch:
>
> 1. Adds a config option, FEATURE_TAR_SELINUX, to enable this feature, and
> 2. Modifies parsing of tar headers to respect SELinux contexts stored in the
> tarball.
>
> Technically, there is no standard yet for recording SELinux contexts.  The
> most prevalent method is by a Red Hat vendor-specific tag,
> "RHT.security.selinux".  The proposed patch is flexible in that it can
> easily support future standards.
>
> Note that this patch only deals with tarball extraction.  It does not modify
> tarball creation.
>
> Signed-off-by: Jason Tang <tang at jtang.org>


> +#if ENABLE_FEATURE_TAR_SELINUX
> +/* Scan a PAX extended header for SELinux contexts, via
> + * "RHT.security.selinux" keyword.  This is the same vendor specific
> + * keyword used by Red Hat's patched version of tar.
> + */
> +#define SELINUX_CONTEXT_KEYWORD "RHT.security.selinux"
> +
> +static void parse_extended_header(archive_handle_t *archive_handle, off_t sz)
> +{
> +       char *buf, *p, *next_p, *keyword, *value;
> +       off_t len;
> +
> +       /* prevent a malloc of 0 */
> +       if (sz == 0) {
> +               bb_error_msg("Malformed extended header: length is 0");
> +               return;
> +       }

Why do you think such header is malformed?

> +
> +       /* forcibly add a newline at the end of the buffer, to prevent
> +          any buffer overflows */
> +       p = buf = xmalloc(sz);
> +       buf[sz - 1] = '\n';
> +
> +       xread(archive_handle->src_fd, buf, sz);

(1) You just overwrote your "\n".
(2) Look at docs/tar_pax.txt example. There, sz == 52
and by reading only 52 bytes here, you fall out of sync with
tar's 512-byte blocks.

> +       do {
> +               /* skip leading whitespace */
> +               while (*p == ' ' || *p == '\t') {
> +                       p++;
> +               }

Maybe use p = skip_whitespace(p) ?

> +               if (!isdigit(*p)) {
> +                       bb_error_msg("Malformed extended header: missing length");
> +                       return;
> +               }
> +
> +               /* scan for the length of the record */
> +
> +               errno = 0;
> +               len = strtoul(p, &keyword, 10);
> +               if ((errno == ERANGE) || ((p + len) > (buf + sz))) {
> +                       bb_error_msg("Malformed extended header: length is out of allowed range");
> +                       return;
> +               }

Use bb_strtou, it makes error checking much simpler.
You will be able to drop isdigit check too.

> +               while (*p != '=' && *p != '\n') {
> +                       p++;
> +               }
> +               if (*p != '=') {
> +                       bb_error_msg("Malformed extended header: missing equal sign");
> +                       return;
> +               }

Can you use strchrnul instead of this block?

> +               while (*p != '\n') {
> +                       p++;
> +               }
> +               *p = '\0';

strchr

> +               if (strcmp(keyword, SELINUX_CONTEXT_KEYWORD) == 0) {
> +                       /* free old context, in case context is given
> +                          multiple times */
> +                       if (setfscreatecon(value) < 0) {
> +                               bb_perror_msg("warning: cannot set label to %s", value);

Drop "warning:" and use "'%s'", not "%s".
Imagine how message would look if value == ""...

-- 
vda