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 */
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
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
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
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
b.a.t.m.a.n@lists.open-mesh.org
-
Jan-Philipp Litza -
Simon Wunderlich -
Sven Eckelmann