[PATCH v2]: write SELinux contexts when extracting a tar file

Denys Vlasenko vda.linux at googlemail.com
Tue Mar 16 15:52:18 UTC 2010


On Tue, Mar 16, 2010 at 3:58 PM, J. Tang <tang at jtang.org> wrote:
> Here is a second iteration of the patch to enhanch tar, incorporating
> suggestions be Denys.  It is baselined against the most recent commit
> to origin/master.


pax doc says:

> +       /* 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);

Look carefully. You stored '\n' in buf[sz - 1].
The very next command reads sz bytes, thus fills
buf[0...sz - 1], destroying '\n'.

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

Why do you skip it?
http://www.opengroup.org/onlinepubs/009695399/utilities/pax.html
"pax Extended Header" section, does not say there can be leading whitespace.
This is what I saw in linux kernel tarball:

00000200  35 32 20 63 6f 6d 6d 65  6e 74 3d 62 31 30 35 30  |52 comment=b1050|
00000210  32 62 32 32 61 31 32 30  39 64 36 62 34 37 36 33  |2b22a1209d6b4763|
00000220  39 64 38 38 62 38 31 32  62 32 31 66 62 35 39 34  |9d88b812b21fb594|
00000230  39 65 34 0a 00 00 00 00  00 00 00 00 00 00 00 00  |9e4.............|

No leading whitespace, as you see.
Do you have an example tar file where it exists?

> +               /* scan for the length of the record */
> +               len = bb_strtou(p, &keyword, 10);
> +               /* expect errno to be EINVAL, because the character
> +                  following the digits is supposed to be a space */
> +               if ((errno != EINVAL) || ((p + len) > (buf + sz))) {

    || len == 0 || *keyword != ' '

> +                       bb_error_msg("Malformed extended header: length is out of allowed range");
> +                       return;
> +               }
> +
> +               next_p = p + len;
> +
> +               p = keyword;
> +
> +               /* scan for the start of the keyword and the value;
> +                  they are '=' separated */
> +
> +               while (*p == ' ' || *p == '\t') {
> +                       p++;
> +               }
> +               keyword = p;
> +
> +               while (*p != '=' && *p != '\n') {
> +                       p++;
> +               }
> +               if (*p != '=') {
> +                       bb_error_msg("Malformed extended header: missing equal sign");
> +                       return;
> +               }
> +
> +               *p = '\0';
> +
> +               p++;
> +               value = p;
> +               while (*p != '\n') {
> +                       p++;
> +               }
> +               *p = '\0';
> +
> +               p = next_p;
> +
> +               /* handle fields that are SELinux related */
> +               if (strcmp(keyword, SELINUX_CONTEXT_KEYWORD) == 0) {
> +                       /* free old context, in case context is given
> +                          multiple times */
     ***  where is the "free old context" operation mentioned in the comment?
> +                       if (setfscreatecon(value) < 0) {
> +                               bb_perror_msg("cannot set label to '%s'", value);
> +                       }
> +               }

All of the above can be replaced by simpler code like this:

p += len;
p[-1] = '\0'; /* replace terminating '\n' */
if (strncmp(keyword+1, SELINUX_CONTEXT_KEYWORD"=",
sizeof(SELINUX_CONTEXT_KEYWORD"=")-1) == 0) {
        /* We found it */
        value = keyword + sizeof(SELINUX_CONTEXT_KEYWORD"=");
        if (setfscreatecon(value) < 0) {
                bb_perror_msg("can't set label to '%s'", value);
        }
}


> +       } while (p < buf + sz);


-- 
vda


More information about the busybox mailing list