On 2010-03-15, at 12:59, Denys Vlasenko wrote:
On Sat, Mar 13, 2010 at 1:20 AM, J. Tang <[email protected]> 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 / [email protected]
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
