After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 794170 - gdbus: hexdecode() and hexencode() do not return/use decoded/encoded string length
gdbus: hexdecode() and hexencode() do not return/use decoded/encoded string l...
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gdbus
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2018-03-08 09:38 UTC by LRN
Modified: 2018-03-13 12:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdbus: actually return string length from hexdecode() (734 bytes, patch)
2018-03-08 09:39 UTC, LRN
none Details | Review
gdbus: actually return string length from hexdecode() v2 (6.14 KB, patch)
2018-03-08 10:43 UTC, LRN
committed Details | Review
gdbus: pass string length to hexencode() (2.46 KB, patch)
2018-03-08 10:45 UTC, LRN
none Details | Review
gdbus: pass string length to hexencode() v2 (2.77 KB, patch)
2018-03-08 11:27 UTC, LRN
committed Details | Review
gdbus: do not initialize gsize variables with -1 (1.58 KB, patch)
2018-03-08 11:27 UTC, LRN
committed Details | Review
gdbus: make hexencode() a shared function to avoid duplication (6.50 KB, patch)
2018-03-09 09:07 UTC, LRN
committed Details | Review

Description LRN 2018-03-08 09:38:54 UTC
It's really quite amazing that this went unnoticed for
roughly 7 or 8 years. I guess all existing authentication
mechanisms don't use string lengths (relying on NUL-terminator
or something like that?). Anyway, i've tried to implement a new
mechanism, and this bug tripped me.
Comment 1 LRN 2018-03-08 09:39:02 UTC
Created attachment 369427 [details] [review]
gdbus: actually return string length from hexdecode()
Comment 2 Philip Withnall 2018-03-08 09:55:50 UTC
Review of attachment 369427 [details] [review]:

Oops, good catch. Looks like mechanism_server_initiate() in gdbusauthmechanismsha1.c and data_matches_credentials() in gdbusauthmechanismexternal.c can be modified to use the initial_response_len rather than strlen() too.

::: gio/gdbusauth.c
@@ +405,3 @@
     }
 
+  *out_len = s->len;

You should set `*out_len = 0` in case of error too, so add this to the `out` label:

```
out:
  if (s != NULL)
    {
      *out_len = 0;
      g_string_free (s, TRUE);
    }
   return ret;

```
Comment 3 LRN 2018-03-08 10:43:52 UTC
Created attachment 369429 [details] [review]
gdbus: actually return string length from hexdecode() v2

v2:
* Set *out_len to 0 when returning NULL
* Force mechanisms to use the length where appropriate
Comment 4 LRN 2018-03-08 10:45:33 UTC
Created attachment 369430 [details] [review]
gdbus: pass string length to hexencode()

This is necessary for it to be able to encode strings
with embedded NUL characters.
Comment 5 Philip Withnall 2018-03-08 11:09:42 UTC
Review of attachment 369429 [details] [review]:

LGTM, thanks.
Comment 6 Philip Withnall 2018-03-08 11:11:04 UTC
Review of attachment 369430 [details] [review]:

::: gio/gdbusauth.c
@@ +427,2 @@
   s = g_string_new (NULL);
+  for (n = 0; str_len < 0 ? str[n] != '\0' : n < str_len; n++)

(str_len < 0) isn’t possible, since gsize is unsigned. Just require str_len to be passed, and omit the nul byte check.
Comment 7 LRN 2018-03-08 11:18:16 UTC
(In reply to Philip Withnall from comment #6)
> Review of attachment 369430 [details] [review] [review]:
> 
> ::: gio/gdbusauth.c
> @@ +427,2 @@
>    s = g_string_new (NULL);
> +  for (n = 0; str_len < 0 ? str[n] != '\0' : n < str_len; n++)
> 
> (str_len < 0) isn’t possible, since gsize is unsigned. Just require str_len
> to be passed, and omit the nul byte check.

Oh...okay, then why is there stuff like:
>  gsize initial_response_len;
>  initial_response_len = -1;
and
>  mechanism_client_initiate = -1;
?

In fact, sha1 mechanism has a *separate* hexencode() implementation that *does* take gssize length...
Comment 8 LRN 2018-03-08 11:19:06 UTC
(In reply to LRN from comment #7)
> and
> >  mechanism_client_initiate = -1;
> ?
> 

Sorry, i meant
> *out_initial_response_len = -1;
Comment 9 LRN 2018-03-08 11:27:21 UTC
Created attachment 369434 [details] [review]
gdbus: pass string length to hexencode() v2

v2:
* Fix the counting, as length is unsigned
* Ensure that length given to hexencode() is not initialized with -1
Comment 10 LRN 2018-03-08 11:27:41 UTC
Created attachment 369435 [details] [review]
gdbus: do not initialize gsize variables with -1

Use 0 instead, since gsize is unsigned.
Comment 11 Philip Withnall 2018-03-08 11:37:52 UTC
Review of attachment 369434 [details] [review]:

r+
Comment 12 Philip Withnall 2018-03-08 11:38:36 UTC
Review of attachment 369435 [details] [review]:

r+
Comment 13 Philip Withnall 2018-03-08 11:42:32 UTC
(In reply to LRN from comment #7)
> (In reply to Philip Withnall from comment #6)
> > Review of attachment 369430 [details] [review] [review] [review]:
> > 
> > ::: gio/gdbusauth.c
> > @@ +427,2 @@
> >    s = g_string_new (NULL);
> > +  for (n = 0; str_len < 0 ? str[n] != '\0' : n < str_len; n++)
> > 
> > (str_len < 0) isn’t possible, since gsize is unsigned. Just require str_len
> > to be passed, and omit the nul byte check.
> 
> Oh...okay, then why is there stuff like:
> >  gsize initial_response_len;
> >  initial_response_len = -1;
> and
> >  mechanism_client_initiate = -1;
> ?
> 
> In fact, sha1 mechanism has a *separate* hexencode() implementation that
> *does* take gssize length...

It would be good to merge the two, to reduce code duplication. Move one of them to gdbusprivate.c, name it `_g_dbus_hexencode()` and have it take the unsigned gsize argument, since none of the call sites need to pass -1.
Comment 14 LRN 2018-03-09 09:07:41 UTC
Created attachment 369483 [details] [review]
gdbus: make hexencode() a shared function to avoid duplication
Comment 15 Philip Withnall 2018-03-09 11:58:15 UTC
Review of attachment 369483 [details] [review]:

Lovely, thanks.
Comment 16 Philip Withnall 2018-03-13 12:13:15 UTC
All pushed to master for 2.58.

Attachment 369435 [details] pushed as cc7ab04 - gdbus: do not initialize gsize variables with -1
Attachment 369483 [details] pushed as aab83f7 - gdbus: make hexencode() a shared function to avoid duplication