Comment 8 for bug 1644364

I reviewed htop version 2.0.2-1 as checked into artful. This should not be
considered a full security audit but rather a quick gauge of
maintainability.

- One CVE in Ubuntu's CVE database, for printing unsanitized data to the
  screen. (Specifically process names; I expect there's more of this.)

- htop is a prettier top
- Build-Depends: debhelper, dh-autoreconf, dpkg-dev, libncurses5-dev,
  libncursesw5-dev, python-minimal
- No cryptography
- No networking
- Does not daemonize
- No pre/post inst/rm scripts
- No initscripts
- No setuid executables
- htop executable in PATH
- No sudo fragments
- No udev rules
- No test suite at build
- Didn't see cronjobs
- Noisy build logs, including missing seteuid() return handling

- htop can spawn strace; the execlp() itself looked fine, but the
  seteuid() nearby does no error checking at all
- Some memory-management wrappers are used; there's a RichString
  abstraction that tries to be friendly with memory management, but
  I'm afraid that not all functions properly handle the distinctions
  between pointer pointing into the struct, and pointer pointing at
  malloc()ed space.
- Most filenames are constructed via CPP token-pasting
- Almost no logging
- Uses LC_CTYPE LC_ALL HTOPRC HOME XDG_CONFIG_HOME environment
  variables; I didn't inspect these closely
- Some privileged operations are used, sometimes without checking errors
  returns.
- No cryptography
- No networking
- The bulk of the code may be executing privileged; transitions to
  unprivileged may not be safe
- No temporary files
- No webkit
- No policykit
- No javascript
- Clean cppcheck

Most of the code looked alright and it might be suitable for use on
personal desktops however I don't think htop is sufficiently paranoid
to be run as root on systems with untrusted unprivileged users. I don't
believe that the benefits of htop outweigh the risks at this time,
so security team NAK for promoting htop to main.

Feel free to re-apply after adding error handling to seteuid() and
snprintf() calls and converting the sprintf() calls with floats to
snprintf() calls; and investigate what happens if a 200-byte RichString
is extended by another 200 bytes. (My guess is it'll just buffer-overflow
and scribble on unrelated memory.)

Here's some notes I took while reviewing htop, I hope they're useful:

- NOT OKAY (void) seteuid(getuid());

- The code makes many assumptions that floats are "safe" when printing
  them. Floats can overflow buffers in unexpected ways. snprintf()
  should be used almost anywhere that floats are being printed.

- The snprintf() error return should be checked everywhere.

- UptimeMeter_updateValues() assumes uptimes are less than 9999 days
  without any error handling.

- RichString_extendLen() looks like it's missing support for extending a
  rich string from e.g. 200 bytes by another 200 bytes.

- LinuxProcessList_readStatFile() could very easily be tricked into buffer
  overflows if /proc/pid/stat files are maliciously constructed (say via
  container filesystems, private filesystem namespaces, etc)

Thanks