Comment 17 for bug 1744072

I reviewed chrony version 3.2-1build1 as checked into bionic. This isn't a
full security audit but rather a quick gauge of maintainability.

- There are ten CVEs in our database; the fixes mostly aren't enumerated
  in our database, but many of the descriptions sound like they were
  handed out 'conservatively' -- errors in administration-level command
  channel or a malicious peer server operator in a position to interpose
  traffic from another peer server.

  I like the paranoia.

- chrony is a new, simpler, smaller, safer, ntp daemon. It's suitable for
  client and server use, and supports some hardware drivers, NIC
  timestamping, but perhaps not as many features as our old NTPD.

- Build-Depends: debhelper, bison, libedit-dev, libtomcrypt-dev,
  libcap-dev, pps-tools, libseccomp-dev, pkg-config, asciidoctor

- libtomcrypt dependency is being worked on; apparently nss is an option
  once we expose an "internal only" library.

- Does daemonize, nicely
- pre/post inst/rm scripts have autogenerated sections. Also:
- postinst script creates _chrony user and group, chowns /var/log/chrony
  and /var/lib/chrony
- postinst cleans up after previous version "key" file (authentication has
  been simplified in newer versions) in a complicated set of comparisons
- postrm removes /var/lib/chrony/, /etc/chrony/, _chrony user and group
- Initscript uses start-stop-daemon to start chrony
- systemd unit file is simple
- No dbus services
- No setuid files
- chronyc and chronyd executables in PATH
- No sudo fragments
- No udev rules
- test suite run at build; not comprehensive, but nice to have
- clean build logs

- sendmail is spawned to send mail via popen(). All variables are under
  control of configuration file. No error handling in case the admin sets
  the "mail to" variable to something silly long or dangerous, but this is
  very low risk.

- Memory management looked careful
- file io looked careful
- logging looked careful
- TZ environment variable used to gather information on leap seconds,
  looked careful
- Privileged operations looked careful
- I did not inspect cryptography
- Privileged portions of the code, privsep-style, looked careful; I did
  not inspect privsep for safety
- Extensive networking, looked careful
- No temporary file handling
- No WebKit
- No JavaScript
- No PolicyKit
- Clean cppcheck

Errors are checked religiously, coding style is unique and awkward but not
a real impediment to maintenance. Obviously ntp is an involved protocol
and probably further flaws will be found -- and we will rely upon
upstream's help for all but the simplest of issues. It looks
professionally programmed.

The only issue I found has no security relevance but may be slightly
surprising:

- reference() uses snprintf() to build a string to call sendmail; the
  username may not fit in the allocated space, and the code gets no
  warning about this.

  Any shell metacharacters in this setting would interfere with proper
  operation of the program.

I'd like to see this addressed for reliability reasons but it's not a
pressing issue.

Security team ACK for promoting chrony to main.

Thanks