Comment 7 for bug 1200296

Seth Arnold (seth-arnold) wrote :

I reviewed spice-vdagent 0.17.0-1ubuntu1 as checked into zesty. This
shouldn't be considered a full audit but rather a quick gauge of
maintainability.

spice-vdagent provides some services between virtual machine host and
guests to make the experience less jarring.

One CVE is in our database for the Windows client.

- Build-Depends: debhelper, pkg-config, dh-systemd, libspice-protocol-dev,
  libdbus-1-dev, libx11-dev, libxrandr-dev, libxfixes-dev,
  desktop-file-utils, libxinerama-dev, libpciaccess-dev, autoconf, automake,
  libglib2.0-dev, systemd, libsystemd-dev, libasound2-dev
- Provides a client and server; both daemonize
- pre/post inst/rm scripts automatically generated
- spice-vdagent init script starts the guest daemon, modprobes uinput
- spice-vdagentd and spice-vdagent systemd service files, start their
  daemons
- no dbus services
- No setuid or setgid files
- Two executables in PATH /usr/bin/spice-vdagent and
  /usr/sbin/spice-vdagentd
- No sudo fragments
- One udev rule for virtio-ports
- No test suite
- No cron
- Clean build logs

- Subprocesses spawned using system(), unsafe construction, reported
  upstream
- Memory management looked good enough; some cases of malloc(a*b) but 'b'
  was often 4, 8, maybe 16, and 'a' calculated from data on the wire in a
  fashion that looked difficult to really abuse.
- File IO looked safe except for uses of system()
- Logging looked safe
- No environment variable use
- chmod(socket, 0666) looked out of place
- other privileged ioctl() calls looked fine
- No cryptography
- Does networking; a quick skim looked like all Unix Domain Sockets
- I didn't see privileged portions of the code
- No tmp files
- No WebKit
- No PolicyKit
- Clean cppcheck

Here's some notes I collected while reviewing spice-vdagent:

- vdagent_file_xfers_data() does not escape xfers->save_dir before giving
  it to the shell (CVE-2017-15108 was assigned for this issue)
- vdagent_file_xfers_data() does not check snprintf() return code; a
  too-long xfers->save_dir could cause the & or ' or any number of other
  characters to go missing.
- daemonize() from ./src/vdagentd.c only forks once
- daemonize() from ./src/vdagent.c only forks once
- why does main() in ./src/vdagentd.c set vdagentd_socket to 0666

This symlink looks out of place:

/usr/share/gdm/greeter/autostart/spice-vdagent.desktop -> /etc/xdg/autostart/spice-vdagent.desktop

Please make sure https://cgit.freedesktop.org/spice/linux/vd_agent/commit/?id=8ba174816d245757e743e636df357910e1d5eb61 is included in our package before promoting the package.

Security team ACK for promoting spice-vdagent to main.

Thanks