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

Sven Eckelmann
2014-11-25T18:06:07Z

The fragmentation code 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 different
total_size. This can cause a crash when these are assembled because the
total_size information is only parsed from the first packet in
batadv_frag_merge_packets but the queueing function always uses the total_size
of the latest packet.

Assume two packets with the size x and y.

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

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

The fragmentation assembling code only took the information from the first
packet with the total_size x+y' and create a packet 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>
---
This is only build tested. I've never spend time in creation of these packets
to verify my claim.

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