Re: [Rhythmbox-devel] music sharing patch #2

> +       switch (rb_daap_content_code_rb_daap_type (code)) {
> +               case RB_DAAP_TYPE_BYTE:
> +               case RB_DAAP_TYPE_SIGNED_INT:
> +                       return G_TYPE_CHAR;

Will a G_TYPE_CHAR hold everything a _TYPE_SIGNED_INT expects?
Should this really map to G_TYPE_INT, and _TYPE_INT maps to G_TYPE_UINT?

> +                       gchar c = (gchar)va_arg (list, gint32);

You can probably use G_VALUE_COLLECT here; this is an example usage from
the DBus GLib bindings:

      GValue value = {0,};
      char *error;
      g_value_init (&value, gtype);
      error = NULL;
      G_VALUE_COLLECT (&value, args, G_VALUE_NOCOPY_CONTENTS, &error);
      if (error)
	  g_free (error);

> +               item->content_code = rb_daap_buffer_read_content_code
> (&(buf[l]));
> +               l += 4;
> +				gint16 minor = rb_daap_buffer_read_int16(&buf[l] + 2);

This stuff is problematic because the buffer length is controlled by the
attacker (from the libsoup response structure, which AIUI is the HTTP
body size)  If they have a truncated message you'll read past the buffer
end, potentially causing a segfault.  

This is also true of the reading codesize for strings and then calling
g_strndup based on that data, etc.  This one might even be exploitable,
because a null terminator will be written by g_strndup...if an attacker
could put the null terminator in the heap control structure I bet they
could cause a later free() of invalid data which AIUI can often lead to
arbitrary code execution.

Also, I talked to a coworker briefly about this, he pointed out that

+	return GINT64_FROM_BE(*(gint64*)buf);

will fail (cause a crash like SIGBUS IIRC) if buf isn't aligned

What this comes down to is that deserializing data from a buffer in C is
a really hard problem :)  Unfortunately GLib doesn't really have an
appropriate API for this.  

The approach D-BUS takes from looking at their code is they have an
entirely separate "validation" function which just checks that the data
is sane (dbus-marshal-validate.c) and then code to demarshal

We could do that; maybe a simpler approach is to mix in the validation
and reading though.

So I guess what we'll have to do is basically take the approach of
adding if (length >= bufsize - itemsize) { break }; every time before we
read data to avoid overflow.  

For the alignment issue we should probably just read the data a byte at
a time, something like:

static gint
rb_daap_buffer_read_int32 (const char *buf, gsize remaining)
  int i;
  gint ret;

  if (remaining < 4)
    return 0;

  return GINT32_FROM_BE (buf[0] << 24 + buf[1] << 16 + buf[2] << 8 + buf[3]);

If the DAAP protocol requires wire data to be aligned we could
potentially verify the alignment and read the data directly (which is
what D-BUS does), but this way seems safer.

Attachment: signature.asc
Description: This is a digitally signed message part

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]