From ef22e53c9360ddb4bdff61a12013a2812fb7346a Mon Sep 17 00:00:00 2001
From: longtao <longtao.wu@zstack.io>
Date: Thu, 16 Jun 2022 14:08:14 +0800
Subject: [PATCH] virMacAddrParse: Fix wrong termination character

The judgment of the termination character should be the '\0' character, not
a space.
Using spaces to judge, content can be injected into mac. such as:
"70:af:e7:1f:3f:89\32injected".

Before this patch, the terminating character was a space ('\32'),not '\0'.
So I can set the DHCP host mac like this "<host mac='c0:3b:04:21:15:35
 injected' name='name129' ip='192.168.100.129'/>".
When running the network, no error is reported.
But, when using this mac to create a virtual machine,  Will get
"virNetSocketReadWire:1805 : End of file while reading data: Input/output
error" in the libvirtd log.
---
 src/util/virmacaddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
index 6b22384cee..ba7c7e7076 100644
--- a/src/util/virmacaddr.c
+++ b/src/util/virmacaddr.c
@@ -163,7 +163,7 @@ virMacAddrParse(const char* str, virMacAddr *addr)

         addr->addr[i] = (unsigned char) result;

-        if ((i == 5) && (*end_ptr <= ' '))
+        if ((i == 5) && (*end_ptr == 0))
             return 0;
         if (*end_ptr != ':')
             break;
-- 
2.32.0

On Thu, Jun 16, 2022 at 14:09:06 +0800, Eustance Wu wrote:
> From ef22e53c9360ddb4bdff61a12013a2812fb7346a Mon Sep 17 00:00:00 2001
> From: longtao <longtao.wu@zstack.io>
> Date: Thu, 16 Jun 2022 14:08:14 +0800
> Subject: [PATCH] virMacAddrParse: Fix wrong termination character
> 
> The judgment of the termination character should be the '\0' character, not
> a space.
> Using spaces to judge, content can be injected into mac. such as:
> "70:af:e7:1f:3f:89\32injected".
> 
> Before this patch, the terminating character was a space ('\32'),not '\0'.

This sentence is inaccurate, it was any byte between 0 and ' '.

> So I can set the DHCP host mac like this "<host mac='c0:3b:04:21:15:35
>  injected' name='name129' ip='192.168.100.129'/>".

This format does not conform to our XML schema. The 'mac' attribute is
defined as:

  <define name="uniMacAddr">
    <data type="string">
      <param name="pattern">[a-fA-F0-9][02468aAcCeE](:[a-fA-F0-9]{2}){5}</param>
    </data>
  </define>

Thus 'space' or any other character is _not_ allowed in int including
the word 'injected'.

What are you actually trying to achieve? This is not clear from the
description in this bug.

Please describe also the end goal ...

> When running the network, no error is reported.
> But, when using this mac to create a virtual machine,  Will get
> "virNetSocketReadWire:1805 : End of file while reading data: Input/output

... rather than just an error you are seeing, since this doesn't seem to
be a plain bugfix.

> error" in the libvirtd log.
> ---
>  src/util/virmacaddr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 6b22384cee..ba7c7e7076 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -163,7 +163,7 @@ virMacAddrParse(const char* str, virMacAddr *addr)

This helper is used in many places, outside of the network driver, so
this will need careful assesment whether any other code depends on the
old behaviour.

> 
>          addr->addr[i] = (unsigned char) result;
> 
> -        if ((i == 5) && (*end_ptr <= ' '))
> +        if ((i == 5) && (*end_ptr == 0))

We prefer to use the character literal version of the nul byte ('\0').

>              return 0;
>          if (*end_ptr != ':')
>              break;
> -- 
> 2.32.0

Thanks a lot.

In libvirt of the latest code of the current master: When configuring the
mac address of the DHCP host (c0:3b:04:21:15:35), if a space is added at
the end (c0:3b:04:21:15:35\32 ), no error is thrown when starting the
network.
I think an error should be thrown because such a mac (c0:3b:04:21:15:35\32)
is not legal
And the mac address (c0:3b:04:21:15:35) does not take effect
The diagram is as follows:
[image: image.png]

> >*          addr->addr[i] = (unsigned char) result;
*> >* -        if ((i == 5) && (*end_ptr <= ' '))
*>* +        if ((i == 5) && (*end_ptr == '\0'))
*



Can I mention a patch for this issue?
Will this affect other functions?


Peter Krempa <pkrempa@redhat.com> 于2022年6月16日周四 14:52写道：

> On Thu, Jun 16, 2022 at 14:09:06 +0800, Eustance Wu wrote:
> > From ef22e53c9360ddb4bdff61a12013a2812fb7346a Mon Sep 17 00:00:00 2001
> > From: longtao <longtao.wu@zstack.io>
> > Date: Thu, 16 Jun 2022 14:08:14 +0800
> > Subject: [PATCH] virMacAddrParse: Fix wrong termination character
> >
> > The judgment of the termination character should be the '\0' character,
> not
> > a space.
> > Using spaces to judge, content can be injected into mac. such as:
> > "70:af:e7:1f:3f:89\32injected".
> >
> > Before this patch, the terminating character was a space ('\32'),not
> '\0'.
>
> This sentence is inaccurate, it was any byte between 0 and ' '.
>
> > So I can set the DHCP host mac like this "<host mac='c0:3b:04:21:15:35
> >  injected' name='name129' ip='192.168.100.129'/>".
>
> This format does not conform to our XML schema. The 'mac' attribute is
> defined as:
>
>   <define name="uniMacAddr">
>     <data type="string">
>       <param
> name="pattern">[a-fA-F0-9][02468aAcCeE](:[a-fA-F0-9]{2}){5}</param>
>     </data>
>   </define>
>
> Thus 'space' or any other character is _not_ allowed in int including
> the word 'injected'.
>
> What are you actually trying to achieve? This is not clear from the
> description in this bug.
>
> Please describe also the end goal ...
>
> > When running the network, no error is reported.
> > But, when using this mac to create a virtual machine,  Will get
> > "virNetSocketReadWire:1805 : End of file while reading data: Input/output
>
> ... rather than just an error you are seeing, since this doesn't seem to
> be a plain bugfix.
>
> > error" in the libvirtd log.
> > ---
> >  src/util/virmacaddr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> > index 6b22384cee..ba7c7e7076 100644
> > --- a/src/util/virmacaddr.c
> > +++ b/src/util/virmacaddr.c
> > @@ -163,7 +163,7 @@ virMacAddrParse(const char* str, virMacAddr *addr)
>
> This helper is used in many places, outside of the network driver, so
> this will need careful assesment whether any other code depends on the
> old behaviour.
>
> >
> >          addr->addr[i] = (unsigned char) result;
> >
> > -        if ((i == 5) && (*end_ptr <= ' '))
> > +        if ((i == 5) && (*end_ptr == 0))
>
> We prefer to use the character literal version of the nul byte ('\0').
>
> >              return 0;
> >          if (*end_ptr != ':')
> >              break;
> > --
> > 2.32.0
>
>