322 lines
11 KiB
Diff
322 lines
11 KiB
Diff
commit 2b7948ef907669e844b52c4fa2268d6e3162a70c
|
|
Author: Simon McVittie <smcv@collabora.com>
|
|
Date: Tue Jun 30 19:29:06 2020 +0100
|
|
|
|
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>
|
|
|
|
Index: dbus-1.12.2/dbus/dbus-sysdeps-unix.h
|
|
===================================================================
|
|
--- dbus-1.12.2.orig/dbus/dbus-sysdeps-unix.h
|
|
+++ dbus-1.12.2/dbus/dbus-sysdeps-unix.h
|
|
@@ -105,6 +105,7 @@ typedef struct DBusGroupInfo DBusGroupIn
|
|
*/
|
|
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 */
|
|
};
|
|
Index: dbus-1.12.2/dbus/dbus-userdb-util.c
|
|
===================================================================
|
|
--- dbus-1.12.2.orig/dbus/dbus-userdb-util.c
|
|
+++ dbus-1.12.2/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 (DBusUs
|
|
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 (DBusUs
|
|
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 (DBusUs
|
|
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;
|
|
Index: dbus-1.12.2/dbus/dbus-userdb.c
|
|
===================================================================
|
|
--- dbus-1.12.2.orig/dbus/dbus-userdb.c
|
|
+++ dbus-1.12.2/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 (DBusUserData
|
|
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 (DBusUserData
|
|
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 (DBusUserData
|
|
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;
|
|
|
|
Index: dbus-1.12.2/dbus/dbus-userdb.h
|
|
===================================================================
|
|
--- dbus-1.12.2.orig/dbus/dbus-userdb.h
|
|
+++ dbus-1.12.2/dbus/dbus-userdb.h
|
|
@@ -85,10 +85,10 @@ const DBusGroupInfo* _dbus_user_database
|
|
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
|