[B.A.T.M.A.N.] [PATCH] alfred: Avoid buffer overflow while querying ARP cache

[B.A.T.M.A.N.] [PATCH] alfred: Avoid buffer overflow while querying ARP cache

[Sven Eckelmann]

The arpreq.arp_dev is a limited buffer (16 bytes). Avoid that more bytes
from the interface name are copied into this buffer by switching from
strcpy to strncpy.

Fixes: c7da798113a2 ("alfred: IPv4 multicast distribution support.")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 util.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util.c b/util.c
index 84ab3af..ed83895 100644
--- a/util.c
+++ b/util.c
@@ -92,7 +92,9 @@ int ipv4_arp_request(struct interface *interface, const alfred_addr *addr,
 	sin->sin_family = AF_INET;
 	sin->sin_addr.s_addr = addr->ipv4.s_addr;
 
-	strcpy(arpreq.arp_dev, interface->interface);
+	strncpy(arpreq.arp_dev, interface->interface, sizeof(arpreq.arp_dev));
+	arpreq.arp_dev[sizeof(arpreq.arp_dev) - 1] = '\0';
+
 	if (ioctl(interface->netsock, SIOCGARP, &arpreq) < 0)
 		return -1;
 
-- 
2.11.0

Re: [B.A.T.M.A.N.] [PATCH] alfred: Avoid buffer overflow while querying ARP cache

[Antonio Quartulli]

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

On Fri, Jan 27, 2017 at 03:10:44PM +0100, Sven Eckelmann wrote:
> The arpreq.arp_dev is a limited buffer (16 bytes). Avoid that more bytes
> from the interface name are copied into this buffer by switching from
> strcpy to strncpy.
> 
> Fixes: c7da798113a2 ("alfred: IPv4 multicast distribution support.")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  util.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/util.c b/util.c
> index 84ab3af..ed83895 100644
> --- a/util.c
> +++ b/util.c
> @@ -92,7 +92,9 @@ int ipv4_arp_request(struct interface *interface, const alfred_addr *addr,
>  	sin->sin_family = AF_INET;
>  	sin->sin_addr.s_addr = addr->ipv4.s_addr;
>  
> -	strcpy(arpreq.arp_dev, interface->interface);
> +	strncpy(arpreq.arp_dev, interface->interface, sizeof(arpreq.arp_dev));

arpreq is already set to 0 few lines above. why not simpling
"sizeof(arpreq.arp_dev) - 1" as last argument for the strncpy() and avoid the
line below?

Or is this required for consistency with the rest of the code?


Cheers,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] alfred: Avoid buffer overflow while querying ARP cache

[Sven Eckelmann]

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

On Freitag, 27. Januar 2017 22:34:22 CET Antonio Quartulli wrote:
[...]
> arpreq is already set to 0 few lines above. why not simpling
> "sizeof(arpreq.arp_dev) - 1" as last argument for the strncpy() and avoid the
> line below?
> 
> Or is this required for consistency with the rest of the code?

It is done for consistency.

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] alfred: Avoid buffer overflow while querying ARP cache

[Simon Wunderlich]

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

On Friday, January 27, 2017 3:10:44 PM CET Sven Eckelmann wrote:
> The arpreq.arp_dev is a limited buffer (16 bytes). Avoid that more bytes
> from the interface name are copied into this buffer by switching from
> strcpy to strncpy.
> 
> Fixes: c7da798113a2 ("alfred: IPv4 multicast distribution support.")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Committed in revision 26ef92b.

Thank you,
     Simon

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