fix CVE-2020-35512

This commit is contained in:
Yangyang Shen 2021-03-19 20:11:24 +08:00
parent 600d75dece
commit 119925bdfe
3 changed files with 441 additions and 6 deletions

View File

@ -0,0 +1,97 @@
From 6ee66ff7bcc91803111d950512f02651e664f74f Mon Sep 17 00:00:00 2001
From: Simon McVittie <smcv@collabora.com>
Date: Tue, 30 Jun 2020 19:13:17 +0100
Subject: [PATCH] userdb: Make lookups return a const pointer
This makes it more obvious that the returned pointer points to a
struct owned by the userdb, which must not be freed or have its
contents modified, and is only valid to dereference until the next
modification to the userdb's underlying hash tables (which in practice
means until the lock is released, because after that we have no
guarantees about what might be going on in another thread).
Signed-off-by: Simon McVittie <smcv@collabora.com>
---
dbus/dbus-userdb-util.c | 6 ++++--
dbus/dbus-userdb.c | 6 ++++--
dbus/dbus-userdb.h | 10 +++++-----
3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/dbus/dbus-userdb-util.c b/dbus/dbus-userdb-util.c
index 44a1a78..af880ed 100644
--- a/dbus/dbus-userdb-util.c
+++ b/dbus/dbus-userdb-util.c
@@ -240,9 +240,9 @@ _dbus_get_user_id_and_primary_group (const DBusString *username,
* @param gid the group ID or #DBUS_GID_UNSET
* @param groupname group name or #NULL
* @param error error to fill in
- * @returns the entry in the database
+ * @returns the entry in the database (borrowed, do not free)
*/
-DBusGroupInfo*
+const DBusGroupInfo *
_dbus_user_database_lookup_group (DBusUserDatabase *db,
dbus_gid_t gid,
const DBusString *groupname,
@@ -328,6 +328,8 @@ _dbus_user_database_lookup_group (DBusUserDatabase *db,
return NULL;
}
+ /* 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 4c42b7d..223eaae 100644
--- a/dbus/dbus-userdb.c
+++ b/dbus/dbus-userdb.c
@@ -122,9 +122,9 @@ _dbus_is_a_number (const DBusString *str,
* @param uid the user ID or #DBUS_UID_UNSET
* @param username username or #NULL
* @param error error to fill in
- * @returns the entry in the database
+ * @returns the entry in the database (borrowed, do not free)
*/
-DBusUserInfo*
+const DBusUserInfo *
_dbus_user_database_lookup (DBusUserDatabase *db,
dbus_uid_t uid,
const DBusString *username,
@@ -211,6 +211,8 @@ _dbus_user_database_lookup (DBusUserDatabase *db,
return NULL;
}
+ /* Return a borrowed pointer to the DBusUserInfo owned by the
+ * hash tables */
return info;
}
}
diff --git a/dbus/dbus-userdb.h b/dbus/dbus-userdb.h
index 53fc90b..9e9ed88 100644
--- a/dbus/dbus-userdb.h
+++ b/dbus/dbus-userdb.h
@@ -76,15 +76,15 @@ dbus_bool_t _dbus_user_database_get_groupname (DBusUserDatabase *db,
DBusError *error);
DBUS_PRIVATE_EXPORT
-DBusUserInfo* _dbus_user_database_lookup (DBusUserDatabase *db,
+const DBusUserInfo *_dbus_user_database_lookup (DBusUserDatabase *db,
dbus_uid_t uid,
const DBusString *username,
DBusError *error);
DBUS_PRIVATE_EXPORT
-DBusGroupInfo* _dbus_user_database_lookup_group (DBusUserDatabase *db,
- dbus_gid_t gid,
- const DBusString *groupname,
- DBusError *error);
+const DBusGroupInfo* _dbus_user_database_lookup_group (DBusUserDatabase *db,
+ dbus_gid_t gid,
+ const DBusString *groupname,
+ DBusError *error);
DBUS_PRIVATE_EXPORT
void _dbus_user_info_free_allocated (DBusUserInfo *info);
DBUS_PRIVATE_EXPORT
--
2.23.0

View File

@ -0,0 +1,330 @@
From 2b7948ef907669e844b52c4fa2268d6e3162a70c Mon Sep 17 00:00:00 2001
From: Simon McVittie <smcv@collabora.com>
Date: Tue, 30 Jun 2020 19:29:06 +0100
Subject: [PATCH] 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>
---
dbus/dbus-sysdeps-unix.h | 2 ++
dbus/dbus-userdb-util.c | 38 ++++++++++++++++++-----
dbus/dbus-userdb.c | 67 ++++++++++++++++++++++++++++++----------
dbus/dbus-userdb.h | 6 ++--
4 files changed, 86 insertions(+), 27 deletions(-)
diff --git a/dbus/dbus-sysdeps-unix.h b/dbus/dbus-sysdeps-unix.h
index 8d3df2d..830d5cd 100644
--- 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 af880ed..170d233 100644
--- 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 223eaae..10434bb 100644
--- 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 9e9ed88..b38e3d1 100644
--- 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
--
2.23.0

View File

@ -1,19 +1,24 @@
Name: dbus
Epoch: 1
Version: 1.12.16
Release: 15
Release: 16
Summary: System Message Bus
License: AFLv2.1 or GPLv2+
URL: http://www.freedesktop.org/Software/dbus/
Source0: https://dbus.freedesktop.org/releases/dbus/%{name}-%{version}.tar.gz
Source1: 00-start-message-bus.sh
# fix CVE-2020-12049
Patch0000: sysdeps-unix-On-MSG_CTRUNC-close-the-fds-we-did-rece.patch
Patch0001: fdpass-test-Assert-that-we-don-t-leak-file-descripto.patch
Patch0002: Solaris-and-derivatives-do-not-adjust-cmsg_len-on-MS.patch
Patch0001: bugfix-let-systemd-restart-dbus-when-the-it-enters-failed.patch
# fix CVE-2020-12049
Patch6000: sysdeps-unix-On-MSG_CTRUNC-close-the-fds-we-did-rece.patch
Patch6001: fdpass-test-Assert-that-we-don-t-leak-file-descripto.patch
Patch6002: Solaris-and-derivatives-do-not-adjust-cmsg_len-on-MS.patch
# fix cve-2020-35512
Patch6003: backport-userdb-Make-lookups-return-a-const-pointer.patch
Patch6004: backport-userdb-Reference-count-DBusUserInfo-DBusGroupInfo.patch
Patch0010: bugfix-let-systemd-restart-dbus-when-the-it-enters-failed.patch
BuildRequires: systemd-devel expat-devel libselinux-devel audit-libs-devel doxygen xmlto cmake
BuildRequires: autoconf-archive libtool libX11-devel libcap-ng-devel libxslt gdb
@ -220,6 +225,9 @@ make check
%exclude %{_pkgdocdir}/README
%changelog
* Fri Mar 19 2021 shenyangyang <shenyangyang4@huawei.com> - 1:1.12.16-16
- Fix CVE-2020-35512
* Mon Jun 22 2020 shenyangyang <shenyangyang4@huawei.com> - 1:1.12.16-15
- Add more test cases modify for solving CVE-2020-12049