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

Acked-by: Martin Hundebøll martin@hundeboll.net

Philipp: Can you please test this patch, and report back if it fixes your crash?

Thanks!

// Martin

On 2014-11-25 19:06, Sven Eckelmann wrote:

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; };

  /**