Bug 1007723 - AUDIT-0: backintime: DBus service helper security review
AUDIT-0: backintime: DBus service helper security review
Status: RESOLVED FIXED
: 961701 (view as bug list)
Classification: Novell Products
Product: SUSE Security Incidents
Classification: Novell Products
Component: Audits
unspecified
Other Other
: P5 - None : Normal
: unspecified
Assigned To: Hrvoje Senjan
Security Team bot
:
Depends on:
Blocks: 1006356
  Show dependency treegraph
 
Reported: 2016-10-31 08:22 UTC by Hrvoje Senjan
Modified: 2019-09-23 07:55 UTC (History)
6 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments
hardened polkit rules patch (1.39 KB, patch)
2017-03-29 11:46 UTC, Matthias Gerstner
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hrvoje Senjan 2016-10-31 08:22:46 UTC
net.launchpad.backintime.serviceHelper.service:

[D-BUS Service]
Name=net.launchpad.backintime.serviceHelper
Exec=/usr/bin/python3 /usr/share/backintime/qt4/serviceHelper.py
User=root




net.launchpad.backintime.policy:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE policyconfig PUBLIC
 "-//freedesktop//DTD PolicyKit Policy Configuration 1.0//EN"
 "http://www.freedesktop.org/standards/PolicyKit/1/policyconfig.dtd">
<policyconfig>

  <vendor>BackInTime</vendor>
  <vendor_url>https://github.com/bit-team/backintime</vendor_url>
  <icon_name>document-save</icon_name>

  <action id="net.launchpad.backintime.qt4gui">
    <message gettext-domain="backintime">Authentication is required to run Back In Time as root.</message>
    <description gettext-domain="backintime">Start Back In Time GUI as root.</description>
    <defaults>
      <allow_any>auth_admin</allow_any>
      <allow_inactive>auth_admin</allow_inactive>
      <allow_active>auth_admin_keep</allow_active>
    </defaults>
    <annotate key="org.freedesktop.policykit.exec.path">/usr/bin/backintime-qt4</annotate>
    <annotate key="org.freedesktop.policykit.exec.allow_gui">true</annotate>
  </action>

  <action id="net.launchpad.backintime.UdevRuleSave">
    <message gettext-domain="backintime">Authentication is required to add Udev rules.</message>
    <description gettext-domain="backintime">This will install Udev rules which will start Back In Time if a drive get connected.</description>
    <defaults>
      <allow_any>auth_admin</allow_any>
      <allow_inactive>auth_admin_keep</allow_inactive>
      <allow_active>auth_admin_keep</allow_active>
    </defaults>
  </action>

  <action id="net.launchpad.backintime.UdevRuleDelete">
    <message gettext-domain="backintime">Authentication is required to delete Udev rules.</message>
    <description gettext-domain="backintime">This will delete Udev rules.</description>
    <defaults>
      <allow_any>auth_admin</allow_any>
      <allow_inactive>auth_admin_keep</allow_inactive>
      <allow_active>auth_admin_keep</allow_active>
    </defaults>
  </action>

</policyconfig>





net.launchpad.backintime.serviceHelper.conf: 

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN"
"http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">

<busconfig>
  <type>system</type>
  <!-- Only root can own the service -->
  <policy user="root">
    <allow own="net.launchpad.backintime.serviceHelper"/>
    <allow send_destination="net.launchpad.backintime.serviceHelper"/>
    <allow send_interface="net.launchpad.backintime.serviceHelper.UdevRules"/>
  </policy>

  <!-- Allow anyone to invoke methods on the interfaces -->
  <policy context="default">
    <deny own="net.launchpad.backintime.serviceHelper"/>
    <allow send_destination="net.launchpad.backintime.serviceHelper"/>
  </policy>
</busconfig>
Comment 1 Andreas Stieger 2016-10-31 08:25:52 UTC
Archiving:Backup/backintime
Comment 2 Matthias Gerstner 2017-03-24 15:16:19 UTC
I will do this review.
Comment 3 Matthias Gerstner 2017-03-29 08:54:24 UTC
Some general remarks:

- backintime requires python3 (does not run with python2). The spec file as a
  Requires for python-keyring, but this is only for python2. There's a
  python3-keyring devel project but it's not in factory yet. Thus the keyring
  related features of backintime cannot be used ATM. It would be good to get
  python3-keyring into factory and to fix the Requires line.
- there are configuration examples config-example-local and config-example-ssh
  that are not currently installed in the spec file. Those should be installed
  to /usr/shared/doc/packages/backintime/examples so users that don't use the
  GUI can get a template configuration file
Comment 4 Matthias Gerstner 2017-03-29 09:45:00 UTC
I've had a look at backintime. The D-Bus service has the following purpose:

In backup profiles we can set the schedule to "When drive gets connected
(udev)". backintime wants to setup a udev rule that reacts to add/change
events of certain block device UUIDs so the backup can be triggered when the
block device in question appears in the system.

Adding udev rules requires root privileges. Thus backintime ships a dbus
service net.launchpad.backintime.serviceHelper that gets dynamically activated
through the dbus system bus on demand. The serviceHelper is a small (~200
lines) program written in python that only serves the purpose of securely
adding udev rules for backintime.

There are four D-Bus methods implemented:

- addRule: adds a new command to be executed via udev. Multiple commands can
  be added this way but won't be directly written, only cached in the
  serviceHelper until the save() method is called.
- clean: removes any rules previously cached via addRule()
- save: writes all rules previously added via addRule() into a user/backintime
  specific udev rule file.
- delete: removes a potentially existing udev rule file for the calling user
  for backintime.

save() and delete() are constrained by polkit rules so they require
an admin password to be called. The polkit implementation looks valid. These
calls also don't take any parameters.

clean() and addRule() can be called by anyone without polkit authorization.
This method only caches rules for later application during save(). It takes a
block device UUID and an arbitrary command to be executed in the calling
user's context. It limits the input parameters to a whitelist of characters to
avoid malicious input. The command will be executed via 'su' as the calling
user, so no privilege escalation should be possible. Different D-Bus sessions
talking to the serviceHelper do not influence each other, because the rules
are separately stored for each unique D-Bus connection.

I couldn't find any critical security issues in the serviceHelper
implementation. I don't like the design of the addRule() method, however,
because it allows any command to be added to be executed via udev in user
context, not just backintime. This is no privilege escalation by itself but
allows for a large degree of freedom than is necessary for the operation
of backintime.

In theory there is an attack surface for denial of service, because an
arbitrary number of rules may be added via addRule() and also an arbitrary
number of cached rules may be resident in the serviceHelper, because the cache
is never cleared if a sender does never save() or clear(). The serviceHelper
never terminates once is has been started.

The polkit rules allow the following:

- backintime-qt GUI may be started with admin authorization using pkexec. This
  allows backups for the root user to be configured. GUI runs completely as
  root in this case.
- the save() and delete() methods of the serviceHelper D-Bus service may be
  called with admin authorization.

The rules are okay so far but I don't like that active/inactive users are set
to 'auth_admin_keep', which means that after entering the admin password once
successfully, further invocations are allowed without password entry for some
time. I don't think this adds much value, because typically these interfaces
won't be called rapid succession. But it weakens security in a way that the
user may not expect at this point.

All in all I think we can accept backintime's D-Bus and polkit usage. But I'd
like to give some recommendations and make some adjustments that improve
overall security. I will elaborate this in a follow-up comment.
Comment 5 Matthias Gerstner 2017-03-29 11:02:19 UTC
Apart from the dbus service and polkit policies there's another notable
security feature in backintime:

backintime can run as a password caching daemon to cache credentials
previously obtained via some other keyring. This is used for being able to
perform cron based backups even if the user isn't logged in any more, or when
an encrypted partition needs to be mounted automatically.

This daemon is integrated with the backintime command in in password.py /
Password_Cache class. It's controlled with the "backintime pw-cache"
subcommand. The implementation is small (~150 lines) and uses a FIFO for IPC.
The FIFO is put safely in the user's home directory. Due to the nature of
python this daemon can't reliably clear memory that contained passwords during
shutdown. Apart from that all seems okay with it. The daemon runs in user
context, not as root. No polkit involved.

The daemon doesn't work in our current openSUSE package, because the
python-keyring is not available for python3.
Comment 6 Matthias Gerstner 2017-03-29 11:38:50 UTC
Another general remark:

As the dbus service creates custom udev rules per user in the form of

  /etc/udev/rules.d/99-backintime-$USER.rules

the RPM spec file should take care to remove such files during uninstallation,
to not leave behind garbage files.
Comment 7 Matthias Gerstner 2017-03-29 11:46:27 UTC
Created attachment 719151 [details]
hardened polkit rules patch
Comment 8 Matthias Gerstner 2017-03-29 11:49:36 UTC
I've created an upstream pull request to add various hardening measures to
backintime:

https://github.com/bit-team/backintime/pull/727

I'd like to see these changes to be added to our openSUSE packages. Backports
should be feasible if no version bump is desired.

Also I'd like a patch for the polkit policy to be applied, as outlined in
attachment 719151 [details]. This downgrades the auth_admin_keep directives to be on the
safe side. I didn't make this part of the upstream pull request, because I
fear they won't agree with it. Let's first see what they say to the pull
request so far.
Comment 9 Andreas Stieger 2017-04-06 11:22:01 UTC
*** Bug 961701 has been marked as a duplicate of this bug. ***
Comment 10 Matthias Gerstner 2017-04-06 12:20:37 UTC
Sebastian found a more severe issue with the backintime DBus service
implementation. It uses a deprecated authorization method "unix-process". This
authorization is subject to a race condition that could allow unauthenticated
users to add udev rules.

I supplemented my previous upstream pull request. This is the important commit:

https://github.com/bit-team/backintime/commit/7f208dc547f569b689c888103e3b593a48cd1869

I've requested a CVE for this and will open a separate security bug.

Since backintime and its DBus service made it into factory, and leap 42.{1,2}
without approval we need to fix the already existing packages.

The rpmlintrc file supressing the DBus related warnings needs to be removed
from all existing backintime packages once the issues have been dealt with!
Comment 11 Matthias Gerstner 2017-04-06 13:19:57 UTC
I've created bug 1032717 to track the security issue described in comment 10.

Review is complete. Reassigning to reporter. Please provide updates as
described in bug 1032717. Then we can approve the DBus service and polkit
rules.

When doing updates please also consider my suggestions from comment 3.

Thank you.
Comment 12 Bernhard Wiedemann 2017-04-20 20:00:59 UTC
This is an autogenerated message for OBS integration:
This bug (1007723) was mentioned in
https://build.opensuse.org/request/show/489654 42.1+42.2 / backintime
Comment 13 Swamp Workflow Management 2017-04-28 13:08:48 UTC
openSUSE-SU-2017:1124-1: An update that solves one vulnerability and has one errata is now available.

Category: security (moderate)
Bug References: 1007723,1032717
CVE References: CVE-2017-7572
Sources used:
openSUSE Leap 42.2 (src):    backintime-1.1.20-3.3.1
openSUSE Leap 42.1 (src):    backintime-1.1.20-3.1
Comment 14 Bernhard Wiedemann 2017-04-28 14:01:27 UTC
This is an autogenerated message for OBS integration:
This bug (1007723) was mentioned in
https://build.opensuse.org/request/show/491831 Factory / rpmlint
Comment 15 Bernhard Wiedemann 2017-05-03 12:00:37 UTC
This is an autogenerated message for OBS integration:
This bug (1007723) was mentioned in
https://build.opensuse.org/request/show/492617 Factory / polkit-default-privs
Comment 16 Sebastian Krahmer 2017-05-16 08:57:02 UTC
looks like it could be closed
Comment 17 Bernhard Wiedemann 2017-05-17 06:00:40 UTC
This is an autogenerated message for OBS integration:
This bug (1007723) was mentioned in
https://build.opensuse.org/request/show/495451 Factory / backintime
Comment 20 Swamp Workflow Management 2017-09-04 19:07:56 UTC
SUSE-RU-2017:2341-1: An update that has 19 recommended fixes can now be installed.

Category: recommended (low)
Bug References: 1004346,1007053,1007723,1019748,1032649,1032717,1033296,1033554,1034309,1039290,1039709,1039848,1049694,846337,917781,984817,987141,996111,997880
CVE References: 
Sources used:
SUSE Linux Enterprise Software Development Kit 12-SP3 (src):    rpmlint-1.5-41.3.1, rpmlint-mini-1.8-2.2.3