wpa_supplicant: If empty bytearray sent via DBUS use default value

In case wpa_supplicant receive empty byte array via dbus
(e.g.: from Network Manager which sends empty private key password field)
it will use the "NULL" default value.

Signed-off-by: Istvan Bodnar <mail.bodnaristvan@gmail.com>
---
 wpa_supplicant/dbus/dbus_new_handlers.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

                                goto error;
                } else if (entry.type == DBUS_TYPE_STRING) {

On Thu, 2019-08-22 at 21:43 +0200, István Bodnár wrote:
> In case wpa_supplicant receive empty byte array via dbus
> (e.g.: from Network Manager which sends empty private key password
> field)
> it will use the "NULL" default value.

Any idea which NM versions send this? NM should get fixed to not do
this if it's a problem.

Dan

> Signed-off-by: Istvan Bodnar <mail.bodnaristvan@gmail.com>
> ---
>  wpa_supplicant/dbus/dbus_new_handlers.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c
> b/wpa_supplicant/dbus/dbus_new_handlers.c
> index 6c36d91a0..45c16c2e7 100644
> --- a/wpa_supplicant/dbus/dbus_new_handlers.c
> +++ b/wpa_supplicant/dbus/dbus_new_handlers.c
> @@ -206,17 +206,25 @@ dbus_bool_t set_network_properties(struct
> wpa_supplicant *wpa_s,
>                 value = NULL;
>                 if (entry.type == DBUS_TYPE_ARRAY &&
>                     entry.array_type == DBUS_TYPE_BYTE) {
> -                       if (entry.array_len <= 0)
> +                       if (entry.array_len < 0)
>                                 goto error;
> +                       if (entry.array_len == 0)
> +                               size = strlen("NULL") + 3;
> +                       else
> +                               size = entry.array_len * 2 + 1;
> 
> -                       size = entry.array_len * 2 + 1;
>                         value = os_zalloc(size);
>                         if (value == NULL)
>                                 goto error;
> 
> -                       ret = wpa_snprintf_hex(value, size,
> -                                              (u8 *)
> entry.bytearray_value,
> -                                              entry.array_len);
> +                       if (entry.array_len == 0)
> +                               ret = os_snprintf(value, size,
> "\"%s\"",
> +                                                 "NULL");
> +                       else
> +                               ret = wpa_snprintf_hex(value, size,
> +                                                      (u8 *)
> entry.bytearray_value,
> +                                                      entry.array_le
> n);
> +
>                         if (ret <= 0)
>                                 goto error;
>                 } else if (entry.type == DBUS_TYPE_STRING) {
> _______________________________________________
> Hostap mailing list
> Hostap@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/hostap

The NM version which causes the problem is 1.10.6. I didn't check
other versions.
I think that wpa_supplicant should do the same in both cases, when it
is configured via config file and when it is configured via dbus
communication.
I mean that it shouldn't work differently if the only difference is
the interface which is used to configure wpa_supplicant, now this is
not fulfilled.
This commit can be abandoned, but what is the proposed way to solve
this inconsistency? I'm open to implement a better solution for this.

Istvan


Dan Williams <dcbw@redhat.com> ezt írta (időpont: 2019. aug. 22., Cs, 22:48):
>
> On Thu, 2019-08-22 at 21:43 +0200, István Bodnár wrote:
> > In case wpa_supplicant receive empty byte array via dbus
> > (e.g.: from Network Manager which sends empty private key password
> > field)
> > it will use the "NULL" default value.
>
> Any idea which NM versions send this? NM should get fixed to not do
> this if it's a problem.
>
> Dan
>
> > Signed-off-by: Istvan Bodnar <mail.bodnaristvan@gmail.com>
> > ---
> >  wpa_supplicant/dbus/dbus_new_handlers.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c
> > b/wpa_supplicant/dbus/dbus_new_handlers.c
> > index 6c36d91a0..45c16c2e7 100644
> > --- a/wpa_supplicant/dbus/dbus_new_handlers.c
> > +++ b/wpa_supplicant/dbus/dbus_new_handlers.c
> > @@ -206,17 +206,25 @@ dbus_bool_t set_network_properties(struct
> > wpa_supplicant *wpa_s,
> >                 value = NULL;
> >                 if (entry.type == DBUS_TYPE_ARRAY &&
> >                     entry.array_type == DBUS_TYPE_BYTE) {
> > -                       if (entry.array_len <= 0)
> > +                       if (entry.array_len < 0)
> >                                 goto error;
> > +                       if (entry.array_len == 0)
> > +                               size = strlen("NULL") + 3;
> > +                       else
> > +                               size = entry.array_len * 2 + 1;
> >
> > -                       size = entry.array_len * 2 + 1;
> >                         value = os_zalloc(size);
> >                         if (value == NULL)
> >                                 goto error;
> >
> > -                       ret = wpa_snprintf_hex(value, size,
> > -                                              (u8 *)
> > entry.bytearray_value,
> > -                                              entry.array_len);
> > +                       if (entry.array_len == 0)
> > +                               ret = os_snprintf(value, size,
> > "\"%s\"",
> > +                                                 "NULL");
> > +                       else
> > +                               ret = wpa_snprintf_hex(value, size,
> > +                                                      (u8 *)
> > entry.bytearray_value,
> > +                                                      entry.array_le
> > n);
> > +
> >                         if (ret <= 0)
> >                                 goto error;
> >                 } else if (entry.type == DBUS_TYPE_STRING) {
> > _______________________________________________
> > Hostap mailing list
> > Hostap@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/hostap
>

Hi!

Can someone help me with this issue? I would like to resolve this
inconsistency somehow.

Thank you,
Istvan


István Bodnár <mail.bodnaristvan@gmail.com> ezt írta (időpont: 2019.
aug. 22., Cs, 23:10):
>
> The NM version which causes the problem is 1.10.6. I didn't check
> other versions.
> I think that wpa_supplicant should do the same in both cases, when it
> is configured via config file and when it is configured via dbus
> communication.
> I mean that it shouldn't work differently if the only difference is
> the interface which is used to configure wpa_supplicant, now this is
> not fulfilled.
> This commit can be abandoned, but what is the proposed way to solve
> this inconsistency? I'm open to implement a better solution for this.
>
> Istvan
>
>
> Dan Williams <dcbw@redhat.com> ezt írta (időpont: 2019. aug. 22., Cs, 22:48):
> >
> > On Thu, 2019-08-22 at 21:43 +0200, István Bodnár wrote:
> > > In case wpa_supplicant receive empty byte array via dbus
> > > (e.g.: from Network Manager which sends empty private key password
> > > field)
> > > it will use the "NULL" default value.
> >
> > Any idea which NM versions send this? NM should get fixed to not do
> > this if it's a problem.
> >
> > Dan
> >
> > > Signed-off-by: Istvan Bodnar <mail.bodnaristvan@gmail.com>
> > > ---
> > >  wpa_supplicant/dbus/dbus_new_handlers.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c
> > > b/wpa_supplicant/dbus/dbus_new_handlers.c
> > > index 6c36d91a0..45c16c2e7 100644
> > > --- a/wpa_supplicant/dbus/dbus_new_handlers.c
> > > +++ b/wpa_supplicant/dbus/dbus_new_handlers.c
> > > @@ -206,17 +206,25 @@ dbus_bool_t set_network_properties(struct
> > > wpa_supplicant *wpa_s,
> > >                 value = NULL;
> > >                 if (entry.type == DBUS_TYPE_ARRAY &&
> > >                     entry.array_type == DBUS_TYPE_BYTE) {
> > > -                       if (entry.array_len <= 0)
> > > +                       if (entry.array_len < 0)
> > >                                 goto error;
> > > +                       if (entry.array_len == 0)
> > > +                               size = strlen("NULL") + 3;
> > > +                       else
> > > +                               size = entry.array_len * 2 + 1;
> > >
> > > -                       size = entry.array_len * 2 + 1;
> > >                         value = os_zalloc(size);
> > >                         if (value == NULL)
> > >                                 goto error;
> > >
> > > -                       ret = wpa_snprintf_hex(value, size,
> > > -                                              (u8 *)
> > > entry.bytearray_value,
> > > -                                              entry.array_len);
> > > +                       if (entry.array_len == 0)
> > > +                               ret = os_snprintf(value, size,
> > > "\"%s\"",
> > > +                                                 "NULL");
> > > +                       else
> > > +                               ret = wpa_snprintf_hex(value, size,
> > > +                                                      (u8 *)
> > > entry.bytearray_value,
> > > +                                                      entry.array_le
> > > n);
> > > +
> > >                         if (ret <= 0)
> > >                                 goto error;
> > >                 } else if (entry.type == DBUS_TYPE_STRING) {
> > > _______________________________________________
> > > Hostap mailing list
> > > Hostap@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/hostap
> >

On Thu, Aug 22, 2019 at 09:43:34PM +0200, István Bodnár wrote:
> In case wpa_supplicant receive empty byte array via dbus
> (e.g.: from Network Manager which sends empty private key password field)
> it will use the "NULL" default value.

Why would a string parameter like private_key_passwd be configured as
DBUS_TYPE_ARRAY(DBUS_TYPE_BYTE) instead of DBUS_TYPE_STRING?

It should also be noted that there is a difference between a zero-length
string value ("") and no value set (NULL). If we were to convert
zero-length byte array into NULL, there would be no way of setting an
empty string (""). When setting parameter values over the text-based
control interface, those two cases are separate:
SET_NETWORK 0 field NULL
SET_NETWORK 0 field ""

Based on the actual changes, there may also be some misunderstanding
here on that "default value" part.. Setting a string to NULL (i.e.,
without quotation marks) clears the parameter (sets the C pointer to
NULL) while setting a string to "NULL" (i.e., with quotation marks) sets
the parameter to a string "NULL" (i.e., allocated memory with NULL\0 in
it).

> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c
> @@ -206,17 +206,25 @@ dbus_bool_t set_network_properties(struct
> wpa_supplicant *wpa_s,
>                 value = NULL;
>                 if (entry.type == DBUS_TYPE_ARRAY &&
>                     entry.array_type == DBUS_TYPE_BYTE) {
> -                       if (entry.array_len <= 0)
> +                       if (entry.array_len < 0)
>                                 goto error;

This part might make sense to allow the zero-length value to be set
(and something similar for the DBUS_TYPE_STRING case for
should_quote_opt() cases).

> +                       if (entry.array_len == 0)
> +                               ret = os_snprintf(value, size, "\"%s\"",
> +                                                 "NULL");

This looks quite strange.. This would set the string to "NULL" instead
of removing a previously configured parameter. Was this supposed to be
simply os_snprintf(value, size, "NULL") to get the remove-a-parameter
behavior?

>                 } else if (entry.type == DBUS_TYPE_STRING) {
... this is the case where zero-length string is also rejected. That
should likely remain consistent with the DBUS_TYPE_ARRAY(DBUS_TYPE_BYTE)
case.