MIR: libmicrohttpd

[Martin Pitt]

Availability: builds on all architectures

Rationale:
 - Used by systemd's remote journal support; we'd like to enable this as it provides nice and secure (over SSL) logging for devices without much space, or writable root etc. Users are asking for it (bug 1480952) and it's also a nice feature for snappy.
 - Enablement done in Debian: http://anonscm.debian.org/cgit/pkg-systemd/systemd.git/commit/?id=52758fa

Security: two issues in the past (http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=libmicrohttpd) through "standard" buffer overflows. usage of it in systemd would mitigate this as the unit is strongly confined.

QA/maintenance:
 - Just a library, no user interface; no debconf,
 - No serious, and very few bug reports in Debian/Ubuntu
 - Standard dh7/dh_install packaging, no oddities
 - Adequate package maintenance in Debian, no Ubuntu delta planned
 - Adequate upstream maintenance: Search for "microhttp" on https://gnunet.org/bugs/view_all_bug_page.php → bugs get fixed and responded to
 - Package has watch file
 - /!\ Package has some automatic tests, and some example C programs for manual testing; not enabled during package build

Dependencies: all build/binary deps already in main

Note that at least for now we don't necessarily need to put the new systemd-journal-remote binary package into main; but we need the -dev as a build dependency, thus libmicrohttpd-dev needs to be in main for this.

[Martin Pitt (pitti)]

If this is otherwise fine, I'll work on running the tests during package build.

[Michael Terry (mterry)]

Blockers:
- Tests should be run indeed, thanks for looking into that.
- Needs a team bug subscriber for whomever will look after this in Ubuntu.

Notes:
- I wish it passed --disable-spdy in debian/rules, because when building on a machine with libopenssl, it will automatically enable that and fail the build because of --fail-missing.
- This seems like a security sensitive package. Will subscribe ubuntu-security for a looksee.

Otherwise, it looks fine.

[Seth Arnold (seth-arnold)]

Does any of this code run in pid 1 when enabled?

Thanks

[Martin Pitt (pitti)]

> Does any of this code run in pid 1 when enabled?

No. This is only used by the split-out systemd-journal-remote package, by /lib/systemd/systemd/-journal-gatewayd. This runs as user "systemd-journal-gateway" and it is tightly locked down in its session cgroup (see systemd-journal-gatewayd.service):

User=systemd-jouranl-gateway
Group=systemd-journal-gateway
PrivateTmp=yes
PrivateDevices=yes
PrivateNetwork=yes
ProtectSystem=full
ProtectHome=yes

So this can't access /home at all, the root partition will be readonly for it, it does not have /dev access (just a small /dev/null and /dev/zero private dev). Its sole purpose is to expose /{var,run}/log/journal/ on a HTTP socket (there is some REST API) so that remote clients can read and store that.

[Martin Pitt (pitti)]

Apparently CVE's search doesn't match word substrings; I adjusted the description accordingly, there *were* two CVEs in the past. Sorry for the initially incorrect information.

[Martin Pitt (pitti)]

https://launchpad.net/ubuntu/+source/libmicrohttpd/0.9.37+dfsg-1ubuntu1 now runs tests during build and adds an autopkgtest for the -dev package. I forwarded both changes to Debian.

[Martin Pitt (pitti)]

I am also subscibed to bugs now.

[Martin Pitt (pitti)]

FTR, I dropped my personal bug subscription and subscribed foundations-bugs now.

[Martin Pitt (pitti)]

Second Debian bug with the autopkgtest is https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=797157

[Martin Pitt (pitti)]

@mterry:
> - I wish it passed --disable-spdy in debian/rules, because when building on a machine with libopenssl, it will automatically enable that and fail the build because of --fail-missing.

This has been fixed in Debian now: https://tracker.debian.org/news/714880 . The spdy packages are now separate binaries (and it's fine for my purposes to keep them in universe)

@Seth: Do you need any further security info about this?

Thanks!

[Tyler Hicks (tyhicks)]

On 2015-10-14 07:54:49, Martin Pitt wrote:
> @Seth: Do you need any further security info about this?

I don't think we need any further security info at this time. We'll do a
shallow security audit of libmicrohttpd during the 16.04 devel cycle and
report back at that time.

[Seth Arnold (seth-arnold)]

I'd prefer to disable SPDY entirely; based on what I saw, I'm not sure that it's ready to be packaged.

Thanks

[Seth Arnold (seth-arnold)]

I reviewed libmicrohttpd version 0.9.44+dfsg-1 as checked into xenial.
This shouldn't be considered a full security audit, but rather a quick
gauge of maintainability.

- [item elided]
- parse_uri() does not check error returns from asprintf()
- store_in_buffer() can leak 'dst' if realloc() fails
- SPDYF_start_daemon_va() calls spdyf_parse_options_va(), which treats all
  addresses as identical struct sockaddr types. However,
  SPDYF_start_daemon_va() includes code which checks the daemon->address
  as if it were a struct sockaddr_in6. I suggest using ASAN or valgrind
  with this with IPv6 addresses.

And some more subjective feedback:

- SPDYF_run() select(2) is a cranky interface, I'd pick something else
  first. select(2) can't handle file descriptors larger than 1024, which
  limits the utility of the server.
- Much of the code needs to be run through indent; the project ought to
  pick a coding style and enforce it. Mixing coding styles within one
  source file is exhausting to read.
- Commented out code is confusing. Consider deleting each piece of
  commented out code.

Lintian errors and warnings:
E: libmicrohttpd10: postinst-must-call-ldconfig usr/lib/x86_64-linux-gnu/libmicrohttpd.so.10.34.0
W: libmicrohttpd-dev: info-document-missing-image-file usr/share/info/libmicrohttpd.info.gz performance_data.png
E: libmicrospdy0: postinst-must-call-ldconfig usr/lib/x86_64-linux-gnu/libmicrospdy.so.0.0.0

The build logs are slightly noisy with ignored error returns from read(),
write(), asprintf() and dpkg-gencontrol warnings about -is and -ip
parameters.

Much of the code looks careful and professional. Some of the code looks
very immature and probably shouldn't have made it into a "library
release", even with a version number 0.9.something.

I think we should disable the SPDY libraries in our packaging: there's a
lot of work left before they're production-ready, and I would not expect
ABI or API stability from this library.

ACK from the security team for promoting libmicrohttpd to main with the
provision that the SPDY libraries are either no longer built or remain in
universe. We suggest removing them for the time being.

Please also address the lintian warnings and errors before release.

Thanks

[Martin Pitt (pitti)]

Thanks Seth!

I disabled the SPDY packages and added the missing PNG for the info page in https://launchpad.net/ubuntu/+source/libmicrohttpd/0.9.44+dfsg-1ubuntu1 . The "postinst-must-call-ldconfig" lintian error sounds like a bug in debhelper or lintian, not something that an individual package could do something about -- but either way, the warning is gone on current xenial, so this has been fixed.

I'll forward the .png fix to Debian.

So this looks approved now, I'll upload systemd with enabling remote journal next week. Thanks!

[Seth Arnold (seth-arnold)]

Thanks Martin,

Christian reported to me that he just released "MHD 0.9.47 without libmicrospdy in it" -- that may be a cleaner way to remove the spdy packages.

Thanks

[Martin Pitt (pitti)]

Seth Arnold [2015-12-08 19:28 -0000]:
> Christian reported to me that he just released "MHD 0.9.47 without
> libmicrospdy in it" -- that may be a cleaner way to remove the spdy
> packages.

Indeed! So we can sync again once Debian updates to the new release.

[Martin Pitt (pitti)]

I uploaded systemd with the build-dep and promoted the package. Thanks for the review!