Thank you for opening the audit bug.

This D-Bus service is written in C++ and has about 3.000 lines of code. Should
be a medium sized review. Three polkit actions are also added to the mix.

Due to the christmas season it might take a bit longer for us to complete the
review.

I will have a first look into the package.

The codebsae for this is pretty complex and larger than first expected.
Basically the general-purpose library is connected directly to the D-Bus
system bus. This requires some more time.

I'm making this bug private. I found a local root exploit in this D-Bus
service. And there is likely not just one, because of a bigger design flaw.

I will have to involve upstream and offer an embargo and coordinated
disclosure.

Please keep all information about this private until the security team
publishes the related bug(s).

dnf5 is not yet part of any openSUSE or SUSE product or distribution as far
as I can see. So on our end this only affects the devel project and the
pending submission to Factory.

I have reached out to RedHat security and reported the problems I found. I
suppose this will take a longer time before it becomes public. The impact is
pretty high IMHO.

I will post the vulnerability details I shared with upstream in the next
comment. Please be aware that this information is under embargo, don't share
anything of this before we make this bug public.

1) Introduction
===============

The dnf5daemon-server component offers a collection of D-Bus interfaces to
interact with the dnf5 package manager on the system. An openSUSE community
packager wants to add this component to the openSUSE Tumbleweed distribution.
New D-Bus system services require a review by the SUSE security team. In the
course of this review I found the issues described in this report.

The version of dnf I reviewed for this is 5.1.9 and any source code references
below are based on the corresponding version tag in the upstream Git
repository.

Apart from the local root exploit I also found some other worrying aspects
that are listed in the following sections.

2) D-Bus Interface Design
=========================

The dnf5daemon-server offers a main interface "org.rpm.dnf.v0.SessionManager"
on which clients can create a new session, which results in a new dynamically
allocated D-Bus object being registered on the bus. This session object
provides a set of additional D-Bus interfaces for modifying package manager
configuration, for installing or removing packages or for inspecting metadata
about packages and repositories on the system.

The dnf5daemon-server is running as root and can be autostarted via the D-Bus
system bus if it is not already running. Any other users with access to the
D-Bus system bus may talk to the dnf5daemon-server.

For certain privileged operations the D-Bus service implements Polkit
authentication. Only three operations are protected by Polkit:

- `org.rpm.dnf.v0.rpm.RepoConf.write`
- `org.rpm.dnf.v0.rpm.execute_transaction`
- `org.rpm.dnf.v0.rpm.Repo.confirm_key`

These relate to changing the repository configuration, executing transactions
or importing new trusted signing keys. Transactions are all kinds of changes
introduced through the package manager.

All of these operations require `auth_admin` privileges on Polkit level. The
integration of the Polkit authorization logic is correct as far as I can tell.

The per-session D-Bus interface provided by dnf5daemon-server is rather large
and many calls take additional key/value maps to tune the behaviour of the
package manager. In summary, the libdnf5 library is attached very closely to
the D-Bus system bus via dnf5daemon-server.

3) Local Root Exploit via Configuration Dictionary
==================================================

While the privileged operations mentioned above are correctly protected by
Polkit, there are issues with the D-Bus interface long before Polkit is even
invoked.

The `org.rpm.dnf.v0.SessionManager.open_session` method takes a key/value map
of configuration entries. A sub-entry in this map, placed under the "config"
key, is another key/value map. The configuration values found in it will be
forwarded as configuration overrides to the `libdnf5::Base` configuration. The
spot where this happens is found in `session.cpp:63` [3].

Practically all libdnf5 configuration aspects can be influenced here, as can be
seen in the ConfigMain class in `config_main.hpp`. Already when opening the
session via D-Bus, the libdnf5 will be initialized using these override
configuration values. There is no sanity checking of the content of this
"config" map, which is untrusted data.

There are surely a lot of different ways to exploit this possibility to
influence the libdnf5 configuration. The simplest approach to get full root
privileges I found is to trick the library into loading a plug-in shared
library under control of the unprivileged user.

To do this the "pluginpath" and "pluginconfpath" configuration entries need to
be supplied and need to point to a user-controlled path. There the
unprivileged user can place a configuration file that in turn points towards a
user controlled shared library. The library will then `dlopen()` this user
controlled shared library which will lead to full code execution in the
context of the root user.

Attached to this e-mail you will find a tarball containing a proof of concept
for exploiting this vulnerability. I successfully tested this exploit also on
Fedora 39 using dnf5daemon-server version 5.1.10. The only precondition for
this exploit is that the dnf5daemon-server package is installed on the system.
Any local user, even `nobody` can obtain root privileges this way.

For fixing this I suggest to enforce a whitelist of allowed configuration
parameters that are allowed to be overridden. Only a very small subset of
values should be allowed for unprivileged clients.

4) No Limit on Number of Open Sessions / Bad Session Close Behaviour
====================================================================

There is no limit on how many sessions D-Bus clients may create using the
`open_session()` D-Bus method. In my tests I was able to quickly create about
4,500 sessions and keep them open. For each session a thread is created in
dnf5daemon-server. This spends a couple of 100 megabytes of memory in the
process. Further connections will become impossible, likely because no more
threads can be spawned by the D-Bus service.

In some cases I even managed to cause the D-Bus service to `abort()` as a
result to hitting resource limits. If the service continues running and the
client disconnects then the cleanup code found in
`SessionManager::on_name_owner_changed()` runs for each session that has been
created. Each Session holds a `ThreadsManager`, where the thread associated
with each session originates from. It is a thread that is busy-waiting to join
other threads, the code for this is found in `threads_manager.cpp:33`. Since
there is a sleep of one second in this thread's loop it takes about ~4,500
seconds for all the threads from the 4,500 sessions to be joined one by one.
The service will be unreachable for more than an hour.

To fix this I suggest to limit the number of sessions for each unprivileged
user in the system to a sensible value. Also the busy-wait loop in
`ThreadsManager` seems ill-devised. Maybe using a condition variable to inform
the cleanup thread of new work would be more efficient. Having a dedicated
`ThreadsManager` and join-thread for each session seems a bit overkill, too.

This issue is a local denial-of-service security issue and I suggest assigning
a dedicated CVE for it, as well.

5) Untrusted `locale` Setting for each Session
==============================================

Another part of the per-session setup is the use of an untrusted `locale`
setting in `session.cpp:54`. This string is passed to the C library's
`newlocale()` function in `threads_manager.cpp:92`. Luckily, when glibc is
used, then glibc already implements a very careful locale lookup algorithm
that prevents that arbitrary paths are accessed if crafted `locale` strings
are passed to it. For this reason, when glibc is used, there are no security
consequences. This might change if a different C library is used.

If anything bad could happen, apart from file system issues, when exotic
locales are selected for a thread running in dnf5daemon-server is another
question that I did not investigate more closely.

For hardening purposes I suggest that dnf5daemon-server performs a sanity
check of the `locale` string to prevent a string that contains e.g. a slash
character `/` to be used.

6) dnfdaemon-client Demands Full Root
=====================================

Although the D-Bus service implements Polkit for privileged operations, the
dnf5daemon-client refuses to perform privileged operations for non-root users.
For example:

    user$ dnf5daemon-client distro-sync
    This command has to be run with superuser privileges (under the root user on most systems).

The code for this is found in various sub-command implementations:

    $ grep -r 'throw UnprivilegedUserError' -C 1
    dnf5daemon-client/commands/downgrade/downgrade.cpp-    if (!libdnf5::utils::am_i_root()) {
    dnf5daemon-client/commands/downgrade/downgrade.cpp:        throw UnprivilegedUserError();
    dnf5daemon-client/commands/downgrade/downgrade.cpp-    }
    --
    dnf5daemon-client/commands/distro-sync/distro-sync.cpp-    if (!libdnf5::utils::am_i_root()) {
    dnf5daemon-client/commands/distro-sync/distro-sync.cpp:        throw UnprivilegedUserError();
    dnf5daemon-client/commands/distro-sync/distro-sync.cpp-    }
    --
    dnf5daemon-client/commands/install/install.cpp-    if (!libdnf5::utils::am_i_root()) {
    dnf5daemon-client/commands/install/install.cpp:        throw UnprivilegedUserError();
    dnf5daemon-client/commands/install/install.cpp-    }
    --
    dnf5daemon-client/commands/remove/remove.cpp-    if (!libdnf5::utils::am_i_root()) {
    dnf5daemon-client/commands/remove/remove.cpp:        throw UnprivilegedUserError();
    dnf5daemon-client/commands/remove/remove.cpp-    }
    --
    dnf5daemon-client/commands/upgrade/upgrade.cpp-    if (!libdnf5::utils::am_i_root()) {
    dnf5daemon-client/commands/upgrade/upgrade.cpp:        throw UnprivilegedUserError();
    dnf5daemon-client/commands/upgrade/upgrade.cpp-    }
    --
    dnf5daemon-client/commands/reinstall/reinstall.cpp-    if (!libdnf5::utils::am_i_root()) {
    dnf5daemon-client/commands/reinstall/reinstall.cpp:        throw UnprivilegedUserError();
    dnf5daemon-client/commands/reinstall/reinstall.cpp-    }

It doesn't make sense that the client forces users to run as root (and thereby
increase the attack by running also the client code as root) when the service
could authenticate the user via Polkit.

I suggest to drop this root-check code in the client and rely on the
authentication logic in the service side code. If authentication fails then
the client code can still hint at the possibility to run the program as root.

7) General Review Summary
=========================

In summary my impression is that the libdnf5 library is too closely connected
to the D-Bus system bus. The library itself is unaware of the fact that it is
running with partially untrusted input. The dnf5daemon-server code needs to
carefully filter untrusted input before it is passed to the generic library
code.

8) References
=============

[1]: https://github.com/rpm-software-management/dnf5/tree/5.1.9
[2]: https://www.openwall.com/lists/oss-security
[3]: https://github.com/rpm-software-management/dnf5/blob/d681cbf93a42a4bb3cc646661031eb44c5e3d56a/dnf5daemon-server/session.cpp#L63

Can I temporarily disable dnf5daemon in the dnf5 package while we wait for these issues to be resolved to unblock my initial SR?

(In reply to ngompa13@gmail.com from comment #9)
> Can I temporarily disable dnf5daemon in the dnf5 package while we wait for these issues to be resolved to unblock my initial SR?

Sure, you just need to drop the configuration files mentioned in the
dbus-file-unauthorized rpmlint errors.

I have little news from upstream. There seem to be some communication problems
between RH security and Fedora people. Suddenly RH security no longer feels
responsible for Fedora only problems.

I've refreshed the submit request with dnf5-5.1.13, are we in good shape with this version?

https://build.opensuse.org/request/show/1149988

(In reply to ngompa13@gmail.com from comment #14)
> I've refreshed the submit request with dnf5-5.1.13, are we in good shape with this version?
> 
> https://build.opensuse.org/request/show/1149988

The issue is officially still under embargo and I did not get any update from
upstream.

Upstream commit 6e51bf2f0d585ab6 contains some logic to address at least part
of the findings:

    dnfdaemon: Explicitly specify allowed config overrides
    
    Limit main config options overrides for dnfdaemon session only to
    those explicitely allowed.

It seems nothing happened yet regarding the other issues I reported. Before I
hear back from upstream and CVEs and patches are fully published I cannot give
a go for this component.

But I can ask upstream what their progress is, pointing to this commit, that
also partially published part of the finding in some way.

@ngompa: I only now see that I made a private comment on 2024-01-18 that you
couldn't even read. Sorry about that.

Of course you can disable the dnf5daemon. You can even include it but you need
to drop the D-Bus configuration files that rpmlint complains about. When you
do that please make sure not to mention anything about a vulnerability in the
package and submit requests.

RedHat security communicated the following CVEs to us:

CVE-2024-1929 -> Local Root Exploit via Configuration Dictionary
CVE-2024-1930 -> No Limit on Number of Open Sessions / Bad Session Close Behaviour

These two public pull requests address them:

https://github.com/rpm-software-management/dnf5/pull/1201
https://github.com/rpm-software-management/dnf5/pull/1236

I did not verify yet whether this actually fixes the issues.

It seems we now have a dialog and attempt to determine a publication date for
the complete report. I have also asked for further details e.g. which version
is supposed to contain all fixes and whether further fixes are expected.

Once we have a release candidate I will look into the package's security once
more.

Publication on the Red Hat Product Security side happens today. 

I already published the report on our blog:

https://security.opensuse.org/2024/03/04/dnf5daemon-server-local-root.html

I will send an email to oss-security soon as well.

Making the bug public as well.

When you package the version that contains the both commits mentioned in comment 20 then we can basically go ahead with whitelisting. Even if a couple of shaky parts remain. I will also have to check up on the whitelisted config entries if they're really harmless before whitelisting the patched version.

I am waiting for your go to have a final look into the new packaging and for
whitelisting the D-Bus components.

It looks like everything is included in dnf5-5.1.14, so I'm rebasing to that now.

Rebased and submission made: https://build.opensuse.org/request/show/1156444

Thanks, I'll look into the configuration whitelist's safety and if all is well
I will whitelist the D-Bus interface.

I have bad news. Even with the patch the service remains susceptible to a local DoS and maybe an information leak and maybe even still privilege escalation.

In the whitelist of configuration items a "reposdir" can still be specified by the unprivileged user. The privileged daemon will open this directory and process all *.repo files in there. It follows symlinks. It performs a racy stat()/openat() on all the *.repo files which allows to trick it into opening a FIFO or a device file. We can cause the daemon to open e.g. /etc/shadow and try to parse it as a .repo configuration file. If parse errors are logged into the unprivileged context then this is an information leak.

If the unprivileged user places a regular test.repo file in this directory then it can set all of these repository configuration variables:

"name"
"enabled"
"cachedir"
"baseurl"
"mirrorlist"
"metalink"
"type"
"mediaid"
"gpgkey"
"excludepkgs"
"exclude"
"includepkgs"
"fastestmirror"
"proxy_username"
"proxy_password"
"username"
"password"
"protected_packages"
"gpgcheck"
"repo_gpgcheck"
"enablegroups"
"retries"
"bandwidth"
"minrate"
"ip_resolve"
"throttle"
"timeout"
"max_parallel_downloads"
"metadata_expire"
"cost"
"priority"
"module_hotfixes"
"sslcacert"
"sslverify"
"sslclientcert"
"sslclientkey"
"proxy_sslcacert"
"proxy_sslverify"
"proxy_sslclientcert"
"proxy_sslclientkey"
"deltarpm"
"deltarpm_percentage"
"skip_if_unavailable"
"enabled_metadata"
"user_agent"
"countme"

If these configuration items are used before an admin password is asked for then it is likely that there are more issues through these configuration settings. I'm not sure about that at the moment due to the complexity of the code.

Upstream did not share the patch with us and did not ask for a verification of the patch. Otherwise the publication would not have happened in this form.

I will have to contact Red Hat Security again to discuss with them on how to continue with this.

RedHat security created a new mail thread involving the dnf5 developer team
directly and forwarded my follow-up report to it. For the moment the
additional information is private.

I'm sorry that this is taking so long, but there is little we can do to
accelerate it at the moment.

@Neal, we have heard back from RH about this followup issue.

They have assigned CVE-2024-2746 with a tentative CRD of 2024-04-05.

Can we hold https://build.opensuse.org/request/show/1156444 until the fix is released upstream?

If the request is merged before the fix is released, we will need to open a VUL-0 for the dnf5 Factory package and setup reactive tracking for the CVE.

Yes, we can keep holding it. Please request that they make a new release immediately after the necessary fixes are merged. That will simplify things for me when I supersede that SR with a new one.

Upstream shared the bugfix with us. It is just a one-liner to remove the
"repodir" setting from the whitelist of configuration variables. I forwarded
your request that they publish a new release with this. But I expect this to
happen anyway.

I don't have a definite publication date yet, I suggested either before
2024-03-29 or after 2024-04-01, due to the easter holidays that are coming up
in various countries.

Upstream stated 2024-04-02 as release date.

There was another miscommunication. Release date is now communicated as April
3rd.

Upstream released version 5.1.17 today which contains the bugfix.

We will publish everything during the course of the day. You can then submit
the new version release and we can finally whitelist the D-Bus and Polkit
components.

My final report about the follow-up issue is now public here:

https://security.opensuse.org/2024/04/03/dnf5daemon-resposdir-followup.html

I will now send out the same report to oss-security. Publication is thus done.

I've made a new submission with the latest dnf5 version: https://build.opensuse.org/request/show/1165011

There is one more thing. The package installs the D-Bus service configuration
into /etc/dbus-1/system.d/org.rpm.dnf.v0.conf. These should go into
/usr/share/dbus-1/system.d instead. Can you change that in the packaging?

That's taken care of now in the latest SR: https://build.opensuse.org/request/show/1165750

I've also submitted a pull request upstream: https://github.com/rpm-software-management/dnf5/pull/1384

This is an autogenerated message for OBS integration:
This bug (1218327) was mentioned in
https://build.opensuse.org/request/show/1166154 Factory / rpmlint

This is an autogenerated message for OBS integration:
This bug (1218327) was mentioned in
https://build.opensuse.org/request/show/1168340 Factory / rpmlint

Circumstances really don't mean well with this dnf5 dbus daemon. Even the
rpmlint submission took two weeks to reach Factory. But now it is through.
Closing this bug as fixed.

I still have this error in my submission:

[  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.rpm.Repo.conf_write (auth_admin:auth_admin:auth_admin_keep)
[  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.rpm.execute_transaction (auth_admin:auth_admin:auth_admin_keep)
[  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.rpm.Repo.confirm_key (auth_admin:auth_admin:auth_admin)
[  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.base.Config.override (auth_admin:auth_admin:auth_admin)
[  439s] The polkit action is not listed in the polkit-default-privs profiles which
[  439s] makes it harder for admins to find. Furthermore improper polkit authorization
[  439s] checks can easily introduce security issues. If the package is intended for
[  439s] inclusion in any SUSE product please open a bug report to request review of
[  439s] the package by the security team. Please refer to
[  439s] https://en.opensuse.org/openSUSE:Package_security_guidelines#audit_bugs for
[  439s] more information.

(In reply to ngompa13@gmail.com from comment #43)
> I still have this error in my submission:
> 
> [  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.rpm.Repo.conf_write (auth_admin:auth_admin:auth_admin_keep)
> [  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.rpm.execute_transaction (auth_admin:auth_admin:auth_admin_keep)
> [  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.rpm.Repo.confirm_key (auth_admin:auth_admin:auth_admin)
> [  439s] dnf5daemon-server.x86_64: E: polkit-untracked-privilege (Badness: 10000) org.rpm.dnf.v0.base.Config.override (auth_admin:auth_admin:auth_admin)

Oh, I forgot the Polkit parts. Sorry about that. Will add it right now.