[PATCH] iproute2: ip maddress: Check multiaddr length

[PATCH] iproute2: ip maddress: Check multiaddr length

[Sascha Hauer]

ip maddress add|del takes a MAC address as argument, so insist on
getting a length of ETH_ALEN bytes. This makes sure the passed argument
is actually a MAC address and especially not an IPv4 address which
was previously accepted and silently taken as a MAC address.

While at it, do not print *argv in the error path as this has been
modified by ll_addr_a2n() and doesn't contain the full string anymore,
which can lead to misleading error messages.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 ip/ipmaddr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 3400e055..9979ed58 100644
--- a/ip/ipmaddr.c
+++ b/ip/ipmaddr.c
@@ -291,7 +291,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
 {
 	struct ifreq ifr = {};
 	int family;
-	int fd;
+	int fd, len;
 
 	if (cmd == RTM_NEWADDR)
 		cmd = SIOCADDMULTI;
@@ -313,9 +313,12 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
 				usage();
 			if (ifr.ifr_hwaddr.sa_data[0])
 				duparg("address", *argv);
-			if (ll_addr_a2n(ifr.ifr_hwaddr.sa_data,
-					14, *argv) < 0) {
-				fprintf(stderr, "Error: \"%s\" is not a legal ll address.\n", *argv);
+			len = ll_addr_a2n(ifr.ifr_hwaddr.sa_data, 14, *argv);
+			if (len < 0)
+				exit(1);
+
+			if (len != ETH_ALEN) {
+				fprintf(stderr, "Error: Invalid address length %d - must be %d bytes\n", len, ETH_ALEN);
 				exit(1);
 			}
 		}
-- 
2.28.0

Re: [PATCH] iproute2: ip maddress: Check multiaddr length

[Stephen Hemminger]

On Fri, 14 Aug 2020 10:46:26 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> ip maddress add|del takes a MAC address as argument, so insist on
> getting a length of ETH_ALEN bytes. This makes sure the passed argument
> is actually a MAC address and especially not an IPv4 address which
> was previously accepted and silently taken as a MAC address.
> 
> While at it, do not print *argv in the error path as this has been
> modified by ll_addr_a2n() and doesn't contain the full string anymore,
> which can lead to misleading error messages.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  ip/ipmaddr.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
> index 3400e055..9979ed58 100644
> --- a/ip/ipmaddr.c
> +++ b/ip/ipmaddr.c
> @@ -291,7 +291,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
>  {
>  	struct ifreq ifr = {};
>  	int family;
> -	int fd;
> +	int fd, len;
>  
>  	if (cmd == RTM_NEWADDR)
>  		cmd = SIOCADDMULTI;
> @@ -313,9 +313,12 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
>  				usage();
>  			if (ifr.ifr_hwaddr.sa_data[0])
>  				duparg("address", *argv);
> -			if (ll_addr_a2n(ifr.ifr_hwaddr.sa_data,
> -					14, *argv) < 0) {
> -				fprintf(stderr, "Error: \"%s\" is not a legal ll address.\n", *argv);
> +			len = ll_addr_a2n(ifr.ifr_hwaddr.sa_data, 14, *argv);

While you are at it, get rid of the hard code 14 here and use sizeof(ifr.ifr_hwaddr.sa_data)?

Re: [PATCH] iproute2: ip maddress: Check multiaddr length

[Sascha Hauer]

On Fri, Aug 14, 2020 at 08:27:56AM -0700, Stephen Hemminger wrote:
> On Fri, 14 Aug 2020 10:46:26 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > ip maddress add|del takes a MAC address as argument, so insist on
> > getting a length of ETH_ALEN bytes. This makes sure the passed argument
> > is actually a MAC address and especially not an IPv4 address which
> > was previously accepted and silently taken as a MAC address.
> > 
> > While at it, do not print *argv in the error path as this has been
> > modified by ll_addr_a2n() and doesn't contain the full string anymore,
> > which can lead to misleading error messages.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  ip/ipmaddr.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
> > index 3400e055..9979ed58 100644
> > --- a/ip/ipmaddr.c
> > +++ b/ip/ipmaddr.c
> > @@ -291,7 +291,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
> >  {
> >  	struct ifreq ifr = {};
> >  	int family;
> > -	int fd;
> > +	int fd, len;
> >  
> >  	if (cmd == RTM_NEWADDR)
> >  		cmd = SIOCADDMULTI;
> > @@ -313,9 +313,12 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
> >  				usage();
> >  			if (ifr.ifr_hwaddr.sa_data[0])
> >  				duparg("address", *argv);
> > -			if (ll_addr_a2n(ifr.ifr_hwaddr.sa_data,
> > -					14, *argv) < 0) {
> > -				fprintf(stderr, "Error: \"%s\" is not a legal ll address.\n", *argv);
> > +			len = ll_addr_a2n(ifr.ifr_hwaddr.sa_data, 14, *argv);
> 
> While you are at it, get rid of the hard code 14 here and use sizeof(ifr.ifr_hwaddr.sa_data)?

Ok. I just sent out a v2.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |