[PATCH] write SELinux contexts when extracting a tar file

J. Tang tang at jtang.org
Tue Mar 16 02:12:57 UTC 2010


On 2010-03-15, at 12:59, Denys Vlasenko wrote:
> On Sat, Mar 13, 2010 at 1:20 AM, J. Tang <tang at jtang.org> wrote:
>> +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?

Excellent point; instead of a warning, the function should simply  
return.

>> +       /* 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".

Not necessarily.  A malformed or malicious tarball might not have a  
\n to terminate the record.

> (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.

There is no syncing problem, because when parse_extended_header()  
returns, there is a "goto again" to realign itself.

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

skip_whitespace() also skips newlines.

>> +               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.

Fixed.

>
>> +               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?

I do not believe that strchrnul() is appropriate here, because the  
buffer is not necessarily null terminated.  It is guaranteed to be  
newline terminated because of the explicit set after xmalloc().

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

Ibid.

>> +               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 == ""...

Fixed.

I will have a new patch later, once I am able to test the changes on  
the target platform.

-- 
Jason Tang  /  tang at jtang.org




More information about the busybox mailing list