userdb: Reference-count DBusUserInfo, DBusGroupInfo

Previously, the hash table indexed by uid (or gid) took ownership of the
single reference to the heap-allocated struct, and the hash table
indexed by username (or group name) had a borrowed pointer to the same
struct that exists in the other hash table.

However, this can break down if you have two or more distinct usernames
that share a numeric identifier. This is generally a bad idea, because
the user-space model in such situations does not match the kernel-space
reality, and in particular there is no effective kernel-level security
boundary between such users, but it is sometimes done anyway.

In this case, when the second username is looked up in the userdb, it
overwrites (replaces) the entry in the hash table that is indexed by
uid, freeing the DBusUserInfo. This results in both the key and the
value in the hash table that is indexed by username becoming dangling
pointers (use-after-free), leading to undefined behaviour, which is
certainly not what we want to see when doing access control.

An equivalent situation can occur with groups, in the rare case where
a numeric group ID has two names (although I have not heard of this
being done in practice).

Solve this by reference-counting the data structure. There are up to
three references in practice: one held temporarily while the lookup
function is populating and storing it, one held by the hash table that
is indexed by uid, and one held by the hash table that is indexed by
name.

Closes: dbus#305
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 2b7948ef907669e844b52c4fa2268d6e3162a70c)

diff --git a/dbus/dbus-sysdeps-unix.h b/dbus/dbus-sysdeps-unix.h
index 8d3df2d672881709d8cf5bf860372c2a568b17c4..830d5cd03c6d61c57b46763df3e0f8d31ac73382 100644 (file)
--- a/dbus/dbus-sysdeps-unix.h
+++ b/dbus/dbus-sysdeps-unix.h
@@ -105,6 +105,7 @@ typedef struct DBusGroupInfo DBusGroupInfo;
  */
 struct DBusUserInfo
 {
+  size_t      refcount;       /**< Reference count */
   dbus_uid_t  uid;            /**< UID */
   dbus_gid_t  primary_gid;    /**< GID */
   dbus_gid_t *group_ids;      /**< Groups IDs, *including* above primary group */
@@ -118,6 +119,7 @@ struct DBusUserInfo
  */
 struct DBusGroupInfo
 {
+  size_t      refcount;       /**< Reference count */
   dbus_gid_t  gid;            /**< GID */
   char       *groupname;      /**< Group name */
 };

diff --git a/dbus/dbus-userdb-util.c b/dbus/dbus-userdb-util.c
index af880ed0177062023cdd5400fe8f9ad2160b5351..170d233e811d6c1da8f8039b3d6e11b07fab6da6 100644 (file)
--- a/dbus/dbus-userdb-util.c
+++ b/dbus/dbus-userdb-util.c
@@ -38,6 +38,15 @@
  * @{
  */
 
+static DBusGroupInfo *
+_dbus_group_info_ref (DBusGroupInfo *info)
+{
+  _dbus_assert (info->refcount > 0);
+  _dbus_assert (info->refcount < SIZE_MAX);
+  info->refcount++;
+  return info;
+}
+
 /**
  * Checks to see if the UID sent in is the console user
  *
@@ -287,13 +296,14 @@ _dbus_user_database_lookup_group (DBusUserDatabase *db,
           dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
           return NULL;
         }
+      info->refcount = 1;
 
       if (gid != DBUS_GID_UNSET)
         {
           if (!_dbus_group_info_fill_gid (info, gid, error))
             {
               _DBUS_ASSERT_ERROR_IS_SET (error);
-              _dbus_group_info_free_allocated (info);
+              _dbus_group_info_unref (info);
               return NULL;
             }
         }
@@ -302,7 +312,7 @@ _dbus_user_database_lookup_group (DBusUserDatabase *db,
           if (!_dbus_group_info_fill (info, groupname, error))
             {
               _DBUS_ASSERT_ERROR_IS_SET (error);
-              _dbus_group_info_free_allocated (info);
+              _dbus_group_info_unref (info);
               return NULL;
             }
         }
@@ -311,23 +321,35 @@ _dbus_user_database_lookup_group (DBusUserDatabase *db,
       gid = DBUS_GID_UNSET;
       groupname = NULL;
 
-      if (!_dbus_hash_table_insert_uintptr (db->groups, info->gid, info))
+      if (_dbus_hash_table_insert_uintptr (db->groups, info->gid, info))
+        {
+          _dbus_group_info_ref (info);
+        }
+      else
         {
           dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          _dbus_group_info_free_allocated (info);
+          _dbus_group_info_unref (info);
           return NULL;
         }
 
 
-      if (!_dbus_hash_table_insert_string (db->groups_by_name,
-                                           info->groupname,
-                                           info))
+      if (_dbus_hash_table_insert_string (db->groups_by_name,
+                                          info->groupname,
+                                          info))
+        {
+          _dbus_group_info_ref (info);
+        }
+      else
         {
           _dbus_hash_table_remove_uintptr (db->groups, info->gid);
+          _dbus_group_info_unref (info);
           dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
           return NULL;
         }
-      
+
+      /* Release the original reference */
+      _dbus_group_info_unref (info);
+
       /* Return a borrowed reference to the DBusGroupInfo owned by the
        * two hash tables */
       return info;

diff --git a/dbus/dbus-userdb.c b/dbus/dbus-userdb.c
index 223eaae65b759973ccc13bc10025d1c2315a1fd3..10434bbb7c7040718bc36a16ac7b9c7153af041b 100644 (file)
--- a/dbus/dbus-userdb.c
+++ b/dbus/dbus-userdb.c
@@ -35,34 +35,57 @@
  * @{
  */
 
+static DBusUserInfo *
+_dbus_user_info_ref (DBusUserInfo *info)
+{
+  _dbus_assert (info->refcount > 0);
+  _dbus_assert (info->refcount < SIZE_MAX);
+  info->refcount++;
+  return info;
+}
+
 /**
- * Frees the given #DBusUserInfo's members with _dbus_user_info_free()
+ * Decrements the reference count. If it reaches 0,
+ * frees the given #DBusUserInfo's members with _dbus_user_info_free()
  * and also calls dbus_free() on the block itself
  *
  * @param info the info
  */
 void
-_dbus_user_info_free_allocated (DBusUserInfo *info)
+_dbus_user_info_unref (DBusUserInfo *info)
 {
   if (info == NULL) /* hash table will pass NULL */
     return;
 
+  _dbus_assert (info->refcount > 0);
+  _dbus_assert (info->refcount < SIZE_MAX);
+
+  if (--info->refcount > 0)
+    return;
+
   _dbus_user_info_free (info);
   dbus_free (info);
 }
 
 /**
- * Frees the given #DBusGroupInfo's members with _dbus_group_info_free()
+ * Decrements the reference count. If it reaches 0,
+ * frees the given #DBusGroupInfo's members with _dbus_group_info_free()
  * and also calls dbus_free() on the block itself
  *
  * @param info the info
  */
 void
-_dbus_group_info_free_allocated (DBusGroupInfo *info)
+_dbus_group_info_unref (DBusGroupInfo *info)
 {
   if (info == NULL) /* hash table will pass NULL */
     return;
 
+  _dbus_assert (info->refcount > 0);
+  _dbus_assert (info->refcount < SIZE_MAX);
+
+  if (--info->refcount > 0)
+    return;
+
   _dbus_group_info_free (info);
   dbus_free (info);
 }
@@ -170,13 +193,14 @@ _dbus_user_database_lookup (DBusUserDatabase *db,
           dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
           return NULL;
         }
+      info->refcount = 1;
 
       if (uid != DBUS_UID_UNSET)
         {
           if (!_dbus_user_info_fill_uid (info, uid, error))
             {
               _DBUS_ASSERT_ERROR_IS_SET (error);
-              _dbus_user_info_free_allocated (info);
+              _dbus_user_info_unref (info);
               return NULL;
             }
         }
@@ -185,7 +209,7 @@ _dbus_user_database_lookup (DBusUserDatabase *db,
           if (!_dbus_user_info_fill (info, username, error))
             {
               _DBUS_ASSERT_ERROR_IS_SET (error);
-              _dbus_user_info_free_allocated (info);
+              _dbus_user_info_unref (info);
               return NULL;
             }
         }
@@ -195,22 +219,33 @@ _dbus_user_database_lookup (DBusUserDatabase *db,
       username = NULL;
 
       /* insert into hash */
-      if (!_dbus_hash_table_insert_uintptr (db->users, info->uid, info))
+      if (_dbus_hash_table_insert_uintptr (db->users, info->uid, info))
+        {
+          _dbus_user_info_ref (info);
+        }
+      else
         {
           dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          _dbus_user_info_free_allocated (info);
+          _dbus_user_info_unref (info);
           return NULL;
         }
 
-      if (!_dbus_hash_table_insert_string (db->users_by_name,
-                                           info->username,
-                                           info))
+      if (_dbus_hash_table_insert_string (db->users_by_name,
+                                          info->username,
+                                          info))
+        {
+          _dbus_user_info_ref (info);
+        }
+      else
         {
           _dbus_hash_table_remove_uintptr (db->users, info->uid);
           dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
+          _dbus_user_info_unref (info);
           return NULL;
         }
-      
+
+      _dbus_user_info_unref (info);
+
       /* Return a borrowed pointer to the DBusUserInfo owned by the
        * hash tables */
       return info;
@@ -570,24 +605,24 @@ _dbus_user_database_new (void)
   db->refcount = 1;
 
   db->users = _dbus_hash_table_new (DBUS_HASH_UINTPTR,
-                                    NULL, (DBusFreeFunction) _dbus_user_info_free_allocated);
+                                    NULL, (DBusFreeFunction) _dbus_user_info_unref);
   
   if (db->users == NULL)
     goto failed;
 
   db->groups = _dbus_hash_table_new (DBUS_HASH_UINTPTR,
-                                     NULL, (DBusFreeFunction) _dbus_group_info_free_allocated);
+                                     NULL, (DBusFreeFunction) _dbus_group_info_unref);
   
   if (db->groups == NULL)
     goto failed;
 
   db->users_by_name = _dbus_hash_table_new (DBUS_HASH_STRING,
-                                            NULL, NULL);
+                                            NULL, (DBusFreeFunction) _dbus_user_info_unref);
   if (db->users_by_name == NULL)
     goto failed;
   
   db->groups_by_name = _dbus_hash_table_new (DBUS_HASH_STRING,
-                                             NULL, NULL);
+                                             NULL, (DBusFreeFunction) _dbus_group_info_unref);
   if (db->groups_by_name == NULL)
     goto failed;
   

diff --git a/dbus/dbus-userdb.h b/dbus/dbus-userdb.h
index 9e9ed88af5efbb63cc4f1d10f5825e9c83f1c775..b38e3d1834ac151207e3d1aa3a837ce70c0fe369 100644 (file)
--- a/dbus/dbus-userdb.h
+++ b/dbus/dbus-userdb.h
@@ -85,10 +85,10 @@ const DBusGroupInfo* _dbus_user_database_lookup_group (DBusUserDatabase *db,
                                                        dbus_gid_t        gid,
                                                        const DBusString *groupname,
                                                        DBusError        *error);
+
+void           _dbus_user_info_unref            (DBusUserInfo     *info);
 DBUS_PRIVATE_EXPORT
-void           _dbus_user_info_free_allocated   (DBusUserInfo     *info);
-DBUS_PRIVATE_EXPORT
-void           _dbus_group_info_free_allocated  (DBusGroupInfo    *info);
+void           _dbus_group_info_unref           (DBusGroupInfo    *info);
 #endif /* DBUS_USERDB_INCLUDES_PRIVATE */
 
 DBUS_PRIVATE_EXPORT