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

[Sven Eckelmann <sven@narfation.org>]

[-- 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 --]