Please attach the DBUS config thats going to be used.
Also the polkit config, if any.

Created attachment 751245 [details]
dbus-service

Please review.

(In reply to Hillwood Yang from comment #2)
> Created attachment 751245 [details]
> dbus-service
> 
> Please review.

Thats not the DBUS config. Thats the .services files used for
DBUS activation.

Created attachment 751270 [details]
dbus-config

Sorry, it's a mistake. I think this time is right.

Sorry for the delay. We are currently rescheduling all our security reviews.
It will take some more time until we can work on your request.

I will be working on this now.

I am through with the code review. Once again sorry for the long delay.

Sadly I cannot whitelist these D-Bus services and polkit action. The quality
of the implementation is not good. Here are the details. deepin-api ships
three D-Bus system services:

A) com.deepin.api.Device.conf /usr/lib/deepin-api/device

This service is allows *anybody* with access to the D-Bus system service to
run /usr/sbin/rfkill with arbitrary arguments. In the code we can see the
following comment:

> device/device.go:61:	// TODO need polkit authentication

So they are aware that an authentication part is missing here. It would be
okay if polkit was used and only locally logged in users would be able to call
it. Even then I would like to see the argument limited to a whitelist of
well-known arguments.

B) com.deepin.api.SoundThemePlayer.conf /usr/lib/deepin-api/sound-theme-player

This service allows to play sound files via ALSA on one of the available ALSA
devices. *Any* user can pass arbitrary files to the service and it will try
to read the file in as an audio file and play it. Doing this from within a
root service is asking for trouble. I don't see a reason why this couldn't be
done from within the unprivileged user session. And if it really needs to be a
system service then the system service needs to be run as a non-root user.

While the service supposedly only looks into a couple of system directories
for the files like in /usr/share/sounds/..., we can trick it by passing
relative path components like so:

> dbus-send --system --print-reply --dest=com.deepin.api.SoundThemePlayer \
>    /com/deepin/api/SoundThemePlayer com.deepin.api.SoundThemePlayer.Play \
>    string:goodtheme string:../../../../../home/mgerstner/test string:alsa

This allows to specify files within user control like e.g. a very big file, a
specially constructed file that triggers a buffer overflow or even a special
device file like a FIFO which will DoS the system service.

C) com.deepin.api.LocaleHelper /usr/lib/deepin-api/locale-helper

This service is supposed to allow selection and generation of locales in the
system. It relies on the locale-gen program and the /etc/locale.gen
configuration file. As far as I can see these aren't even available on
Tumbleweed so I doubt this will work at all.

This is the only service that uses polkit authentication. This is where the
rpmlint error for polkit action com.deepin.api.locale-helper.manage-locale is
coming from. However the implementation uses the "unix process authentication
subject" from polkit in locale-helper/ifc.go/checkAuth. Only the PID of the
requesting process is passed to the polkitd daemon. This is vulnerable to
CVE-2013-4288. Instead the implementation should use the D-Bus Bus Name
authentication subject. Upstream commit 2dccc331a099d298285b05a214a8203cb05a41a4
seems to improve a bit on the situation but I couldn't test it, because it
requires new Go dependencies to apply this patch.

There is a further bad practice in locale-helper/main.go: in
doGenLocaleWithParam() it calls ("/bin/sh", "-c", cmd) where cmd is a user
supplied parameter. This allows injection of special shell characters that can
lead to code execution or other unexpected results.

--

While this all sounds bad I think it is not impossible to fix it: For A) I
recommend implementing a polkit authentication check based on the System Bus
Name Authentication Subject. For B) the root privileges should not be required
at all. For C) the system bus name subject and use of a safe call to
locale-gen is required.

Also the question remains if the locale-gen stuff is working at all on
openSUSE.

I am reassigning this bug to you. You can close it or keep it open if you want
to improve on the issues.

Thank you.

Hi, I'm dev from deepin, we'll look into the security issues and hopefully release a fixed version ASAP, thank you all :)

(In reply to Matthias Gerstner from comment #7)

> A) com.deepin.api.Device.conf /usr/lib/deepin-api/device
> 
> This service is allows *anybody* with access to the D-Bus system service to
> run /usr/sbin/rfkill with arbitrary arguments. In the code we can see the
> following comment:
> 
> > device/device.go:61:	// TODO need polkit authentication
> 
> So they are aware that an authentication part is missing here. It would be
> okay if polkit was used and only locally logged in users would be able to
> call
> it. Even then I would like to see the argument limited to a whitelist of
> well-known arguments.
> 

We have a fix for this: https://cr.deepin.io/#/c/dde/dde-api/+/37335/.

- Changed the API since only dde-daemon is using it, so one can only use the API to query or change the block status of bluetooth devices.
- Restricted the caller that only the ones that are in the sudo group can use that API.

(In reply to Matthias Gerstner from comment #7)

> B) com.deepin.api.SoundThemePlayer.conf
> /usr/lib/deepin-api/sound-theme-player
> 
> This service allows to play sound files via ALSA on one of the available ALSA
> devices. *Any* user can pass arbitrary files to the service and it will try
> to read the file in as an audio file and play it. Doing this from within a
> root service is asking for trouble. I don't see a reason why this couldn't be
> done from within the unprivileged user session. And if it really needs to be
> a
> system service then the system service needs to be run as a non-root user.
> 

The reason why it should be a system wide service is that we want sound effects playing along with the login/logout/shutdown process, if we use a session wide service the sound effects may be interrupted or cut off by the session manager(system).

But as you have pointed, the system service doesn't need to be run as root, so we have changed the service configuration to user a unpriviledged deepin-sound-player user.

> While the service supposedly only looks into a couple of system directories
> for the files like in /usr/share/sounds/..., we can trick it by passing
> relative path components like so:
> 
> > dbus-send --system --print-reply --dest=com.deepin.api.SoundThemePlayer \
> >    /com/deepin/api/SoundThemePlayer com.deepin.api.SoundThemePlayer.Play \
> >    string:goodtheme string:../../../../../home/mgerstner/test string:alsa
> 
> This allows to specify files within user control like e.g. a very big file, a
> specially constructed file that triggers a buffer overflow or even a special
> device file like a FIFO which will DoS the system service.
> 

The fix for this part is at https://cr.deepin.io/#/c/go-lib/+/37362/ .

> C) com.deepin.api.LocaleHelper /usr/lib/deepin-api/locale-helper
> 
> This service is supposed to allow selection and generation of locales in the
> system. It relies on the locale-gen program and the /etc/locale.gen
> configuration file. As far as I can see these aren't even available on
> Tumbleweed so I doubt this will work at all.
> 

I'm not suse user, so please help us :)

> This is the only service that uses polkit authentication. This is where the
> rpmlint error for polkit action com.deepin.api.locale-helper.manage-locale is
> coming from. However the implementation uses the "unix process authentication
> subject" from polkit in locale-helper/ifc.go/checkAuth. Only the PID of the
> requesting process is passed to the polkitd daemon. This is vulnerable to
> CVE-2013-4288. Instead the implementation should use the D-Bus Bus Name
> authentication subject. Upstream commit
> 2dccc331a099d298285b05a214a8203cb05a41a4
> seems to improve a bit on the situation but I couldn't test it, because it
> requires new Go dependencies to apply this patch.
> 
> There is a further bad practice in locale-helper/main.go: in
> doGenLocaleWithParam() it calls ("/bin/sh", "-c", cmd) where cmd is a user
> supplied parameter. This allows injection of special shell characters that
> can
> lead to code execution or other unexpected results.
> 

We have restricted the search path of locale-gen: https://cr.deepin.io/#/c/dde/dde-api/+/37356/.
And actually we have that parameter validated before using it at https://github.com/linuxdeepin/dde-api/blob/master/locale-helper/ifc.go#L72.

> --
> 
> While this all sounds bad I think it is not impossible to fix it: For A) I
> recommend implementing a polkit authentication check based on the System Bus
> Name Authentication Subject. For B) the root privileges should not be
> required
> at all. For C) the system bus name subject and use of a safe call to
> locale-gen is required.
> 

Thanks for your suggestion on those issues !

> Also the question remains if the locale-gen stuff is working at all on
> openSUSE.
> 
> I am reassigning this bug to you. You can close it or keep it open if you
> want
> to improve on the issues.
> 
> Thank you.

(In reply to mr.asianwang@gmail.com from comment #9)

Hello and thank you for working on these issues!

> (In reply to Matthias Gerstner from comment #7)
> > > device/device.go:61:	// TODO need polkit authentication
> 
> We have a fix for this: https://cr.deepin.io/#/c/dde/dde-api/+/37335/.
> 
> - Changed the API since only dde-daemon is using it, so one can only use the API to query or change the block status of bluetooth devices.
> - Restricted the caller that only the ones that are in the sudo group can use that API.

This looks good on first sight. Thanks.

(In reply to mr.asianwang@gmail.com from comment #10)

> (In reply to Matthias Gerstner from comment #7)
> 
> > B) com.deepin.api.SoundThemePlayer.conf
> > [...]
> > This service allows to play sound files via ALSA on one of the available ALSA
> > devices.
> 
> The reason why it should be a system wide service is that we want sound effects playing along with the login/logout/shutdown process, if we use a session wide service the sound effects may be interrupted or cut off by the session manager(system).
> 
> But as you have pointed, the system service doesn't need to be run as root, so we have changed the service configuration to user a unpriviledged deepin-sound-player user.

I understand. An unprivileged user is the best solution then, I think.

> > While the service supposedly only looks into a couple of system directories
> > for the files like in /usr/share/sounds/..., we can trick it by passing
> > relative path components like so:
> > [...]
> 
> The fix for this part is at https://cr.deepin.io/#/c/go-lib/+/37362/ .

This also looks good to me.

> > C) com.deepin.api.LocaleHelper /usr/lib/deepin-api/locale-helper
> > 
> > This service is supposed to allow selection and generation of locales in the
> > system. It relies on the locale-gen program and the /etc/locale.gen
> > configuration file. As far as I can see these aren't even available on
> > Tumbleweed so I doubt this will work at all.
> > 
> 
> I'm not suse user, so please help us :)

It's also not my area of expertise, I'm just concerned with security ;-)

It looks like all locales are pregenerated in /usr/lib/locale. openSUSE ships
'localedef' for generating locales. Configuration is in
/etc/sysconfig/language. There is some official documentation:

https://doc.opensuse.org/documentation/leap/reference/html/book.opensuse.reference/cha.suse.html#sec.suse.l10n

Then there is also systemd which ships localectl but I don't know if both
approaches work well together.

The package maintainer hillwoodroc@gmail.com should care about integration
with openSUSE, maybe he can provide more help.

> > There is a further bad practice in locale-helper/main.go: in
> > doGenLocaleWithParam() it calls ("/bin/sh", "-c", cmd) where cmd is a user
> > supplied parameter. This allows injection of special shell characters that
> > can
> > lead to code execution or other unexpected results.
> > 
> 
> We have restricted the search path of locale-gen: https://cr.deepin.io/#/c/dde/dde-api/+/37356/.
> And actually we have that parameter validated before using it at https://github.com/linuxdeepin/dde-api/blob/master/locale-helper/ifc.go#L72.

Yes this looks better!

Created attachment 803749 [details]
dbus-services-3.18.3

Upstream have updated deepin-api, please check.
https://build.opensuse.org/package/show/X11:Deepin/deepin-api

Created attachment 803778 [details]
Full dbus profiles

I have looked at the new version of deepin-api. I have checked the issues A),
B) and C) that I listed in comment 7. The result is as follows:

A) rfkill is now protected by Polkit authentication. But the
`checkAuthorization()` method is again using the caller's PID for
authentication which is not secure. The D-Bus "system bus name" subject needs
to be used for authentication.

B) SoundThemePlayer.service now specified a user "deepin-sound-player" that
the service should run as. Also the sound files are now safely looked up and
'/' and '.' and '..' are rejected in theme names and file paths.

The deepin-api.spec doesn't create the "deepin-sound-player" user, however.
This still needs to be adjusted, otherwise the D-Bus service can't start with
this error message:

`Unknown username "deepin-sound-player" in message bus configuration file`

C) Just like in A) the `checkAuth()` method is still using the caller's PID
for authentication.

So B) is a packaging issue and A) and C) are still upstream issues. When this
is fixed then we can whitelist everything.

Will let my colleagues give it a look soon, thanks :)

Hi, Matthias

Why checking the caller PID is insecure if I may ask? since we also checked the start-time of the process. My colleague submitted a PR against this https://github.com/linuxdeepin/dde-api/pull/12/files, please have a look at that PR.

(In reply to mr.asianwang@gmail.com from comment #18)
> Hi, Matthias

Hello!

> Why checking the caller PID is insecure if I may ask? since we also checked
> the start-time of the process.

What do you mean by "checked"? In the code I only find the following lines:

```
subject.SetDetail("start-time", uint64(0))
```

This means polkit itself will determine the start time based on the specified
PID. Polkit performs a lot of logic to try to avoid the involved race
condition as described by CVE-2013-4288. Just recently another attack vector
was found that can fool this handling of the process start time [1],
CVE-2019-6133. More of that kind may be existing.

You can see in the polkit upstream code that the check they implement is still
shaky by the name alone [2], `polkit_unix_process_get_racy_uid__`.

[1]: https://gitlab.freedesktop.org/polkit/polkit/issues/75
[2]: https://gitlab.freedesktop.org/polkit/polkit/blob/2cb40c4d5feeaa09325522bd7d97910f1b59e379/src/polkit/polkitunixprocess.c#L182

So for these reasons it is way better to use the D-Bus sender subject that is
based on the open UNIX domain socket and performs the decision on the kernel
side.

> My colleague submitted a PR against this
> https://github.com/linuxdeepin/dde-api/pull/12/files, please have a look at
> that PR.

Okay I looked at it and it seems to be the right approach!

Thanks, Matthias. Lessons learned.

PR to fix dde-daemon: https://github.com/linuxdeepin/dde-daemon/pull/104/files.

(In reply to mr.asianwang@gmail.com from comment #20)
> Thanks, Matthias. Lessons learned.

Do you know of a security contact at deepin? The overall security is not in
good shape and there are more issues I found in bug 1130388 (deepin-clone) and
bug 1134131 (deepin-file-manager). There are serious issues among them and
they would need following a security protocol to have them fixed quickly and
to inform users and distributors about the issues (e.g. assigning CVEs).

There has been no response from the packager or from upstream for a long time.
Closing this and related deepin bugs.

SUSE-SU-2021:0437-1: An update that solves 26 vulnerabilities and has 16 fixes is now available.

Category: security (important)
Bug References: 1070943,1121826,1121872,1157298,1168952,1173942,1176395,1176485,1177411,1178123,1178182,1178589,1178622,1178886,1179107,1179140,1179141,1179204,1179419,1179508,1179509,1179601,1179616,1179663,1179666,1179745,1179877,1179960,1179961,1180008,1180027,1180028,1180029,1180030,1180031,1180032,1180052,1180086,1180559,1180562,1181349,969755
CVE References: CVE-2019-19063,CVE-2019-20934,CVE-2019-6133,CVE-2020-0444,CVE-2020-0465,CVE-2020-0466,CVE-2020-11668,CVE-2020-15436,CVE-2020-15437,CVE-2020-25211,CVE-2020-25285,CVE-2020-25668,CVE-2020-25669,CVE-2020-27068,CVE-2020-27673,CVE-2020-27777,CVE-2020-27786,CVE-2020-27825,CVE-2020-28915,CVE-2020-28974,CVE-2020-29568,CVE-2020-29569,CVE-2020-29660,CVE-2020-29661,CVE-2020-36158,CVE-2021-3347
JIRA References: 
Sources used:
SUSE OpenStack Cloud 7 (src):    kernel-default-4.4.121-92.149.1, kernel-source-4.4.121-92.149.1, kernel-syms-4.4.121-92.149.1, kgraft-patch-SLE12-SP2_Update_39-1-3.3.1
SUSE Linux Enterprise Server for SAP 12-SP2 (src):    kernel-default-4.4.121-92.149.1, kernel-source-4.4.121-92.149.1, kernel-syms-4.4.121-92.149.1, kgraft-patch-SLE12-SP2_Update_39-1-3.3.1
SUSE Linux Enterprise Server 12-SP2-LTSS (src):    kernel-default-4.4.121-92.149.1, kernel-source-4.4.121-92.149.1, kernel-syms-4.4.121-92.149.1, kgraft-patch-SLE12-SP2_Update_39-1-3.3.1
SUSE Linux Enterprise Server 12-SP2-BCL (src):    kernel-default-4.4.121-92.149.1, kernel-source-4.4.121-92.149.1, kernel-syms-4.4.121-92.149.1
SUSE Linux Enterprise High Availability 12-SP2 (src):    kernel-default-4.4.121-92.149.1

NOTE: This line indicates an update has been released for the listed product(s). At times this might be only a partial fix. If you have questions please reach out to maintenance coordination.

The uptream will deal with this bug.

(In reply to Hillwood Yang from comment #24)
> The uptream will deal with this bug.

Assign to justforlxz@gmail.com

Sorry for taking so long to solve these problems.

A) b64999a96c1fba16c9645ea2ae668a4225ca7964 add check authorization
B) fix in go-lib 233385b7d3c6fff4893340f1e195d79bed865b4b
C) remove /bin/sh in 18d32014e656adc38b7edd94ded5d29c1de8443e

Okay I will have another look at everything. @hillwoodroc: are these changes
also contained in the current packaging? Did you fix the username problem
described in comment 16?

(In reply to Matthias Gerstner from comment #27)
> Okay I will have another look at everything. @hillwoodroc: are these changes
> also contained in the current packaging? Did you fix the username problem
> described in comment 16?

No. I will create the user account in spec for deepin-api.spec after the upstream fixes this bug. I drop all dbus and polkit profiles by default, so I have not done this work.

(In reply to Matthias Gerstner from comment #27)
> Okay I will have another look at everything. @hillwoodroc: are these changes
> also contained in the current packaging? Did you fix the username problem
> described in comment 16?

I think this config in %pre macro should fix the user name problem. I will add it in spec but not enable it.

%pre
# getent group deepin-sound-player >/dev/null || %{_sbindir}/groupadd --system deepin-sound-player
# getent passwd deepin-sound-player >/dev/null || %{_sbindir}/useradd --system -c "deepin-sound-player User" \
#         -d %{_localstatedir}/deepin-sound-player -m -g deepin-sound-player -s %{_sbindir}/nologin \
#         -G audio deepin-sound-player

It looks like the upstream fixes should be sufficient now. Please adjust your
packaging and remove these strange deepin-api-polkit and deepin-api-dbus
sub-packages.

The scripts "deepin-api-polkit-installer.in" and
"deepin-api-dbus-installer.in" use fixed directories in /tmp as root and
probably allow local root exploits (!).

Once the package is in good shape again I can submit a whitelisting for D-Bus
and polkit.

(In reply to Matthias Gerstner from comment #30)
> It looks like the upstream fixes should be sufficient now. Please adjust your
> packaging and remove these strange deepin-api-polkit and deepin-api-dbus
> sub-packages.
> 
> The scripts "deepin-api-polkit-installer.in" and
> "deepin-api-dbus-installer.in" use fixed directories in /tmp as root and
> probably allow local root exploits (!).
> 
> Once the package is in good shape again I can submit a whitelisting for D-Bus
> and polkit.

The package is ready. Please check it: https://build.opensuse.org/package/show/X11:Deepin:Factory/deepin-api

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

The D-Bus whitelisting is now also in Factory. Closing this bug.

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

(In reply to Matthias Gerstner from comment #34)
> The D-Bus whitelisting is now also in Factory. Closing this bug.

Polkit actions still not be whitelisted.

[  167s] deepin-api.x86_64: E: polkit-unauthorized-privilege (Badness: 10) com.deepin.api.locale-helper.manage-locale (no:no:yes)
[  167s] The package allows unprivileged users to carry out privileged
[  167s] operations without authentication. This could cause security         problems
[  167s] if not done carefully. If the package is intended for         inclusion in any
[  167s] SUSE product please open a bug report to request         review of the package
[  167s] by the security team. Please refer to
[  167s] https://en.opensuse.org/openSUSE:Package_security_guidelines#audit_bugs
[  167s] for more information.
[  167s] 
[  167s] deepin-api.x86_64: E: polkit-untracked-privilege (Badness: 10) com.deepin.api.device.unblock-bluetooth-devices (no:no:auth_admin_keep)
[  167s] The privilege is not listed in /etc/polkit-default-privs.*         which makes
[  167s] it harder for admins to find. Furthermore polkit         authorization checks
[  167s] can easily introduce security issues. If the         package is intended for
[  167s] inclusion in any SUSE product please open         a bug report to request
[  167s] review of the package by the security team.         Please refer to
[  167s] https://en.opensuse.org/openSUSE:Package_security_guidelines#audit_bugs for
[  167s] more information.

Created attachment 852058 [details]
fresh polkit actions

I see the com.deepin.api.locale-helper.policy requests an invalid command in openSUSE. I will drop it for openSUSE. Just whilelisting com.deepin.api.device.unblock-bluetooth-devices is fine.

Please be patient a bit longer. The whitelisting is in OBS sr#912905 currently
under review for inclusion in Factory.

SUSE-RU-2022:1373-1: An update that has three recommended fixes can now be installed.

Category: recommended (moderate)
Bug References: 1070943,1196681,1198521
CVE References: 
JIRA References: 
Sources used:
openSUSE Leap 15.4 (src):    rpmlint-1.10-150000.7.49.1
openSUSE Leap 15.3 (src):    rpmlint-1.10-150000.7.49.1
SUSE Linux Enterprise Realtime Extension 15-SP2 (src):    rpmlint-1.10-150000.7.49.1
SUSE Linux Enterprise Module for Development Tools 15-SP4 (src):    rpmlint-1.10-150000.7.49.1
SUSE Linux Enterprise Module for Development Tools 15-SP3 (src):    rpmlint-1.10-150000.7.49.1

NOTE: This line indicates an update has been released for the listed product(s). At times this might be only a partial fix. If you have questions please reach out to maintenance coordination.