Created attachment 213948 [details] [review]
gresolver: More robust parsing of DNS responses

Review of attachment 213948 [details] [review]:

Hm, was the old code exploitable?  Seems like there are at least several cases where we'd walk off the end of a stack buffer.

I think this code needs a more extensive audit.

::: gio/gresolver.c
@@ +1017,3 @@
+  if (*p + 4 > end)
+    return FALSE;
+  GETSHORT (*value, *p);

GETLONG ?

Created attachment 213977 [details] [review]
Updated: gresolver: More robust parsing of DNS responses

I'm not sure. It seems you could make the application crash. But we
never wrote to the buffer, so perhaps that makes it not exploitable? 

FWIW, it seems like we probably need to handle truncated DNS responses
without just failing. But obviously we shouldn't read off the end of
the application buffer.

Comment on attachment 213977 [details] [review]
Updated: gresolver: More robust parsing of DNS responses

> I'm not sure. It seems you could make the application crash. But we
> never wrote to the buffer, so perhaps that makes it not exploitable? 

"can make the app crash remotely" is sometimes considered "exploitable" (eg, a denial of service attack) even if you can't do anything else.

> FWIW, it seems like we probably need to handle truncated DNS responses
> without just failing.

If the response is truncated, it's someone else's fault, so there's not a lot we can do. But we should definitely return G_RESOLVER_ERROR_TEMPORARY_FAILURE, not G_RESOLVER_ERROR_NOT_FOUND.

>+parse_short (guchar **p,
>+             guchar *end,
>+             guint16 *value)

line up args please

>-  while (count-- && p < end)
>+  for (i = 0; i < count && p < end; i++)
...
>+  /* Incomplete response */
>+  if (count > 0)
>+    return NULL;

should be "if (i < count)" now, right?

Looks like here:

        case T_TXT:
          record = parse_res_txt (answer, p + rdlength, &p);
          break;

We should ensure that p+rdlength < end.

That's all I saw in about 15 minutes of looking over the code, but the whole dn_* DNS API is completely insane.  Then again it's papering over the original DNS wire protocol, which is itself completely insane ( http://cr.yp.to/djbdns/notes.html )

Created attachment 215163 [details] [review]
Updated: gresolver: More robust parsing of DNS responses

Right, so the stable version of GResolver between 2.22.x and 2.32.x is crashable.  Should we add a similar fix for this to the 2.32.x stable branch?

Lined up args, added explicit failures for responses that are somehow truncated.
More checks for parsing out of bounds.

Created attachment 215164 [details] [review]
Patch for glib-2-32 branch

gresolver: More robust parsing of DNS responses

 * Handle truncated responses, and invalid names

Comment on attachment 215163 [details] [review]
Updated: gresolver: More robust parsing of DNS responses

looks good. I don't know if we care about 2.32 (or if there are going to be any more releases on glib-2-32 anyway)