telepathy-glib: _tp_create_temp_unix_socket: avoid using tmpnam()

[Simon McVittie]

Module: telepathy-glib
Branch: master
Commit: f4076dffc10600006dd6415c3584f97bd36907a6
URL:    http://cgit.freedesktop.org/telepathy/telepathy-glib/commit/?id=f4076dffc10600006dd6415c3584f97bd36907a6

Author: Simon McVittie <simon.mcvittie at collabora.co.uk>
Date:   Tue Aug 20 20:42:10 2013 +0100

_tp_create_temp_unix_socket: avoid using tmpnam()

On current Debian unstable, gcc/ld issues a warning about tmpnam(),
because it's usually used in an unsafe way. "gcc -Wl,--fatal-warnings"
(which I'm using in my development environment) upgrades that to fatal.

Our usage was in fact safe (trying to listen on a socket always
behaves like O_EXCL|O_CREAT, which can DoS'd but is not subject to
symlink attacks), but we're swimming against the current by trying
to use tmpnam(). Instead, create a secure private temporary directory
with g_dir_make_tmp(), and put our socket in there.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=68350
Reviewed-by: Guillaume Desmottes <guillaume.desmottes at collabora.co.uk>

---

 telepathy-glib/stream-tube-channel.c |   10 ++++++-
 telepathy-glib/util-internal.h       |    1 +
 telepathy-glib/util.c                |   51 ++++++++++++++++++++++-----------
 3 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/telepathy-glib/stream-tube-channel.c b/telepathy-glib/stream-tube-channel.c
index 607abad..8a110d7 100644
--- a/telepathy-glib/stream-tube-channel.c
+++ b/telepathy-glib/stream-tube-channel.c
@@ -145,6 +145,7 @@ struct _TpStreamTubeChannelPrivate
   /* Offering side */
   GSocketService *service;
   GSocketAddress *address;
+  gchar *unix_tmpdir;
   /* GSocketConnection we have accepted but are still waiting a
    * NewRemoteConnection to identify them. Owned ConnWaitingSig. */
   GSList *conn_waiting_sig;
@@ -266,6 +267,13 @@ tp_stream_tube_channel_dispose (GObject *obj)
       self->priv->address = NULL;
     }
 
+    if (self->priv->unix_tmpdir != NULL)
+      {
+        g_rmdir (self->priv->unix_tmpdir);
+        g_free (self->priv->unix_tmpdir);
+        self->priv->unix_tmpdir = NULL;
+      }
+
   tp_clear_pointer (&self->priv->access_control_param, tp_g_value_slice_free);
   tp_clear_object (&self->priv->local_conn_waiting_id);
   tp_clear_object (&self->priv->client_socket);
@@ -1466,7 +1474,7 @@ tp_stream_tube_channel_offer_async (TpStreamTubeChannel *self,
       case TP_SOCKET_ADDRESS_TYPE_UNIX:
         {
           self->priv->address = _tp_create_temp_unix_socket (
-              self->priv->service, &error);
+              self->priv->service, &self->priv->unix_tmpdir, &error);
 
           /* check there wasn't an error on the final attempt */
           if (self->priv->address == NULL)
diff --git a/telepathy-glib/util-internal.h b/telepathy-glib/util-internal.h
index 9410648..b1ecaa4 100644
--- a/telepathy-glib/util-internal.h
+++ b/telepathy-glib/util-internal.h
@@ -39,6 +39,7 @@ void _tp_quark_array_merge_valist (GArray *array,
 
 #ifdef HAVE_GIO_UNIX
 GSocketAddress * _tp_create_temp_unix_socket (GSocketService *service,
+    gchar **tmpdir,
     GError **error);
 #endif /* HAVE_GIO_UNIX */
 
diff --git a/telepathy-glib/util.c b/telepathy-glib/util.c
index 91a42d5..c89bc66 100644
--- a/telepathy-glib/util.c
+++ b/telepathy-glib/util.c
@@ -31,6 +31,7 @@
 
 #include "config.h"
 
+#include <glib/gstdio.h>
 #include <gobject/gvaluecollector.h>
 
 #ifdef HAVE_GIO_UNIX
@@ -43,6 +44,7 @@
 #include <telepathy-glib/util-internal.h>
 #include <telepathy-glib/util.h>
 
+#include <errno.h>
 #include <stdio.h>
 #include <string.h>
 
@@ -1563,32 +1565,47 @@ _tp_quark_array_merge_valist (GArray *array,
 #ifdef HAVE_GIO_UNIX
 GSocketAddress *
 _tp_create_temp_unix_socket (GSocketService *service,
+    gchar **tmpdir,
     GError **error)
 {
-  guint i;
   GSocketAddress *address;
+  gchar *dir = g_dir_make_tmp ("tp-glib-socket.XXXXXX", error);
+  gchar *name;
+
+  if (dir == NULL)
+    return NULL;
 
-  /* why doesn't GIO provide a method to create a socket we don't
-   * care about? Iterate until we find a valid temporary name.
-   *
-   * Try a maximum of 10 times to get a socket */
-  for (i = 0; i < 10; i++)
+  if (g_chmod (dir, 0700) != 0)
     {
-      address = g_unix_socket_address_new (tmpnam (NULL));
+      int e = errno;
 
-      g_clear_error (error);
+      g_set_error (error, G_IO_ERROR, g_io_error_from_errno (e),
+          "unable to set permissions of %s to 0700: %s", dir,
+          g_strerror (e));
+      g_free (dir);
+      return NULL;
+    }
 
-      if (g_socket_listener_add_address (
-            G_SOCKET_LISTENER (service),
-            address, G_SOCKET_TYPE_STREAM,
-            G_SOCKET_PROTOCOL_DEFAULT,
-            NULL, NULL, error))
-        return address;
-      else
-        g_object_unref (address);
+  name = g_build_filename (dir, "s", NULL);
+  address = g_unix_socket_address_new (name);
+  g_free (name);
+
+  if (!g_socket_listener_add_address (G_SOCKET_LISTENER (service),
+        address, G_SOCKET_TYPE_STREAM,
+        G_SOCKET_PROTOCOL_DEFAULT,
+        NULL, NULL, error))
+    {
+      g_object_unref (address);
+      g_free (dir);
+      return NULL;
     }
 
-  return NULL;
+  if (tmpdir != NULL)
+    *tmpdir = dir;
+  else
+    g_free (dir);
+
+  return address;
 }
 #endif /* HAVE_GIO_UNIX */