Re: [PATCH] opreport: buffer overflow fixes in xml generator

[Maynard J. <may...@us...>]

Robert Richter wrote:
> Signed-off-by: Robert Richter <rob...@am...>
Robert,
I sincerely apologize for not replying to this patch before now.  The posting got buried in my inbox, and I didn't see it until today (while doing some cleanup during holiday quiet time).  Does this patch fix some vulnerability?  I'm not seeing where a user might actually exploit the existing code to cause a buffer overflow.  Can you point this out?

Thanks.
-Maynard
> ---
>  ChangeLog                |    8 ++
>  libop/op_xml_events.c    |   65 ++++++++-------
>  libop/op_xml_out.c       |  197 +++++++++++++++++++++++++++-------------------
>  libop/op_xml_out.h       |   10 +-
>  libutil++/xml_output.cpp |   18 ++---
>  5 files changed, 170 insertions(+), 128 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index e7a858c..a13b131 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> +2008-11-21 Robert Richter <rob...@am...>
> +
> +	* libop/op_xml_events.c
> +	* libop/op_xml_out.c
> +	* libop/op_xml_out.h
> +	* libutil++/xml_output.cpputils/opcontrol: buffer overflow fixes
> +	  in xml generator
> +
>  2008-11-17 Robert Richter <rob...@am...>
> 
>  	* utils/opcontrol: restore parameter dump of --start and
> diff --git a/libop/op_xml_events.c b/libop/op_xml_events.c
> index 5b9ac7d..63f21d2 100644
> --- a/libop/op_xml_events.c
> +++ b/libop/op_xml_events.c
> @@ -17,37 +17,37 @@
> 
>  static op_cpu cpu_type;
>  #define MAX_BUFFER 4096
> +static char buffer[MAX_BUFFER];
> +
>  void open_xml_events(char const * title, char const * doc, op_cpu the_cpu_type)
>  {
>  	char const * schema_version = "1.0";
> -	char buffer[MAX_BUFFER];
> 
>  	buffer[0] = '\0';
>  	cpu_type = the_cpu_type;
> -	open_xml_element(HELP_EVENTS, 0, buffer);
> -	open_xml_element(HELP_HEADER, 1, buffer);
> -	init_xml_str_attr(HELP_TITLE, title, buffer);
> -	init_xml_str_attr(SCHEMA_VERSION, schema_version, buffer);
> -	init_xml_str_attr(HELP_DOC, doc, buffer);
> -	close_xml_element(NONE, 0, buffer);
> +	open_xml_element(HELP_EVENTS, 0, buffer, MAX_BUFFER);
> +	open_xml_element(HELP_HEADER, 1, buffer, MAX_BUFFER);
> +	init_xml_str_attr(HELP_TITLE, title, buffer, MAX_BUFFER);
> +	init_xml_str_attr(SCHEMA_VERSION, schema_version, buffer, MAX_BUFFER);
> +	init_xml_str_attr(HELP_DOC, doc, buffer, MAX_BUFFER);
> +	close_xml_element(NONE, 0, buffer, MAX_BUFFER);
>  	printf("%s", buffer);
>  }
> 
>  void close_xml_events(void)
>  {
> -	char buffer[MAX_BUFFER];
> -
>  	buffer[0] = '\0';
> -	close_xml_element(HELP_EVENTS, 0, buffer);
> +	close_xml_element(HELP_EVENTS, 0, buffer, MAX_BUFFER);
>  	printf("%s", buffer);
>  }
> 
> -static void xml_do_arch_specific_event_help(struct op_event const * event,
> -					    char * buffer)
> +static void xml_do_arch_specific_event_help(struct op_event const *event,
> +					    char *buffer, size_t size)
>  {
>  	switch (cpu_type) {
>  	case CPU_PPC64_CELL:
> -		init_xml_int_attr(HELP_EVENT_GROUP, event->val / 100, buffer);
> +		init_xml_int_attr(HELP_EVENT_GROUP, event->val / 100, buffer,
> +				  size);
>  		break;
>  	default:
>  		break;
> @@ -60,34 +60,39 @@ void xml_help_for_event(struct op_event const * event)
>  	uint i;
>  	int nr_counters;
>  	int has_nested = strcmp(event->unit->name, "zero");
> -	char buffer[MAX_BUFFER];
> 
>  	buffer[0] = '\0';
> -	open_xml_element(HELP_EVENT, 1, buffer);
> -	init_xml_str_attr(HELP_EVENT_NAME, event->name, buffer);
> -	xml_do_arch_specific_event_help(event, buffer);
> -	init_xml_str_attr(HELP_EVENT_DESC, event->desc, buffer);
> +	open_xml_element(HELP_EVENT, 1, buffer, MAX_BUFFER);
> +	init_xml_str_attr(HELP_EVENT_NAME, event->name, buffer, MAX_BUFFER);
> +	xml_do_arch_specific_event_help(event, buffer, MAX_BUFFER);
> +	init_xml_str_attr(HELP_EVENT_DESC, event->desc, buffer, MAX_BUFFER);
> 
>  	nr_counters = op_get_nr_counters(cpu_type);
> -	init_xml_int_attr(HELP_COUNTER_MASK, event->counter_mask, buffer);
> -	init_xml_int_attr(HELP_MIN_COUNT, event->min_count, buffer);
> +	init_xml_int_attr(HELP_COUNTER_MASK, event->counter_mask, buffer,
> +			  MAX_BUFFER);
> +	init_xml_int_attr(HELP_MIN_COUNT, event->min_count,
> +			  buffer, MAX_BUFFER);
> 
>  	if (has_nested) {
> -		close_xml_element(NONE, 1, buffer);
> -		open_xml_element(HELP_UNIT_MASKS, 1, buffer);
> -		init_xml_int_attr(HELP_DEFAULT_MASK, event->unit->default_mask, buffer);
> -		close_xml_element(NONE, 1, buffer);
> +		close_xml_element(NONE, 1, buffer, MAX_BUFFER);
> +		open_xml_element(HELP_UNIT_MASKS, 1, buffer, MAX_BUFFER);
> +		init_xml_int_attr(HELP_DEFAULT_MASK, event->unit->default_mask,
> +				  buffer, MAX_BUFFER);
> +		close_xml_element(NONE, 1, buffer, MAX_BUFFER);
>  		for (i = 0; i < event->unit->num; i++) {
> -			open_xml_element(HELP_UNIT_MASK, 1, buffer);
> +			open_xml_element(HELP_UNIT_MASK, 1, buffer, MAX_BUFFER);
>  			init_xml_int_attr(HELP_UNIT_MASK_VALUE,
> -					  event->unit->um[i].value, buffer);
> +					  event->unit->um[i].value,
> +					  buffer, MAX_BUFFER);
>  			init_xml_str_attr(HELP_UNIT_MASK_DESC,
> -					  event->unit->um[i].desc, buffer);
> -			close_xml_element(NONE, 0, buffer);
> +					  event->unit->um[i].desc,
> +					  buffer, MAX_BUFFER);
> +			close_xml_element(NONE, 0, buffer, MAX_BUFFER);
>  		}
> -		close_xml_element(HELP_UNIT_MASKS, 0, buffer);
> +		close_xml_element(HELP_UNIT_MASKS, 0, buffer, MAX_BUFFER);
>  	}
> -	close_xml_element(has_nested ? HELP_EVENT : NONE, has_nested, buffer);
> +	close_xml_element(has_nested ? HELP_EVENT : NONE, has_nested,
> +			  buffer, MAX_BUFFER);
>  	printf("%s", buffer);
>  }
> 
> diff --git a/libop/op_xml_out.c b/libop/op_xml_out.c
> index d779c45..7cf0abb 100644
> --- a/libop/op_xml_out.c
> +++ b/libop/op_xml_out.c
> @@ -91,143 +91,176 @@ char const * xml_tag_name(tag_t tag)
>  }
> 
> 
> -void open_xml_element(tag_t tag, int with_attrs, char * buffer)
> +void open_xml_element(tag_t tag, int with_attrs, char *buffer, size_t max)
>  {
> -	char const * tag_name = xml_tag_name(tag);
> -	unsigned int const max_len = strlen(tag_name) + 3;
> -	char tmp_buf[MAX_BUF_LEN];
> +	char *buf;
> +	int size, ret;
> 
> -	if (max_len >= sizeof(tmp_buf))
> -		fprintf(stderr,"Warning: open_xml_element: buffer overflow %d\n", max_len);
> +	buffer[max - 1] = '\0';
> +	size = strlen(buffer);
> +	buf = &buffer[size];
> +	size = max - 1 - size;
> 
> -	if (snprintf(tmp_buf, sizeof(tmp_buf), "<%s%s", tag_name,
> -		(with_attrs ? " " : ">\n")) < 0) {
> +	ret = snprintf(buf, size, "<%s%s", xml_tag_name(tag),
> +		       (with_attrs ? " " : ">\n"));
> +
> +	if (ret < 0 || ret >= size) {
>  		fprintf(stderr,"open_xml_element: snprintf failed\n");
>  		exit(EXIT_FAILURE);
>  	}
> -	strncat(buffer, tmp_buf, sizeof(tmp_buf));
>  }
> 
> 
> -void close_xml_element(tag_t tag, int has_nested, char * buffer)
> +void close_xml_element(tag_t tag, int has_nested, char *buffer, size_t max)
>  {
> -	char const * tag_name = xml_tag_name(tag);
> -	unsigned int const max_len = strlen(tag_name) + 3;
> -	char tmp_buf[MAX_BUF_LEN];
> +	char *buf;
> +	int size, ret;
> 
> -	if (max_len >= sizeof(tmp_buf))
> -		fprintf(stderr,"Warning: close_xml_element: buffer overflow %d\n", max_len);
> +	buffer[max - 1] = '\0';
> +	size = strlen(buffer);
> +	buf = &buffer[size];
> +	size = max - 1 - size;
> 
> -	if (tag == NONE) {
> -		if (snprintf(tmp_buf, sizeof(tmp_buf), "%s\n", (has_nested ? ">" : "/>")) < 0) {
> -			fprintf(stderr, "close_xml_element: snprintf failed\n");
> -			exit(EXIT_FAILURE);
> -		}
> -	} else {
> -		if (snprintf(tmp_buf, sizeof(tmp_buf), "</%s>\n", tag_name) < 0) {
> -			fprintf(stderr, "close_xml_element: snprintf failed\n");
> -			exit(EXIT_FAILURE);
> -		}
> +	if (tag == NONE)
> +		ret = snprintf(buf, size, "%s\n", (has_nested ? ">" : "/>"));
> +	else
> +		ret = snprintf(buf, size, "</%s>\n", xml_tag_name(tag));
> +
> +	if (ret < 0 || ret >= size) {
> +		fprintf(stderr, "close_xml_element: snprintf failed\n");
> +		exit(EXIT_FAILURE);
>  	}
> -	strncat(buffer, tmp_buf, sizeof(tmp_buf));
>  }
> 
> 
> -void init_xml_int_attr(tag_t attr, int value, char * buffer)
> +void init_xml_int_attr(tag_t attr, int value, char *buffer, size_t max)
>  {
> -	char const * attr_name = xml_tag_name(attr);
> -	char tmp_buf[MAX_BUF_LEN];
> -	unsigned int const max_len = strlen(attr_name) + 50;
> +	char *buf;
> +	int size, ret;
> 
> -	if (max_len >= sizeof(tmp_buf)) {
> -		fprintf(stderr,
> -			"Warning: init_xml_int_attr: buffer overflow %d\n", max_len);
> -	}
> +	buffer[max - 1] = '\0';
> +	size = strlen(buffer);
> +	buf = &buffer[size];
> +	size = max - 1 - size;
> 
> +	ret = snprintf(buf, size, " %s=\"%d\"", xml_tag_name(attr), value);
> 
> -	if (snprintf(tmp_buf, sizeof(tmp_buf), " %s=\"%d\"", attr_name, value) < 0) {
> +	if (ret < 0 || ret >= size) {
>  		fprintf(stderr,"init_xml_int_attr: snprintf failed\n");
>  		exit(EXIT_FAILURE);
>  	}
> -	strncat(buffer, tmp_buf, sizeof(tmp_buf));
>  }
> 
> 
> -void init_xml_dbl_attr(tag_t attr, double value, char * buffer)
> +void init_xml_dbl_attr(tag_t attr, double value, char *buffer, size_t max)
>  {
> -	char const * attr_name = xml_tag_name(attr);
> -	unsigned int const max_len = strlen(attr_name) + 50;
> -	char tmp_buf[MAX_BUF_LEN];
> +	char *buf;
> +	int size, ret;
> +
> +	buffer[max - 1] = '\0';
> +	size = strlen(buffer);
> +	buf = &buffer[size];
> +	size = max - 1 - size;
> 
> -	if (max_len >= sizeof(tmp_buf))
> -		fprintf(stderr, "Warning: init_xml_dbl_attr: buffer overflow %d\n", max_len);
> +	ret = snprintf(buf, size, " %s=\"%.2f\"", xml_tag_name(attr), value);
> 
> -	if (snprintf(tmp_buf, sizeof(tmp_buf), " %s=\"%.2f\"", attr_name, value) < 0) {
> +	if (ret < 0 || ret >= size) {
>  		fprintf(stderr, "init_xml_dbl_attr: snprintf failed\n");
>  		exit(EXIT_FAILURE);
>  	}
> -	strncat(buffer, tmp_buf, sizeof(tmp_buf));
>  }
> 
> 
> -static char * xml_quote(char const * str, char * quote_buf)
> +static void xml_quote(char const *str, char *buffer, size_t max)
>  {
> -	int i;
> -	int pos = 0;
> -	int len = strlen(str);
> +	char *buf;
> +	char *quote;
> +	char *pos = (char*)str;
> +	size_t size;
> +	int ret;
> 
> -	
> -	quote_buf[pos++] = '"';
> +	buffer[max - 1] = '\0';
> +	size = strlen(buffer);
> +	buf = &buffer[size];
> +	size = max - 1 - size;
> 
> -	for (i = 0; i < len; i++) {
> -		if (pos >= MAX_BUF_LEN - 10) {
> -			fprintf(stderr,"quote_str: buffer overflow %d\n", pos);
> -			exit(EXIT_FAILURE);
> -		}
> +	if (size < strlen(pos) + 2)
> +		goto Error;
> 
> -		switch(str[i]) {
> +	*buf = '"';
> +	buf++;
> +	size--;
> +
> +	while (*pos) {
> +		switch(*pos) {
>  		case '&':
> -			strncpy(quote_buf + pos, "&amp;", 5);
> -			pos += 5;
> +			quote = "&amp;";
>  			break;
>  		case '<':
> -			strncpy(quote_buf + pos, "&lt;", 4);
> -			pos += 4;
> +			quote = "&lt;";
>  			break;
>  		case '>':
> -			strncpy(quote_buf + pos, "&gt;", 4);
> -			pos += 4;
> +                        quote = "&gt;";
>  			break;
>  		case '"':
> -			strncpy(quote_buf + pos, "&quot;", 6);
> -			pos += 6;
> +			quote = "&quot;";
>  			break;
>  		default:
> -			quote_buf[pos++] = str[i];
> -			break;
> +			*buf = *pos;
> +			pos++;
> +			buf++;
> +			size--;
> +			continue;
>  		}
> +
> +		pos++;
> +		ret = snprintf(buf, size, "%s", quote);
> +		if (ret < 0 || ret >= (int)size)
> +			goto Error;
> +		buf += ret;
> +		size -= ret;
> +		if (size < strlen(pos))
> +			goto Error;
>  	}
> 
> -	quote_buf[pos++] = '"';
> -	quote_buf[pos++] = '\0';
> -	return quote_buf;
> +	if (!size)
> +		goto Error;
> +
> +	*buf = '"';
> +	buf++;
> +	*buf = '\0';
> +
> +	return;
> +
> +Error:
> +	fprintf(stderr,"quote_str: buffer overflow\n");
> +	exit(EXIT_FAILURE);
>  }
> 
> 
> -void init_xml_str_attr(tag_t attr, char const * str, char * buffer)
> +void init_xml_str_attr(tag_t attr, char const *str, char *buffer, size_t max)
>  {
> -	char tmp_buf[MAX_BUF_LEN];
> -	char quote_buf[MAX_BUF_LEN];
> -	char const * attr_name = xml_tag_name(attr);
> -	char const * quote_str = xml_quote(str, quote_buf);
> -	const unsigned int max_len = strlen(attr_name) + strlen(quote_str) + 10;
> +	char *buf;
> +	int size, ret;
> 
> -	if (max_len >= sizeof(tmp_buf))
> -		fprintf(stderr, "Warning: init_xml_str_attr: buffer overflow %d\n", max_len);
> +	buffer[max - 1] = '\0';
> +	size = strlen(buffer);
> +	buf = &buffer[size];
> +	size = max - 1 - size;
> 
> -	if (snprintf(tmp_buf, sizeof(tmp_buf), " %s=""%s""", attr_name, quote_str) < 0) {
> -		fprintf(stderr,"init_xml_str_attr: snprintf failed\n");
> -		exit(EXIT_FAILURE);
> -	}
> -	strncat(buffer, tmp_buf, sizeof(tmp_buf));
> +	ret = snprintf(buf, size, " %s=", xml_tag_name(attr));
> +	if (ret < 0 || ret >= size)
> +		goto Error;
> +
> +	buf += ret;
> +	size -= ret;
> +
> +	if (!size)
> +		goto Error;
> +
> +	xml_quote(str, buf, size);
> +	return;
> +Error:
> +	fprintf(stderr,"init_xml_str_attr: snprintf failed\n");
> +	exit(EXIT_FAILURE);
>  }
> diff --git a/libop/op_xml_out.h b/libop/op_xml_out.h
> index 52e8d8f..ae0fea5 100644
> --- a/libop/op_xml_out.h
> +++ b/libop/op_xml_out.h
> @@ -59,11 +59,11 @@ typedef enum {
>  	} tag_t;
> 
>  char const * xml_tag_name(tag_t tag);
> -void open_xml_element(tag_t tag, int with_attrs, char * result);
> -void close_xml_element(tag_t tag, int has_nested, char * result);
> -void init_xml_int_attr(tag_t attr, int value, char * result);
> -void init_xml_dbl_attr(tag_t attr, double value, char * result);
> -void init_xml_str_attr(tag_t attr, char const * str, char * result);
> +void open_xml_element(tag_t tag, int with_attrs, char *buffer, size_t size);
> +void close_xml_element(tag_t tag, int has_nested, char *buffer, size_t size);
> +void init_xml_int_attr(tag_t attr, int value, char *buffer, size_t size);
> +void init_xml_dbl_attr(tag_t attr, double value, char *buffer, size_t size);
> +void init_xml_str_attr(tag_t attr, char const *str, char *buffer, size_t size);
> 
>  #ifdef __cplusplus
>  }
> diff --git a/libutil++/xml_output.cpp b/libutil++/xml_output.cpp
> index 0f0b189..4ac9a1b 100644
> --- a/libutil++/xml_output.cpp
> +++ b/libutil++/xml_output.cpp
> @@ -16,7 +16,8 @@
> 
>  using namespace std;
> 
> -#define MAX_XML_BUF 1024
> +#define MAX_XML_BUF 4096
> +static char buf[MAX_XML_BUF];
> 
>  string tag_name(tag_t tag)
>  {
> @@ -29,10 +30,9 @@ string tag_name(tag_t tag)
>  string open_element(tag_t tag, bool with_attrs)
>  {
>  	ostringstream out;
> -	char buf[MAX_XML_BUF];
> 
>  	buf[0] = '\0';
> -	open_xml_element(tag, with_attrs, buf);
> +	open_xml_element(tag, with_attrs, buf, MAX_XML_BUF);
>  	out << buf;
>  	return out.str();
>  }
> @@ -41,10 +41,9 @@ string open_element(tag_t tag, bool with_attrs)
>  string close_element(tag_t tag, bool has_nested)
>  {
>  	ostringstream out;
> -	char buf[MAX_XML_BUF];
> 
>  	buf[0] = '\0';
> -	close_xml_element(tag, has_nested, buf);
> +	close_xml_element(tag, has_nested, buf, MAX_XML_BUF);
>  	out << buf;
>  	return out.str();
>  }
> @@ -53,10 +52,9 @@ string close_element(tag_t tag, bool has_nested)
>  string init_attr(tag_t attr, size_t value)
>  {
>  	ostringstream out;
> -	char buf[MAX_XML_BUF];
> 
>  	buf[0] = '\0';
> -	init_xml_int_attr(attr, value, buf);
> +	init_xml_int_attr(attr, value, buf, MAX_XML_BUF);
>  	out << buf;
>  	return out.str();
>  }
> @@ -65,10 +63,9 @@ string init_attr(tag_t attr, size_t value)
>  string init_attr(tag_t attr, double value)
>  {
>  	ostringstream out;
> -	char buf[MAX_XML_BUF];
> 
>  	buf[0] = '\0';
> -	init_xml_dbl_attr(attr, value, buf);
> +	init_xml_dbl_attr(attr, value, buf, MAX_XML_BUF);
>  	out << buf;
>  	return out.str();
>  }
> @@ -77,10 +74,9 @@ string init_attr(tag_t attr, double value)
>  string init_attr(tag_t attr, string const & str)
>  {
>  	ostringstream out;
> -	char buf[MAX_XML_BUF];
> 
>  	buf[0] = '\0';
> -	init_xml_str_attr(attr, str.c_str(), buf);
> +	init_xml_str_attr(attr, str.c_str(), buf, MAX_XML_BUF);
>  	out << buf;
>  	return out.str();
>  }