> + 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_warning(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
doing:
+ return GINT64_FROM_BE(*(gint64*)buf);
will fail (cause a crash like SIGBUS IIRC) if buf isn't aligned
correctly.
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
(dbus-marshal-recursive.c).
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