Comment 13 for bug 1220950

I reviewed open-vm-tools version 2013.04.16-1098359-0ubuntu2 as checked
into saucy. This should not be considered a full security review, but
rather a quick gauge of code maintainability.

There are a lot of moving pieces in this package; I certainly cannot claim
to have studied much of it, but I aimed primarily for finding rough edges
around the backdoor execution mechanisms. Scott Moser raises many good
points about the difficulty of reasoning about a system's sanity from
within the system when a 'backdoor' is so readily available, but I do not
believe this package significantly changes the problem. The hypervisor is
necessarily completely trusted and it is at the whim of the hypervisor
administrators to destroy, damage, or degrade service within a guest.

The backdoor communication channel exists without this package and can be
used by a variety of third-party tools regardless of the presence of this
package. The hypervisor already has the feature and (as I understand) it
cannot be disabled -- though portions of it can be administratively
controlled from the hypervisor itself.

I further understand that access can be limited within the guest by
restricting the CAP_SYS_RAWIO capability or denying iopl(2) and ioperm(2)
systemcalls via seccomp2 filters. (None of which is helped nor hindered by
this package.)

So I do not worry about this package providing a less visible
administrative interface: this issue already largely exists through
less convenient mechanisms with all hypervisors. (And, one could argue,
through the firmwares of physical hardware devices, absent hypervisors.)

- open-vm-tools provides a variety of guest services for use with the
  VMware family of hypervisors, including host-guest filesystems, shared
  copy-and-paste facilities, environment control, and ability to execute
  code within the guest.
- Build-deps autotools, doxygen, libcunit, libdumbnet, libfuse, libgtk2.0,
  libgtkmm-2.4, libicu, libnotify, libpam0g, libprocps0, libx11,
  libxinerama, libxss, libxtst, gcc-4.7
- Depends on ethtool, zerofree, xauth, xdg-utils
- SSL stubs exist to disable SSL, go figure...
- Provides variety of services, some via daemons, some via loadable kernel
  modules. vmtoolsd at least properly daemonizes.
- Initscripts looked fine
- No dbus
- One setuid executable, vmware-user-suid-wrapper, looked safe
- No sudo fragments
- No cronjobs
- Udev file looked safe
- Tests look built during build, but don't appear to be run during build
- Build looks clean; uses -Werror

Many lintian warnings and errors:
- hardening-no-fortify-functions
- latest-debian-changelog-entry-without-new-version
- manpage-has-errors-from-man
- binary-without-manpage
- malformed-override

The hardening-no-fortify-functions warning may be a spurious warning; I see
-D_FORTIFY_SOURCE=2 throughout the build logs.

- open-vm-tools does spawn programs. The interfaces look overly
  complicated to try to support multiple operating systems from one
  codebase, the environment handling routines are quite complicated, and
  there's some surprising double-encoding of filenames, arguments. I'm
  skeptical of some of the quoted argument handling though I couldn't
  find flaws in it.
- Most memory management looked safe; some environment handling code
  looked like it was overly complicated and could have been written with
  far fewer allocations and copies.
- Extensive file operations; the operations I inspected look safe, though
  hard-coded /tmp/VMwareDnD/ path is awkward.
- Extensive logging operations; the ones I inspected looked safe.
- The bulk of the cryptography code exists to disable SSL without
  drastically modifying other code elsewhere. Odd.
- Extensive networking, including within kernel modules. I did not inspect
  this code.
- There are extensive areas of privilege management; the ones I inspected
  looked safe.
- The little temporary file handling looked safe.
- Does not use WebKit
- Does not use PolicyKit

Here's some notes I took while reading the source code, in the hopes that
someone finds them useful:

- vmware-user-suid-wrapper/main.c StartVMwareUser() does not drop
  supplementary groups -- should it?
  It is safer to drop groups before dropping user.
- Presumes username, home directory, gecos, shell, all Unicode-strings
- Overly complicated VixToolsBuildUserEnvironmentTable() could just
  replace the first '=' with '\0' and drastically simplify the rest of the
  memory handling
- VixToolsEnvironmentTableEntryToEnvpEntry() exists only to undo the work
  done in VixToolsBuildUserEnvironmentTable()
- VixMsgEncodeBuffer() engages in baffling base64 + escaping, but none of
  the escaped characters occur in base64 output.
- HgfsBdChannelClose() isn't idempotent despite the comment

This is a large package with complicated code covering many potential
attack surfaces. While the code I inspected looked carefully programmed,
it was complicated to cover many Unix-ish and Windows platforms, in
addition to Linux. Large compatibility layers have been written to get
this kind of portability, even where some aspects don't make much sense
on all platforms involved.

Security team ACK for promoting to main.

Thanks