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.
- 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