[MIR] systemd-shim

[Allison Karlitskaya]

* The package is in universe and built on all archs: https://launchpad.net/ubuntu/+source/systemd-shim/0.0-0ubuntu1

* Rationale:

This is a necessary part of the work to have systemd-services replacing ubuntu-system-services.

The service emuates a few select systemd interface on an ad hoc basis in order to allow various things depending on systemd to work. For now this is the "Virtualization" property to detect if the system is a VM (with code to do this copied from systemd itself) and the unit control APIs for a faked "ntpd.service" unit. This allows timedated to think that it is requesting systemd to start and stop ntpd when really it is executing the logic that used to be in the Debian-specific gnome-settings-daemon patch we carried to setup and call ntpdate (but ntpd support is also included).

Without this or the real systemd running, timedated won't even start. We could patch that away, but I don't want to get into the business of carrying large/ugly distro-specific patches to timedated when we can just as easily do the compatibility along a documented and stable interface (http://www.freedesktop.org/wiki/Software/systemd/dbus).

It is expected that a few more odds and ends will be discovered over time that belong here. logind work is somewhat likely to kick up a thing or two.

* Security:

The code is small but it needs a full security review. The parts that enable/disable NTP were already running as root via the g-s-d DateTimeMechanism (although the code has been refactored a bit). The virtualisation detection code is copied straight out of systemd, which is being reviewed as part of the systemd MIR. The rest of the code (ie: mostly D-Bus logic) is newly-written.

This is a system service running as root (so that it can start/stop NTP). The primary mechanism for security control is the D-Bus policy file. Root-owned processes are allowed to call all methods (no help there if they already have root). Other processes are only allowed access only to the standard D-Bus interfaces (Introspection, Peer) and property getters. The code dealing with property gets (there is only one property) is extremely small and unlikely to contain exploitable flaws. The D-Bus interfaces (Introspection, Peer) are implemented by GDBus and although it is complicated it is already running inside of several other system services.

* Quality:
- not a user-visible component in any way
- no configuration settings
- no exotic hardware interaction (although it does attempt to use some nice tricks to detect virtualization, but those are copied straight from systemd)
- new code, no known bugs yet, but....
- when the bugs are found, I am the developer, so I'll fix them :)

The desktop bugs team is subscribed to the package in launchpad, foundations/desktop will maintain the package and look to the bug reports regularly.

[Michael Terry (mterry)]

If this is for raring, after Jamie looks at this, it will likely need an FFe too.

[Allison Karlitskaya (desrt)]

See bug 1153567 for the blanket FFe (which was ACKed already).

[Seth Arnold (seth-arnold)]

I audited systemd-shim version 0.0-0ubuntu1 as checked into Raring.

This should not be considered a complete security audit, but rather
a quick gauge of maintainability.

- Package provides shim services for some systemd functionality to run on
  Ubuntu and Debian systems. The interface with other programs and users
  is via DBus interfaces.
- No encryption, no networking, linked against libc, libdl, libffi, libgio-2.0,
  libglib-2.0, libgmodule-2.0, libgobject-2.0, libpcre, libpthread, libresolv,
  libselinux, libz
- Runs as root
- Does not daemonize as usual -- mainloop is started directly from main()
  without usual fork(2), setsid(2), fork(2), chdir(2), close(2) fds
- No {pre,post}{rm,inst} scripts
- Minimal DBus services
- No uid/gid/capability swapping code
- No sudo fragments
- No cronjobs
- No testsuite -- global state and side-effects may complicate writing one
- No build errors or warnings
- Lintian warning hardening-no-fortify-functions, likely a false positive,
  as the package uses glib string manipulation functions
- Subprocesses are spawned carefully
- Most memory management is careful
- Logging looks careful

Specific concerns:

- Doesn't daemonize as usual -- is this intentional to ease upstart
  tracking the service?
- running_in_chroot() is liable to not function reliably in new namespaces;
  expect this to work only for traditional chroot environments
- detect_container() may give silly results if kernel command line includes
  "container=..." string or if the userns namespace started init with funny
  environment variables
- _set_using_ntpdate() uses '/bin/mv -f' rather than rename(2)
- _set_using_ntpdate() does not stop ntpd before starting ntpdate
- _set_using_ntpdate() returns TRUE if ntpdate is not available
- _set_using_ntpd() returns TRUE if ntpd is not available
- _set_using_ntp_debian() returns TRUE if:
  (a) ntpd doesn't exist and ntpdate doesn't exist
  (b) ntpd doesn't exist and ntpdate started once
  (c) ntpd doesn't exist and if-up.d/ntpdate and if-up.d/ntpdate.disabled
      don't exist
  (d) ntpd was (re)started or stopped and ntpdate doesn't exist
  (e) ntpd was (re)started or stopped and ntpdate started once
  (f) ntpd was (re)started or stopped and if-up.d/ntpdate and
      if-up.d/ntpdate.disabled don't exist
  returns FALSE if errors returned while executing:
  (a) g_strconcat ("/usr/sbin/update-rc.d ntp ", using_ntp ? "enable" : "disable", NULL);
  (b) g_strconcat ("/usr/sbin/service ntp ", using_ntp ? "restart" : "stop", NULL);;
  (c) "/bin/mv -f "NTPDATE_DISABLED" "NTPDATE_ENABLED;
  (d) "/bin/mv -f "NTPDATE_ENABLED" "NTPDATE_DISABLED;
  (e) "/etc/network/if-up.d/ntpdate"
- return value of detect_virtualization() is ignored
- shim_get_property allocates memory with g_variant_new(), appears leaked.
  Is this the intentional way to use DBus?

The ntpd vs ntpdate handling feels complicated. I can't imagine when it
would be useful to have both ntpdate and ntpd enabled -- ntpdate will
completely muck with ntpd's drift calculations, leading to overall worse
timekeeping than either ntpdate or ntpd alone. The success and failure
of _set_using_ntp_debian() doesn't make sense to me.

I would like to see a ...

[Allison Karlitskaya (desrt)]

Reply to some of the points raised:

- Doesn't daemonize as usual -- is this intentional to ease upstart
  tracking the service?

It's not required becaues the service is only ever started as a result of D-Bus activation. D-Bus is okay with that.

- running_in_chroot() is liable to not function reliably in new namespaces;
  expect this to work only for traditional chroot environments
- detect_container() may give silly results if kernel command line includes
  "container=..." string or if the userns namespace started init with funny
  environment variables

This code is copied straight out of upstream systemd sources -- I'm trying to emulate what they do here.

- _set_using_ntpdate() uses '/bin/mv -f' rather than rename(2)
- _set_using_ntpdate() does not stop ntpd before starting ntpdate
- _set_using_ntpdate() returns TRUE if ntpdate is not available
- _set_using_ntpd() returns TRUE if ntpd is not available
- _set_using_ntp_debian() returns TRUE if:
  (a) ntpd doesn't exist and ntpdate doesn't exist
  (b) ntpd doesn't exist and ntpdate started once
  (c) ntpd doesn't exist and if-up.d/ntpdate and if-up.d/ntpdate.disabled
      don't exist
  (d) ntpd was (re)started or stopped and ntpdate doesn't exist
  (e) ntpd was (re)started or stopped and ntpdate started once
  (f) ntpd was (re)started or stopped and if-up.d/ntpdate and
      if-up.d/ntpdate.disabled don't exist
  returns FALSE if errors returned while executing:
  (a) g_strconcat ("/usr/sbin/update-rc.d ntp ", using_ntp ? "enable" : "disable", NULL);
  (b) g_strconcat ("/usr/sbin/service ntp ", using_ntp ? "restart" : "stop", NULL);;
  (c) "/bin/mv -f "NTPDATE_DISABLED" "NTPDATE_ENABLED;
  (d) "/bin/mv -f "NTPDATE_ENABLED" "NTPDATE_DISABLED;
  (e) "/etc/network/if-up.d/ntpdate"

The NTP code is gross but we already have it in Ubuntu as part of the gnome-settings-daemon datetime mechanism. It's true that this could use some cleaning up, but I wanted to go with 'tried and tested' rather than rewriting it too much.

- return value of detect_virtualization() is ignored

This is a good catch. I ignored the return value because in the case of failure I was just going to return the empty string, but on closer reading of the code I now see that detect_virtualization() doesn't actually set the return value in the failure case. I'll fix this by initialising to "".

- shim_get_property allocates memory with g_variant_new(), appears leaked.
  Is this the intentional way to use DBus?

From the GDBus documentation for the callback type of this function:

Returns :

A GVariant with the value for property_name or NULL if error is set. If the returned GVariant is floating, it is consumed - otherwise its reference count is decreased by one.

I'll fix up the virtualisation thing, but I think we should avoid refactoring the NTP code for now and clean it up after raring.

[Seth Arnold (seth-arnold)]

Thank you for the detailed responses.

ACK for the proposed plan to fix detect_virtualization() now and address the ntpd/ntpdate issues later.

[Michael Terry (mterry)]

From a packaging/maintainability point of view, this is fine. It'll be in Ubuntu's court to keep up with systemd changes, but that's to be expected here.

Once detect_virtualization() is fixed as Seth requested, this will be approved.

[Michael Terry (mterry)]

Oop, Seb fixed that in a recent upload. Approved. Would be nice to see tests, but not a blocker for this package.

[Colin Watson (cjwatson)]

Moved to main.