GNOME Bugzilla – Bug 794170
gdbus: hexdecode() and hexencode() do not return/use decoded/encoded string length
Last modified: 2018-03-13 12:13:34 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.
Created attachment 369427 [details] [review] gdbus: actually return string length from hexdecode()
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; ```
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
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.
Review of attachment 369429 [details] [review]: LGTM, thanks.
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.
(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...
(In reply to LRN from comment #7) > and > > mechanism_client_initiate = -1; > ? > Sorry, i meant > *out_initial_response_len = -1;
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
Created attachment 369435 [details] [review] gdbus: do not initialize gsize variables with -1 Use 0 instead, since gsize is unsigned.
Review of attachment 369434 [details] [review]: r+
Review of attachment 369435 [details] [review]: r+
(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.
Created attachment 369483 [details] [review] gdbus: make hexencode() a shared function to avoid duplication
Review of attachment 369483 [details] [review]: Lovely, thanks.
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