[PATCH] makedumpfile-1.5.4 dump dmesg may not work due to improper sprintf usage

[Atsushi Kumagai]

Hello Nick,

On Mon, 29 Jul 2013 09:27:57 -0700
Nick Bartos <nick at pistoncloud.com> wrote:

> I am reporting this bug as requested in the "BUG REPORT" of
> the makedumpfile README.
> 
> There are several times when sprintf is used to append to a buffer like so:
> sprintf(buf, "%s.", buf);
> 
> In the sprintf manpage, it describes that behavior is not consistent.  I
> first noticed this as my hardned system would make all dmesg output lines
> empty.  Here is the excerpt from the man page:
> 
>     C99 and POSIX.1-2001 specify that the results  are  undefined if a call
> to sprintf(), snprintf(),  vsprintf(),  or vsnprintf() would cause copying
> to take place between objects that overlap (e.g., if the target string
> array and one of the supplied input  arguments refer to the same buffer).
>  See NOTES.
> 
> NOTES
>        Some programs imprudently rely on code such as the following
> 
>            sprintf(buf, "%s some further text", buf);
> 
>        to  append text to buf.  However, the standards explicitly note that
> the results are undefined if source and  destination  buffers  overlap
>  when  calling  sprintf(),  snprintf(), vsprintf(),  and  vsnprintf().
> Depending on the version of gcc(1) used, and the compiler options employed,
> calls such as the above will not produce the expected results.
> 
>        The glibc implementation of the functions snprintf() and vsnprintf()
> conforms to  the  C99 standard, that is, behaves as described above, since
> glibc version 2.1.  Until glibc 2.0.6 they would return -1 when the output
> was truncated.
> 
> 
> In addition to using sprintf improperly, it appears that there may be
> buffer overflow potential in the current code, since the loop does not stop
> if the size of buf is exhausted.
> 
> I have attached a patch which modifies this behavior.

I have some comments on this patch, please see below.

> diff -U3 -r makedumpfile-1.5.4.orig/makedumpfile.c makedumpfile-1.5.4/makedumpfile.c
> --- makedumpfile-1.5.4.orig/makedumpfile.c	2013-07-01 08:08:49.000000000 +0000
> +++ makedumpfile-1.5.4/makedumpfile.c	2013-07-22 22:47:34.195721706 +0000
> @@ -3701,6 +3701,7 @@
>  	char *msg, *p;
>  	unsigned int i, text_len;
>  	unsigned long long ts_nsec;
> +	int buf_s;
>  	char buf[BUFSIZE];
>  	ulonglong nanos;
>  	ulong rem;
> @@ -3713,21 +3714,49 @@
>  
>  	msg = logptr + SIZE(log);
>  
> -	sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000);
> +	buf_s = snprintf(buf, BUFSIZE, "[%5lld.%06ld] ", nanos, rem/1000);
> +	if (buf_s < 0) {
> +		DEBUG_MSG("snprintf returned an error in dump_log_entry.\n");
> +		return FALSE;
> +	}

This is the error case, so you should use ERRMSG instead of DEBUG_MSG.
The same can be said about all other messages.

> +	if (buf_s >= BUFSIZE) {
> +		DEBUG_MSG("snprintf truncated output in dump_log_entry.\n");
> +		DEBUG_MSG("  increase BUFSIZE and recompile.\n");
> +		/* No point in continuing, we can't append anything useful */
> +		return FALSE;
> +	}
> +
> +	/* It's unlikely but possible we may not have enough to terminate the
> +	   string */
> +	if (buf_s >= BUFSIZE - 1) {
> +		DEBUG_MSG("not enough buffer space in dump_log_entry.\n");
> +		DEBUG_MSG("  increase BUFSIZE and recompile.\n");
> +		return FALSE;
> +	}
> +
> +	for (i = 0, p = msg; i < text_len; i++, p++, buf_s++) {
> +		if (buf_s >= BUFSIZE - 2) {
> +			DEBUG_MSG("output truncated in dump_log_entry.\n");
> +			DEBUG_MSG("  increase BUFSIZE and recompile.\n");
> +			break;
> +		}

I don't think to check the remaining buffer space 3 times is meaningful,
they indicate just "lack of buffer space".
Is the last one enough to check the remaining buffer space ?

>  
> -	for (i = 0, p = msg; i < text_len; i++, p++) {
>  		if (*p == '\n')
> -			sprintf(buf, "%s.", buf);
> +			buf[buf_s] = '.';
>  		else if (isprint(*p) || isspace(*p))
> -			sprintf(buf, "%s%c", buf, *p);
> +			buf[buf_s] = *p;
>  		else
> -			sprintf(buf, "%s.", buf);
> +			buf[buf_s] = '.';
>  	}
>  
> -	sprintf(buf, "%s\n", buf);
> +	buf[buf_s] = '\n';
> +	buf_s++;
> +	buf[buf_s] = NULL;

Null character(\0) should be used to terminate strings instead of
null pointer.

>  
> -	if (write(info->fd_dumpfile, buf, strlen(buf)) < 0)
> +	if (write(info->fd_dumpfile, buf, strlen(buf)) < 0) {
> +		DEBUG_MSG("Problem writing to dump file.\n");
>  		return FALSE;
> +        }
>  	else
>  		return TRUE;
>  }


Thanks
Atsushi Kumagai