[B.A.T.M.A.N.] [PATCH] alfred: Tighten size check on received packet

[B.A.T.M.A.N.] [PATCH] alfred: Tighten size check on received packet

[Jan-Philipp Litza]

When first checking if a received packet is truncated, the size of the
alfred_tlv structure is ignored, thus allowing packets that are
truncated by 4 bytes or less to pass the check unnoticed.

Even the check itself might access memory after the packet if its size
was only 2 bytes or less.

Signed-off-by: Jan-Philipp Litza <janphilipp@litza.de>
---
 recv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/recv.c b/recv.c
index 90db0b3..870485f 100644
--- a/recv.c
+++ b/recv.c
@@ -402,7 +402,8 @@ int recv_alfred_packet(struct globals *globals, struct interface *interface)
 		return -1;
 
 	/* drop truncated packets */
-	if (length < ((int)ntohs(packet->length)))
+	if (length < (int)sizeof(*packet) ||
+	    length < (int)(ntohs(packet->length) + sizeof(*packet)))
 		return -1;
 
 	/* drop incompatible packet */
-- 
2.1.0

Re: [B.A.T.M.A.N.] [PATCH] alfred: Tighten size check on received packet

[Sven Eckelmann]

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

On Monday 19 January 2015 21:59:32 Jan-Philipp Litza wrote:
> When first checking if a received packet is truncated, the size of the
> alfred_tlv structure is ignored, thus allowing packets that are
> truncated by 4 bytes or less to pass the check unnoticed.
> 
> Even the check itself might access memory after the packet if its size
> was only 2 bytes or less.
[...]
>  	/* drop truncated packets */
> -	if (length < ((int)ntohs(packet->length)))
> +	if (length < (int)sizeof(*packet) ||
> +	    length < (int)(ntohs(packet->length) + sizeof(*packet)))
>  		return -1;
> 
>  	/* drop incompatible packet */

Thanks for the patch. It is basically correct but maybe you can modify it
slightly to make it also catch very small packets.

diff --git a/recv.c b/recv.c
index 90db0b3..288f577 100644
--- a/recv.c
+++ b/recv.c
@@ -391,7 +391,12 @@ int recv_alfred_packet(struct globals *globals, struct interface *interface)
 		return -1;
 	}
 
+	/* drop packets smaller than tlv */
+	if (length < (int)sizeof(*packet))
+		return -1;
+
 	packet = (struct alfred_tlv *)buf;
+	length -= sizeof(*packet);
 
 	/* drop packets not sent over link-local ipv6 */
 	if (!is_ipv6_eui64(&source.sin6_addr))

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] alfred: Tighten size check on received packet

[Jan-Philipp Litza]

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

> On Monday 19 January 2015 21:59:32 Jan-Philipp Litza wrote:
>> When first checking if a received packet is truncated, the size of the
>> alfred_tlv structure is ignored, thus allowing packets that are
>> truncated by 4 bytes or less to pass the check unnoticed.
>>
>> Even the check itself might access memory after the packet if its size
>> was only 2 bytes or less.
> [...]
>>  	/* drop truncated packets */
>> -	if (length < ((int)ntohs(packet->length)))
>> +	if (length < (int)sizeof(*packet) ||
>> +	    length < (int)(ntohs(packet->length) + sizeof(*packet)))
>>  		return -1;
>>
>>  	/* drop incompatible packet */
> 
> Thanks for the patch. It is basically correct but maybe you can modify it
> slightly to make it also catch very small packets.

It already does that in the first line of the if statement. Your
modification only checks this earlier, but because no data from the
packet is accessed before the length check, it should not matter when we
do this. Or am I missing the point?

> 
> diff --git a/recv.c b/recv.c
> index 90db0b3..288f577 100644
> --- a/recv.c
> +++ b/recv.c
> @@ -391,7 +391,12 @@ int recv_alfred_packet(struct globals *globals, struct interface *interface)
>  		return -1;
>  	}
>  
> +	/* drop packets smaller than tlv */
> +	if (length < (int)sizeof(*packet))
> +		return -1;
> +
>  	packet = (struct alfred_tlv *)buf;
> +	length -= sizeof(*packet);
>  
>  	/* drop packets not sent over link-local ipv6 */
>  	if (!is_ipv6_eui64(&source.sin6_addr))
> 
> Kind regards,
> 	Sven
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] alfred: Tighten size check on received packet

[Sven Eckelmann]

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

On Tuesday 20 January 2015 09:01:54 Jan-Philipp Litza wrote:
> It already does that in the first line of the if statement. Your
> modification only checks this earlier, but because no data from the
> packet is accessed before the length check, it should not matter when we
> do this. Or am I missing the point?

Sorry, only looked with one eye at the patch when I came in this morning. So I 
missed the first line.

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] alfred: Tighten size check on received packet

[Simon Wunderlich]

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

On Monday 19 January 2015 21:59:32 Jan-Philipp Litza wrote:
> When first checking if a received packet is truncated, the size of the
> alfred_tlv structure is ignored, thus allowing packets that are
> truncated by 4 bytes or less to pass the check unnoticed.
> 
> Even the check itself might access memory after the packet if its size
> was only 2 bytes or less.
> 
> Signed-off-by: Jan-Philipp Litza <janphilipp@litza.de>

applied in commit 0e2728c.

Thanks!
    Simon

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