[MIR] htop

Your rationale section doesn't explain what you want to do with htop. Is the Ubuntu Server team looking to seed htop on the ubuntu-server image?

Yes - I have updated the section.

htop needs a team subscriber, and a review/cleanup of old bugs in Launchpad. I see no other issues with the package; but please fix the above. Thanks!

The server team is now a team subscriber.

The bug backlog was evaluated and replied to each. Of the two actionable items, one was found only in precise and one was found only in trusty, however I am not bothering with an SRU for either due to lack of demand, precise falling off support shortly, and lack of where the fix is for the trusty defect.

This is a complex package and it has already had a CVE, and a similar bug has been recently opened that looks as though packages being "hidden" might still be an issue:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=793106

I see no particular issues with the package (other than the complexity of its source), but I think it would benefit a quick Security review.

Upstream uses the throw a tarball over the wall model of development rather than open development in a tracked repo. This greatly complicates security support for this type of package.

Scratch that last comment, the repo is at https://github.com/hishamhm/htop

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

I am withdrawing this MIR request and marking this invalid.

While a very useful application, based on the security review it seems
this is not a good candidate at the current time. There are more than a
few small changes required to get this accepted. This review will be
passed onto the maintainer, but not acted on by myself.

htop appears to be a good opportunity to be delivered as a snap with
confinement to get prevent concerns from the security review. Once
there is a defined process for a snap-based MIR I can reapply at that
time.

Thanks to folks for their time and reviews.

Josh, I had a very nice mail from the author; he intends to address most of my issues with the next release. I believe it would be worthwhile addition after he does so -- it is a very popular request from users and it's undeniably pretty. :)

Thanks

I reviewed the fixes Hisham has committed to the htop git tree and liked what I saw.

Security team ACK for promoting htop > 2.0.2 to main. (The fixes I've asked for aren't yet in a release.)

Thanks

Thanks Seth! Marking bug new again.

This is still in a locked state - not sure if that will be in 18.04.
The ack is for >2.0.2

The changes are in git for a while, but no version with them was released yet.
Next steps are:
1. waiting until a new version is released upstream
2. Debian to pick it up (here we could move ahead if we want)
3. only then we are allowed to change the seeds in regard to the security ack

It will be done, but there are a few things not that much under our control we have to wait on.

Filed an upstream issue polling for a 2.0.3 release we could align to.
That is more proactive than just wait-and-see.
We can then decide our next steps based on the answer there.

https://github.com/hishamhm/htop/issues/720 completed
2.1.0 released.

I opened Debian bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=889634 to poll if they would mind packing that soon.
If not we will likely go ahead for 18.04.

From Debian - looks good:
We're packaging it and should have it in sid by end of next week.
That should be well before the import sync freeze for Bionic Beaver
(planned for March 1st).

Released in Debian (thanks) and also 2.1.0-2 is in Bionic universe now.
That said all preconditions for the MIR ack to be ok are complete.

I'll prep a seed change to review (IIRC this was only adding to main, not about shipping it on disk - let me know if intentions were otherwise)

Linked lp:~paelzer/ubuntu-seeds/18.04-support-htop which is the proposed seed change.

@MIR Team - we realized that this has no MIR Team ack yet, so I kindly ask you as well to formally ack this MIR.

MIR approved.

this doesn't show up in any component mismatches

Override component to main
htop 2.1.0-2 in bionic: universe/utils -> main
htop 2.1.0-2 in bionic amd64: universe/utils/optional/100% -> main
htop 2.1.0-2 in bionic arm64: universe/utils/optional/100% -> main
htop 2.1.0-2 in bionic armhf: universe/utils/optional/100% -> main
htop 2.1.0-2 in bionic i386: universe/utils/optional/100% -> main
htop 2.1.0-2 in bionic ppc64el: universe/utils/optional/100% -> main
htop 2.1.0-2 in bionic s390x: universe/utils/optional/100% -> main
7 publications overridden.

Thanks Mathias - I was waiting on an ack on https://code.launchpad.net/~paelzer/ubuntu-seeds/18.04-support-htop/+merge/337201 which will trigger the mismatch then.

Branch is merged now.