[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