[B.A.T.M.A.N.] [PATCH-maint] batman-adv: Check total_size when reassembling fragments

[B.A.T.M.A.N.] [PATCH-maint] batman-adv: Check total_size when reassembling fragments

[Sven Eckelmann]

The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606
("batman-adv: Receive fragmented packets and merge") by an implementation which
handles the queueing+merging of fragments based on their size and the
total_size of the non-fragmented packet. This total_size is announced by each
fragment. The new implementation doesn't check if the the total_size
information of the packets inside one chain is consistent. This allows an
attacker to inject packets belonging to the same fragmentation sequence number
with varying total_size information. The missing validation can cause a crash
when the fragments are merged because the total_size information is only
retrieved from the first packet by batadv_frag_merge_packets. But the queueing
function batadv_frag_insert_packet always uses the total_size from the latest
packet to check if the fragmented packet was transferred completely and is now
ready to be merged.

Assume two packets with the size x and y.

1. first packet (fragno 1) is sent with a size x and the total_size x+y' (y' < y)
2. second packet (fragno 0) is sent with a size y and the total_size x+y

The fragmentation code would try to merge the two packets because the
accumulated packets have a combined size of x+y and the second packet was sent
with total_size of x+y.

The fragments merging code only took the information from the first packet with
the total_size x+y' and created a buffer with enough space for x+y' bytes. But
the second packet cannot be copied inside the prepared free space because it is
y-y' bytes larger than the remaining space.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
Acked-by: Martin Hundebøll <martin@hundeboll.net>
---
This is only a resend of the patch
 https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-November/012584.html

This was necessary because the mail thread was hijacked by others who started
to discuss a different problem which may or may not be caused by the
fragmentation code (or batman-adv at all). At least they removed me from the
Cc so I had not received their "responses".

This also gave me the opportunity to change some words in the commit message.
---
 fragmentation.c | 7 +++++--
 types.h         | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/fragmentation.c b/fragmentation.c
index 362e91a..3a19d4d 100644
--- a/fragmentation.c
+++ b/fragmentation.c
@@ -161,6 +161,7 @@ static bool batadv_frag_insert_packet(struct batadv_orig_node *orig_node,
 		hlist_add_head(&frag_entry_new->list, &chain->head);
 		chain->size = skb->len - hdr_size;
 		chain->timestamp = jiffies;
+		chain->total_size = ntohs(frag_packet->total_size);
 		ret = true;
 		goto out;
 	}
@@ -195,9 +196,11 @@ static bool batadv_frag_insert_packet(struct batadv_orig_node *orig_node,
 
 out:
 	if (chain->size > batadv_frag_size_limit() ||
-	    ntohs(frag_packet->total_size) > batadv_frag_size_limit()) {
+	    chain->total_size != ntohs(frag_packet->total_size) ||
+	    chain->total_size > batadv_frag_size_limit()) {
 		/* Clear chain if total size of either the list or the packet
-		 * exceeds the maximum size of one merged packet.
+		 * exceeds the maximum size of one merged packet. Don't allow
+		 * packets to have different total_size.
 		 */
 		batadv_frag_clear_chain(&chain->head);
 		chain->size = 0;
diff --git a/types.h b/types.h
index 462a70c..c4d7d24 100644
--- a/types.h
+++ b/types.h
@@ -132,6 +132,7 @@ struct batadv_orig_ifinfo {
  * @timestamp: time (jiffie) of last received fragment
  * @seqno: sequence number of the fragments in the list
  * @size: accumulated size of packets in list
+ * @total_size: expected size of the assembled packet
  */
 struct batadv_frag_table_entry {
 	struct hlist_head head;
@@ -139,6 +140,7 @@ struct batadv_frag_table_entry {
 	unsigned long timestamp;
 	uint16_t seqno;
 	uint16_t size;
+	uint16_t total_size;
 };
 
 /**
-- 
2.1.3

Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: Check total_size when reassembling fragments

[Sven Eckelmann]

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

On Sunday 30 November 2014 22:33:57 Sven Eckelmann wrote:
> The fragmentation code was replaced in
> 9b3eab61754d74a93c9840c296013fe3b4a1b606 ("batman-adv: Receive fragmented
> packets and merge") by an implementation which handles the queueing+merging
> of fragments based on their size and the total_size of the non-fragmented
> packet. This total_size is announced by each fragment. The new
> implementation doesn't check if the the total_size information of the
> packets inside one chain is consistent. This allows an attacker to inject
> packets belonging to the same fragmentation sequence number with varying
> total_size information. The missing validation can cause a crash when the
> fragments are merged because the total_size information is only retrieved
> from the first packet by batadv_frag_merge_packets. But the queueing
> function batadv_frag_insert_packet always uses the total_size from the
> latest packet to check if the fragmented packet was transferred completely
> and is now ready to be merged.
> 
> Assume two packets with the size x and y.
> 
> 1. first packet (fragno 1) is sent with a size x and the total_size x+y' (y'
> < y) 2. second packet (fragno 0) is sent with a size y and the total_size
> x+y
> 
> The fragmentation code would try to merge the two packets because the
> accumulated packets have a combined size of x+y and the second packet was
> sent with total_size of x+y.
> 
> The fragments merging code only took the information from the first packet
> with the total_size x+y' and created a buffer with enough space for x+y'
> bytes. But the second packet cannot be copied inside the prepared free
> space because it is y-y' bytes larger than the remaining space.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> Acked-by: Martin Hundebøll <martin@hundeboll.net>
> ---

I think this patch should not be applied to maint because parts of the 
assumptions in the commit message are contradicted by
 https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-December/012609.html

Maybe I will prepare a v2 which is for master and not for maint. It will not 
have the "omg, we will all die" scenario in the commit message which no ones 
seems to have checked (and ended up to be wrong). The main purpose of the 
upcoming patches would be to remove the skb parameter in the merge function 
and make sure that the sizes are checked before the queuing is done.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]