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 ** Changed in: open-vm-tools (Ubuntu) Assignee: Seth Arnold (seth-arnold) => (unassigned) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1220950 Title: [MIR] open-vm-tools To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/open-vm-tools/+bug/1220950/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs