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 high-level description of the goals here; my
assumption is that we'd like to support, in the end, a configuration
dialog box with three radio buttons:

   ( ) Use ntpd (preferred for good network connection)
   ( ) Use ntpdate (preferred for sporadic network connection)
   ( ) Do not synchronize time (bad idea)

(Maybe these "radio buttons" don't exist in a user-facing dialog but might
be high-level choices we make based on the form factor or power saving
settings or something like that. But I think they accurately represent
our choices.)

The current layout of functions seems complicated to support this dialog
box; the functions are doing a lot. I think it'd be easier to have a
function for enabling/disabling ntpd, a function for enabling/disabling
ntpdate, and then provide switch_to_ntpd(), switch_to_ntpdate(),
and switch_to_bad_time() (or whatever..) functions that just call the
targeted helpers.

At least some of the error conditions could be avoided by static string
definitions and using rename(2) instead of spawning mv. (rename(2) can
still fail, of course, but for fewer reasons than fork(2) or execve(2)
can fail.)

I think this needs another iteration before landing in main.

Thanks