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 675966 - gresolver: More robust parsing of DNS responses
gresolver: More robust parsing of DNS responses
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-05-13 05:46 UTC by Stef Walter
Modified: 2012-06-22 06:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gresolver: More robust parsing of DNS responses (4.96 KB, patch)
2012-05-13 05:46 UTC, Stef Walter
reviewed Details | Review
Updated: gresolver: More robust parsing of DNS responses (4.96 KB, patch)
2012-05-14 03:00 UTC, Stef Walter
reviewed Details | Review
Updated: gresolver: More robust parsing of DNS responses (8.28 KB, patch)
2012-05-29 08:11 UTC, Stef Walter
committed Details | Review
Patch for glib-2-32 branch (4.22 KB, patch)
2012-05-29 08:28 UTC, Stef Walter
none Details | Review

Description Stef Walter 2012-05-13 05:46:24 UTC
 * Handle truncated responses, and invalid names
Comment 1 Stef Walter 2012-05-13 05:46:26 UTC
Created attachment 213948 [details] [review]
gresolver: More robust parsing of DNS responses
Comment 2 Colin Walters 2012-05-13 13:19:02 UTC
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 ?
Comment 3 Stef Walter 2012-05-14 03:00:09 UTC
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 4 Dan Winship 2012-05-14 13:26:49 UTC
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?
Comment 5 Colin Walters 2012-05-15 16:19:39 UTC
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 )
Comment 6 Stef Walter 2012-05-29 08:11:25 UTC
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.
Comment 7 Stef Walter 2012-05-29 08:28:05 UTC
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 8 Dan Winship 2012-06-20 14:20:27 UTC
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)