slplink: fix buffer overflow vulnerability

The issue stems from the fact that the body of an SLP message can have a different length than what the header claims. You make the check

 if (msg->msnslp_header.total_size < msg->msnslp_header.length)

which uses declared length of the packet in the SLP header; however, the variable len does not originate from  msg->msnslp_header.length, but is rather computed in msn_message_parse_payload and msn_message_parse_slp_body. No checks are made to ensure that msg->body_len == msg->msnslp_header.length. Thus, the vulnerability in question revolves around the situation in which len == msg->body_len > msg->msnslp_header.length

As such, a malicious program can construct a SLP message sequence in the following manner:
Send a new incomplete SLP message in which msnslp_header.session_id=SESSION_ID, msnslp_header.id=MESSAGE_ID, msnslp_header.offset = 0, and msnslp_header.length < msnslp_header.total_size. The SLP message is incomplete, which allows the attacker to send a second packet with a non-zero offset. For the purposes of this example, assume msnslp_header.length = 5 and msnslp_header.total_size = 10.

Send the second message with same session and message ids msnslp_header.session_id=SESSION_ID, msnslp_header.id=MESSAGE_ID. However, send more data in the payload than the header specifies. For the purposes of the example, let us say msnslp_header.length = 5 and msnslp_header.total_size = 10 again, but this time the SLP message body is longer than the header specifies and contains a 1KiB of malicious data Thus msn_message_get_bin_data will set len = 1024. Furthermore, let us assume the attacker chooses offset = -2048 == 0xfffffffffffff800. The check performed is then evaluated as this:
if ((gsize)offset > ((long long)slpmsg->size - (gsize)len))

On a system where gsize is 32 bits, offset gets truncated to 32 bits and this evaluates to if((gsize)0xfffff800 > (long long)-1014). 0xfffff800 is then implicitly converted back to a positive 64-bit integer, because an unsigned 32-bit value fits within the range of a signed 64-bit value, and the check works because the unsigned offset correctly doesn't get sign-extended.

However, on a system where gsize and long long is 64 bits, this evaluates to if((gsize)0xfffffffffffff800 > (long long)-1014), the full untruncated offset specified in the packet. In this case, we have a 64-bit unsigned vs 64-bit signed comparison that GCC doesn't produce a warning for. Regardless of whether the compiler treats the result as signed or unsigned, the check is bypassed. 0xfffffffffffff800 > 0xfffffffffffffc0a and -2048 > -1014 are both false. Thus the memcpy line executes, writing the malicious data to buffer - 2048, well outside the 10 byte buffer allocated.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

cvr/slplink.c

@@ -287,7 +287,7 @@ static void
 msg_ack(MsnMessage *msg, void *data)
 {
 	MsnSlpMessage *slpmsg;
-	long long real_size;
+	guint64 real_size;
 
 	slpmsg = data;
 
@@ -333,7 +333,7 @@ void
 msn_slplink_send_msgpart(MsnSlpLink *slplink, MsnSlpMessage *slpmsg)
 {
 	MsnMessage *msg;
-	long long real_size;
+	guint64 real_size;
 	size_t len = 0;
 
 	/* Maybe we will want to create a new msg for this slpmsg instead of
@@ -538,7 +538,7 @@ msn_slplink_process_msg(MsnSlpLink *slplink, MsnMessage *msg)
 {
 	MsnSlpMessage *slpmsg;
 	const char *data;
-	gsize offset;
+	guint64 offset;
 	gsize len;
 
 #ifdef PECAN_DEBUG_SLP
@@ -625,7 +625,7 @@ msn_slplink_process_msg(MsnSlpLink *slplink, MsnMessage *msg)
 	}
 	else if (slpmsg->size)
 	{
-		if (offset > (slpmsg->size - len))
+		if (len > slpmsg->size || offset > (slpmsg->size - len))
 		{
 			pecan_error ("oversized slpmsg");
 			g_return_if_reached();

cvr/slpmsg.c

@@ -111,7 +111,7 @@ msn_slpmsg_destroy(MsnSlpMessage *slpmsg)
 void
 msn_slpmsg_set_body(MsnSlpMessage *slpmsg,
 					gconstpointer *body,
-					long long size)
+					guint64 size)
 {
 	/* We can only have one data source at a time. */
 	g_return_if_fail(slpmsg->buffer == NULL);

cvr/slpmsg.h

@@ -51,7 +51,7 @@ struct MsnSlpMessage
 	long id;
 	long ack_id;
 	long ack_sub_id;
-	long long ack_size;
+	guint64 ack_size;
 	long app_id;
 
 	gboolean sip; /**< A flag that states if this is a SIP slp message. */
@@ -60,8 +60,8 @@ struct MsnSlpMessage
 
 	FILE *fp;
 	gchar *buffer;
-	long long offset;
-	long long size;
+	guint64 offset;
+	guint64 size;
 
 	GList *msgs; /**< The real messages. */
 
@@ -92,7 +92,7 @@ void msn_slpmsg_destroy(MsnSlpMessage *slpmsg);
 
 void msn_slpmsg_set_body(MsnSlpMessage *slpmsg,
 						 gconstpointer *body,
-						 long long size);
+						 guint64 size);
 void msn_slpmsg_set_image (MsnSlpMessage *slpmsg, PecanBuffer *image);
 void msn_slpmsg_open_file(MsnSlpMessage *slpmsg,
 						  const char *file_name);