[MIR] systemd

For the record, if you want to play with this: https://launchpad.net/~ubuntu-core-dev/+archive/logind . This also stages packages which block on this MIR until we can upload them, like pulseaudio.

@MIR team: We want to update systemd to the current version 198 instead of releaseing raring with the old version 44. Some packages like gnome-session require a newer version than 44, and we would thus not be able to convert that to logind session tracking. So please feel free to review the package in https://launchpad.net/~ubuntu-core-dev/+archive/logind/+packages instead, which also has a lot more standardized packaging (broken out patches instead of single debian/patches/debian-patches, etc.).

Seth already reviewed the version 198 packages in the PPA, but for the record: 198 has landed in raring-proposed (after Seth's pre-approval on IRC), and removed from the PPA.

Just installed the new udev (175-0ubuntu20) and all appropriate dependants.
However, now my 64-bit nvidia graphics card setup ends up in busybox.
I can go on with normal boot by commanding "exit".

I reviewed version 198-0ubuntu0ppa2 from pitti's PPA.

I confined my review primarily to src/logind/ and src/udevd/ directories,
as these are the largest of the components we intend to use. This should
not be considered a full security audit, but rather a quick and dirty
gauge of code cleanliness.

- No cron jobs, fscaps, sudo
- Several initscripts
- Provides dbus services
- Limited use of setuid(2) looked safe
- Some executables not PIE
- All executables use stack protection, fortify, relro, bind_now
- Minimal tests; extensive global state would be difficult to test
- Daemons initialize carefully
- Many libtool warnings
- Many dpkg-shlibdeps warnings
- Memory allocations check for failure
- Error codes are returned, checked
- String manipulation uses good utility routines
- Crypto used only in un-audited portions

I did not verify if the package provides needed functionality.

Since this is a fairly specialized sort of package, I'm not too surprised
about e.g. libtool and dpkg-shlibdeps warnings. However, they would make
it more difficult to spot warnings in the future. Please consider spending
some time to reduce the warning count.

ACK for the proposed selective inclusion into main.

> - Minimal tests; extensive global state would be difficult to test

Actually, upstream does quite extensive testing with automatically building VM images and booting them in qemu-kvm: http://cgit.freedesktop.org/systemd/systemd/tree/test ; but these are not shipped in release tarballs. I think one day they should so that we can integrate it into autopkgtest.

I moved the source into main, as we already use the libudev* libraries from main. I won't move systemd-services until something depends on it and the FFE is approved.

Thanks for the review!

And now for the packaging/maintainability review. I looked only at version 198 in raring-proposed.

* Packaging is understandably complex. Not only are there lots of vaguely-related components in this one source, but they are system-level core bits.
* Builds cleanly.
* Has bug subscribers.
* Both desktop and foundation teams are looking after the package.
* Existing bugs seem tolerable (most of them deal with the init pieces anyway). Should keep a watch on Debian bug 701364, which says it ftbfs with gcc-4.8.
* Uses symbols files.
* Uses python2, but does so with dh_python2 and the python-systemd package is not likely to be pulled onto the Desktop CD, so that's fine.

Seems almost OK. This will definitely not be a hands-off package, but it's important enough for us to accept the burden.

The one big beef I have is that we don't run any of the unit tests, because some of them fail (pitti says they make assumptions that aren't true on Ubuntu). I'd like to see those fixed or the rest of the unit tests enabled, so we have at least some coverage.

- is there a reason not to provide a python3-systemd package?
- gcc-4.8 can be found in the ubuntu-toolchain-r ppa
- just want to clarify that the MIR is currently not done for the systemd package,
  which I assume will stay in universe.

On Thu, Mar 14, 2013 at 8:18 PM, Michael Terry
<email address hidden> wrote:
> Should keep a watch on Debian bug 701364, which says it ftbfs with gcc-4.8.

That bug has a link to upstream commit from 2012-11-14, which I
believe is included in systemd 198.

Hello Michael, Matthias,

thanks for the review!

Michael Terry [2013-03-14 16:18 -0000]:
> * Existing bugs seem tolerable (most of them deal with the init
> pieces anyway). Should keep a watch on Debian bug 701364, which
> says it ftbfs with gcc-4.8.

I built current master and 198 with gcc-4.8, works fine. The Debian
bug has a link to the upstream commit.

> * Uses python2, but does so with dh_python2 and the python-systemd
> package is not likely to be pulled onto the Desktop CD, so that's
> fine.

Right, that's supposed to stay in universe for now.

> The one big beef I have is that we don't run any of the unit tests,
> because some of them fail (pitti says they make assumptions that aren't
> true on Ubuntu). I'd like to see those fixed or the rest of the unit
> tests enabled, so we have at least some coverage.

I fixed and enabled unit tests during package build, works fine now:
http://people.canonical.com/~pitti/tmp/systemd-patches/0011-Fix-and-reenable-unit-tests.patch
This will be part of the next systemd upload.

Matthias Klose [2013-03-15 4:58 -0000]:
> - is there a reason not to provide a python3-systemd package?

It doesn't currently build with PYTHON=python3.

> - gcc-4.8 can be found in the ubuntu-toolchain-r ppa

See above, I tested with gcc-4.8 and it works fine.

> - just want to clarify that the MIR is currently not done for the
> systemd package, which I assume will stay in universe.

We don't even build systemd and systemd-sysv binaries on Ubuntu, for
mostly political reasons. But I'm wary of bringing this up over and
over again, so I don't intend to enable them for universe for the time
being.

Thanks,

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)

systemd with enabled "make check" tests is in raring now. I had to disable the "unit-name" test as it fails on the buildds (presumably they don't have a /etc/hostname), the other 25 succeed now.

I also wrote autopkgtests for the three D-BUS services hostnamed, localed, and timezoned, and fixed localed to actually work:
https://jenkins.qa.ubuntu.com/view/Raring/view/AutoPkgTest/job/raring-adt-systemd/

I wrote an autopkgtest for logind as well (not uploaded yet), but due to the nature of our jenkins server (where the test doesn't actually get a seat) this works only locally. Its still useful for regression testing, though, it discovered that logind doesn't apply ACLs for hotplugged devices properly. I added a work item for this to the BP.

Awesome, thanks Martin! Approved for the binary packages we need (systemd-services and such; and obviously the udev packages which are already in main).

Thanks! I moved the source to main, and the libsystemd-* libraries, so that component-mismatches will be happy. I keep systemd-services in universe for now, as the FFE hasn't been approved yet.

I was asked to expand the audit I performed earlier (summarized in
comment #5 above). I reviewed version 198-0ubuntu0ppa2 from pitti's PPA.

Again, this is not intended to be a complete audit. Not everything I found
is a security issue, I'm just reporting things that looked surprising to me.

timedated

- Forces /etc/localtime to symlink
- Racecondition between valid_timezone() and write_data_timezone()
  low risk because ../usr/share/zoneinfo/ is prepended, untrusted accounts
  shouldn't have write access here
- hwclock_set_timezone() and hwclock_reset_timezone() look misused or abused:
  both are used to 'seal' the Linux kernel's magic settimeofday(2) handling to
  adjust for CMOS-based clock being set to local time (windows) or UTC (unix).
  The 'tz' variable should otherwise be unused. So calling these functions
  multiple times in one boot is probably useless.
- write_data_local_rtc() appears to leave 'w' without terminating NUL
- SetTime timespec_store() doesn't appear to account for wraparound
- Many strings in timedatectl.c aren't localized

hostnamed

- read_full_file() realloc grows buf by one byte each loop iteration
  this will be bad performance for files between 4K and 4M.
- hostname_is_valid() will allow invalid names such as '.' or '..' or '_hi'
- Many strings in hostnamectl.c aren't localized

ACK for including both timedated and hostnamed in main, provided that
upstream is consulted for the settimeofday(2) issues, timespec_store()
wraparound issue, and hostname_is_valid() issue.

I've reviewed the review in comment #14 and looked over the code myself as well as talking to Lennart.

Point-by-point:

- I don't understand why having /etc/localtime as a symlink is anything but desirable other than that Debian didn't do it this way before. I do notice that the symlink is not replaced atomically (using the rename-temp trick), however, meaning that the system is (de facto) on UTC for a short period during changes. I've raised this point with upstream.

- The race condition between checking timezone validity and changing the timezone is a total non-issue; mostly for the reason that you mention, but also because even if the timezone was erased _after_ the link was established, the result would be the same: dangling symlink. This is really not a race...

- the story with hwclock_set_timezone() and hwclock_reset_timezone() is stupidly complicated because of Linux being ridiculous on this point. The kernel wants to know the timezone for two reasons: in case the RTC is in localtime, it wants to jog the system time (that it assumed was in UTC) to proper UTC. This is done only on the first call to settimeofday(). The kernel also wants to know the timezone for other reasons though (like setting proper timestamps on FAT filesystems). The way to set the timezone for these other reasons without doing the 'first time warp' is to first set a NULL timezone (no warp on the first time) and then make a second call to actually set the timezone. I agree that it's stupid that the kernel should want to know what timezone userspace thinks that it's in, but that's how it is...

- the write_data_local_rtc() point you raise is valid code, but utterly ugly. I asked Lennart to rewrite it.

- the wraparound issue: I assume you mean that relative time changes are not checked for wrapping the clock. I don't think this is a big problem since it's a privileged operation and you could simply say that this is the semantics of the interface (and honestly, I can't imagine what else you would want to happen in this situation -- we could end up with a clock set to a ridiculous time, but we could end up with that anyway).

- localised strings: true.

- Lennart claims that realloc() is smart enough not to reallocate every time so we'd be wasting our time by trying to duplicate its efforts. That claim seems a bit suspect from the standpoint of my doubt that it would waste as much as double the amount of requested memory, but I didn't investigate further. I don't believe this to be a substantial problem in any case.

- The examples you give for hostnames accepted by hostname_is_valid() are obviously pretty bogus. This will be fixed upstream.

Martin Pitt (pitti) wrote on 2013-03-15:
> Matthias Klose [2013-03-15 4:58 -0000]:
> > - is there a reason not to provide a python3-systemd package?

> It doesn't currently build with PYTHON=python3.
That is unexpected. Can you elaborate? At least on Debian/sid it builds fine with PYTHON=python3.

zbyszek [2013-03-22 18:30 -0000]:
> That is unexpected. Can you elaborate? At least on Debian/sid it
> builds fine with PYTHON=python3.

Indeed, current master builds fine now. Thanks for pointing out!

--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)

Ryan: the reason /etc/localtime isn't a symlink was always to support /usr being on a separate partition. Reverting this will force completion of the work to have the initramfs mount /usr, which AFAIK is not yet done in raring.

Hello,

Revisiting this MIR since we (now) want to have systemd-container (from src:systemd) from Bionic/universe in Bionic/main.

This package is to be included in the Google cloud images the public cloud team builds, going back to Bionic. As cloud images are to ship only packages from main, this request is to see that happen.

src:systemd is in main in Bionic but bin:systemd-container isn't. Adding a task for Bionic for the same. Please let me know if there's anything else needed? TIA!

we usually don't require MIRs for new binary packages. Please just make sure that the security team knows about the added binary in bionic, and then we can promote it.

The usual way we determine if a package is in main or not is to check the package lists; will the promotion step make the systemd-container binary package visible to package lists or rmadison output?

Thanks

Yes, the systemd-container package will end up in main, likely for the current package in bionic-updates, and thus will be reflected that way in rmadison etc.

For the record, ack from the Ubuntu Security Team on promoting the systemd-container binary from universe to main in bionic.

Thanks.

Hello,

> For the record, ack from the Ubuntu Security Team on promoting the
> systemd-container binary from universe to main in bionic.

Thank you, Steve! I'll work on doing the next steps.

Override component to main
systemd-container 237-3ubuntu10.48 in bionic amd64: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.48 in bionic arm64: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.48 in bionic armhf: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.48 in bionic i386: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.48 in bionic ppc64el: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.48 in bionic s390x: universe/admin/optional/100% -> main
6 publications overridden.
Override component to main
systemd-container 237-3ubuntu10.38 in bionic amd64: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.38 in bionic arm64: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.38 in bionic armhf: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.38 in bionic i386: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.38 in bionic ppc64el: universe/admin/optional/100% -> main
systemd-container 237-3ubuntu10.38 in bionic s390x: universe/admin/optional/100% -> main
6 publications overridden.