Re: [conntrack-tools] XML output is invalid

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> Does this patch help?
>>>
>>>
>>> +    if (output_mask & _O_XML) {
>>>          op_type = NFCT_O_XML;
>>> +        if (dump_xml_header_done) {
>>> +            dump_xml_header_done = 0;
>>> +            len = snprintf(buf, 1024, "<?xml version=\"1.0\"?>\n"
>>> +                          "<conntrack>\n");
>>> +        }
>>> +    }
>>>      if (output_mask & _O_EXT)
>>>          op_flags = NFCT_OF_SHOW_LAYER3;
>>>      if (output_mask & _O_ID)
>>>          op_flags |= NFCT_OF_ID;
>>>  
>>> -    nfct_snprintf(buf, 1024, ct, NFCT_T_UNKNOWN, op_type, op_flags);
>>> +    nfct_snprintf(buf+len, 1024-len, ct, NFCT_T_UNKNOWN, op_type,
>>> op_flags);
>>
>> It doesn't seem to matter here, but that looks buggy (combined
>> with the snprintf above). When the buffer size is exceed, snprintf
>> returns the amount of characters it *would have written* if
>> enough space was available. So when this really happens above,
>> you have a buffer overflow in the second snprintf.
> 
> The string above has a fixed size and the buffer is big enough to print
> the flow entry, so the buffer overflow is very unlikely.
> 
> Anyhow, I think that the following patch perform more strict and robust
> checkings regarding the buffer size. I hope that you like better :).

Oops, I forgot the patch. I can also make another patch to check for
other snprintf return values.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3473 bytes --]

diff --git a/include/conntrack.h b/include/conntrack.h
index 2e17475..dc30c13 100644
--- a/include/conntrack.h
+++ b/include/conntrack.h
@@ -189,4 +189,10 @@ extern void register_udp(void);
 extern void register_icmp(void);
 extern void register_icmpv6(void);
 
+#define BUFFER_SIZE(ret, len, offset)			\
+	if (ret > len)					\
+		ret = len;				\
+	offset += ret;					\
+	len -= ret;
+
 #endif
diff --git a/src/conntrack.c b/src/conntrack.c
index 25a3a57..f018c82 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -604,10 +604,16 @@ static int ignore_nat(const struct nf_conntrack *obj,
 }
 
 static int counter;
+static int dump_xml_header_done = 1;
 
 static void __attribute__((noreturn))
 event_sighandler(int s)
 {
+	if (dump_xml_header_done == 0) {
+		printf("</conntrack>\n");
+		fflush(stdout);
+	}
+
 	fprintf(stderr, "%s v%s: ", PROGNAME, VERSION);
 	fprintf(stderr, "%d flow events has been shown.\n", counter);
 	nfct_close(cth);
@@ -619,6 +625,7 @@ static int event_cb(enum nf_conntrack_msg_type type,
 		    void *data)
 {
 	char buf[1024];
+	int ret, offset = 0, len = 1024;
 	struct nf_conntrack *obj = data;
 	unsigned int op_type = NFCT_O_DEFAULT;
 	unsigned int op_flags = 0;
@@ -629,8 +636,20 @@ static int event_cb(enum nf_conntrack_msg_type type,
 	if (options & CT_COMPARISON && !nfct_cmp(obj, ct, NFCT_CMP_ALL))
 		return NFCT_CB_CONTINUE;
 
-	if (output_mask & _O_XML)
+	if (output_mask & _O_XML) {
 		op_type = NFCT_O_XML;
+		if (dump_xml_header_done) {
+			dump_xml_header_done = 0;
+			ret = snprintf(buf, len, "<?xml version=\"1.0\"?>\n"
+						 "<conntrack>\n");
+			if (ret == -1) {
+				fprintf(stderr, "evil! snprintf fails\n");
+				return NFCT_CB_CONTINUE;
+			}
+
+			BUFFER_SIZE(ret, len, offset);
+		}
+	} 
 	if (output_mask & _O_EXT)
 		op_flags = NFCT_OF_SHOW_LAYER3;
 	if (output_mask & _O_TMS) {
@@ -644,7 +663,8 @@ static int event_cb(enum nf_conntrack_msg_type type,
 	if (output_mask & _O_ID)
 		op_flags |= NFCT_OF_ID;
 
-	nfct_snprintf(buf, 1024, ct, type, op_type, op_flags);
+	nfct_snprintf(buf+offset, len, ct, type, op_type, op_flags);
+
 	printf("%s\n", buf);
 	fflush(stdout);
 
@@ -658,6 +678,7 @@ static int dump_cb(enum nf_conntrack_msg_type type,
 		   void *data)
 {
 	char buf[1024];
+	int ret, offset = 0, len = 1024;
 	struct nf_conntrack *obj = data;
 	unsigned int op_type = NFCT_O_DEFAULT;
 	unsigned int op_flags = 0;
@@ -668,14 +689,26 @@ static int dump_cb(enum nf_conntrack_msg_type type,
 	if (options & CT_COMPARISON && !nfct_cmp(obj, ct, NFCT_CMP_ALL))
 		return NFCT_CB_CONTINUE;
 
-	if (output_mask & _O_XML)
+	if (output_mask & _O_XML) {
 		op_type = NFCT_O_XML;
+		if (dump_xml_header_done) {
+			dump_xml_header_done = 0;
+			ret = snprintf(buf, len, "<?xml version=\"1.0\"?>\n"
+						 "<conntrack>\n");
+			if (ret == -1) {
+				fprintf(stderr, "evil! snprintf fails\n");
+				return NFCT_CB_CONTINUE;
+			}
+
+			BUFFER_SIZE(ret, len, offset);
+		}
+	}
 	if (output_mask & _O_EXT)
 		op_flags = NFCT_OF_SHOW_LAYER3;
 	if (output_mask & _O_ID)
 		op_flags |= NFCT_OF_ID;
 
-	nfct_snprintf(buf, 1024, ct, NFCT_T_UNKNOWN, op_type, op_flags);
+	nfct_snprintf(buf+offset, len, ct, NFCT_T_UNKNOWN, op_type, op_flags);
 	printf("%s\n", buf);
 
 	counter++;
@@ -1129,6 +1162,11 @@ int main(int argc, char *argv[])
 		else
 			res = nfct_query(cth, NFCT_Q_DUMP, &family);
 
+		if (dump_xml_header_done == 0) {
+			printf("</conntrack>\n");
+			fflush(stdout);
+		}
+
 		nfct_close(cth);
 		break;