Comment 2 for bug 1101978

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

After a high-level limited review:
 * no initscripts, no use of sudo/pkexec/gksudo, no cron jobs, no daemons or
setuid/fscaps, no dbus services
 * is memory management sane?
  - client: not particularly defensive. lots of string operations are not
    checking for return codes
  - server: is defensive - aborts on memory allocations in spice_malloc() and
    that is used almost everywhere (where it isn't, we check the return code)
 * spice_logv() is not verifying the environment variables it is honoring but doesn't seem to be abusable
 * There is setuid(0) call which underscores that the server code can be running as root within qemu, so this code is sensitive. However, the supported virtualization solution for qemu-kvm is to run under libvirt, which provides apparmor confinement by default and doesn't run qemu as root by default. Regular users typically won't run qemu as root when used alone
 * is there any use of encryption? yes. it seems to be checking return codes and doing ssl verification correctly
 * has a predictable filenames in:
  - client/playback_channel.cpp: sprintf(file_name, "/tmp/%u.wav", ++file_id);
  - spice-common/common/canvas_base.c: sprintf(file_str, "/tmp/spice_dump/%u.jpg", id);
  - ... etc

I'd prefer spice-client stay out of main. The server code has some areas of improvement, but it is defensively programmed for the most part, the code will be run confined via AppArmor and upstream (RedHat) seems committed to the project.

Conditional ACK once compiler warnings are addressed and enabling the test suite is investigated/implemented. It would be good to address the /tmp filename issues even though we've got symlink restrictions in Ubuntu. At a minimum these should reported upstream or shown to not be an issue.