Thank you for opening the review bug. The package is rather small so it should
not take too long for us to review it. We will schedule the review.

I will look into this one, too.

I am through with the review. This D-Bus service is quite more complex than
the other. The operations it performs are okay, so it deals with state files
in /sys and reacts to udev events all related to power management, to no big
surprise.

I am seeing two problems here, however:

a) there is no authentication required to access the D-Bus interface. This
  means just any user with access to the D-Bus system bus can control power
  settings in the system. Even e.g. the 'nobody' user. This should be more
  restricted and typically the polkit authentication framework is used for
  this. Users that own a local session can then perform operations without
  special authentication and others are not allowed to do that, or need to
  enter a root password.
b) the D-Bus method net.hadess.PowerProfiles.HoldProfile allows anybody to
  register a "profile hold". This call accepts arbitrary strings that will be
  entered internally in the power-profiles-daemin into a hash map. There is no
  limit to the string lengths and no limit to the number of profile holds that
  may be active at any time. Therefore this could serve as a denial-of-service
  attack vector.

The issue in b) is demonstrated by this Python script:

```
import dbus

sbus = dbus.SystemBus()
proxy = sbus.get_object('net.hadess.PowerProfiles', '/net/hadess/PowerProfiles')

while True:
        ret = proxy.HoldProfile("performance", "test" * 20000, "test" * 20000, dbus_interface='net.hadess.PowerProfiles')
        if (ret % 100) == 0:
                print(ret, flush=True)
```

This will cause quickly increasing memory usage and finally an OOM.

The implementation of this D-Bus method call should probably also make sure
that the strings passed to it don't contain any malicious characters, since
the strings serve display purposes as it seems.

Are you in contact with upstream to communicate this to them or should I reach
out to them?

Sorry, I am a bit late to follow-up, but I have now opened
https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/issues/47

Matthias, the dev for the upstream bug report has made it private, so I don't know if you can see it (even after signing in to Gitlab). I will update this report when there is a fix available.

Matthias, please have a look at the patched package I have built at <https://build.opensuse.org/package/show/home:badshah400:Staging/power-profiles-daemon>. It includes patches from the upstream merge request <https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/merge_requests/83> that should make the calls more restrictive. Specifically, the patch in question is this:
<https://build.opensuse.org/package/view_file/home:badshah400:Staging/power-profiles-daemon/power-profiles-daemon-polkit-policy.patch?expand=1>.

The upstream bug report <https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/issues/47> is also now public, btw.

(In reply to badshah400@gmail.com from comment #7)
> Matthias, please have a look at the patched package I have built at
> <https://build.opensuse.org/package/show/home:badshah400:Staging/power-profiles-daemon>.

Yes the polkit parts look good so far. You can add it to the devel package.

I am not yet happy with upstream's idea of memory limitation. If they don't
want to make their API call themselves then we can consider adding a small
patch that does this on our side.

So I tried with a follow-up comment in the upstream issue but the upstream dev
is not cooperative on the remaining issues. I suggest you get your devel
package ready and I will have a final look and provide you a small patch that
adds the desired additional safety to the D-Bus method in question.

Thanks Matthias. The devel package is ready with the polkit patches from upstream for your consideration.

Created attachment 852965 [details]
hardening of HoldProfile D-Bus method

There is currently some problem building your packaged introduced through the
"python3-dbusmock" BuildRequires line. It's some conflict in the D-Bus RPMs, I
worked around it by commenting out this BuildRequires.

I tested the polkit changes from upstream and they look all right. Furthermore
in attachment 852965 [details] you find a patch for harden the HoldProfile method.
Please add it to the package as well and reference this review bug in a
comment to clarify its use.

After you added the patch I can submit whitelisting for polkit and D-Bus so
you can continue on to Factory.

Big thanks, Matthias. Your patch atop the updated 0.10.0 version from upstream that implements the polkit changes is now in Base:System. Please feel free to proceed with whitelisting this for Factory at your convenience.

Matthias, please note that there are multiple files that need to be whit-listed now. From the rpmlint log:

> [   23s] power-profiles-daemon.x86_64: E: polkit-user-privilege (Badness: 10000) net.hadess.PowerProfiles.switch-profile (no:no:yes)
> [   23s] power-profiles-daemon.x86_64: E: polkit-user-privilege (Badness: 10000) net.hadess.PowerProfiles.hold-profile (no:no:yes)
> [   23s] The package allows unprivileged users to carry out privileged operations
> [   23s] without root authentication. This could cause security problems if not done
> [   23s] carefully. If the package is intended for inclusion in any SUSE product please
> [   23s] open a bug report to request review of the package by the security team.
> [   23s] Please refer to
> [   23s] https://en.opensuse.org/openSUSE:Package_security_guidelines#audit_bugs for
> [   23s] more information.
> [   23s] 
> [   23s] power-profiles-daemon.x86_64: E: dbus-file-unauthorized (Badness: 10000) /usr/share/dbus-1/system-services/net.hadess.PowerProfiles.service (file digest sha256:7f3edf9bc7e29fa2a24fc6f33e159c1b1f644a872fde031e769993d165f0affb)
> [   23s] power-profiles-daemon.x86_64: E: dbus-file-unauthorized (Badness: 10000) /etc/dbus-1/system.d/net.hadess.PowerProfiles.conf (file digest sha256:1e41a29b7189bb39e3998284af8f0cd9744f7522e6c152c6558447a643a5b60a)

The polkit whitelisting I already submitted to Factory. The D-Bus whitelisting
will have to wait a bit, because there is already an update running.
I can probably make the submission on monday.

This is an autogenerated message for OBS integration:
This bug (1189900) was mentioned in
https://build.opensuse.org/request/show/924174 Factory / polkit-default-privs

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

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

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

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

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

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

The polkit-default-privs whitelisting has already been accepted to Factory. In
the rpmlint whitelisting area a lot of updating is going on as you can see
from comments 17 through 22. If you want you can already submit / update your
package and ask it to be placed into the same Staging project as rpmlint
(currently sr#925253). Then the whitelisting should work as well.

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

The rpmlint whitelisting finally made it into Factory. You should be able to
submit your package now. Closing the bug.

(In reply to Matthias Gerstner from comment #25)
> The rpmlint whitelisting finally made it into Factory. You should be able to
> submit your package now. Closing the bug.

Big thanks, Matthias for your help.