Archiving:Backup/backintime

I will do this review.

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

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.

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.

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.

Created attachment 719151 [details]
hardened polkit rules patch

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.

*** Bug 961701 has been marked as a duplicate of this bug. ***

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!

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.

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

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

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

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

looks like it could be closed

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

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