Comment 4 for bug 1749672

I reviewed xdg-desktop-portal (0.11-2) as checked into cosmic. This is not
a full security audit but rather a quick gauge of maintainability.

I did not audit the design of the permission caching database, nor the
safety of the DBus interface, nor the FUSE implementation. The review was
strictly to gauge the relative costs of maintaining what is already
written.

- no pre,post inst,rm scripts
- no init scripts
- three user systemd unit files, dbus-activated
- no system dbus services
- no setuid files
- no binaries in PATH
- no sudo fragments
- no udev rules
- tests run during the build, many tests, but I'm unsure of coverage
- no cron jobs
- clean build log

- one instance of spawning a process, to unmount a fuse filesystem; it
  uses g_spawn_sync() with an array for argv
- memory management looked careful
- extensive file IO, including a FUSE interface
  - I'm really skeptical of the path rewriting functionality:
    /newroot /app /usr /etc handling
- logging looked fine
- Uses environment variables: XDG_CURRENT_DESKTOP, GIO_USE_VFS,
  PARENT_WINDOW_ID, XDG_RUNTIME_DIR, XDG_DATA_HOME, XDG_RUNTIME_DIR
- Does not use privileged syscalls
- Does not use cryptography
- DBus is used to reach privileged portions of code
- Does not use webkit
- Temporary file handling re-implements mkstemp() and uses the insecure
  RNG filename generator
- Does not use webkit
- Does not directly use javascript
- Two cppcheck errors, both cppcheck failurs
- Does not use polkit

I have to admit I expected far less code for this service than is
here. DBus and FUSE both seem like fairly large surfaces. I expected one
quarter the code and passing file descriptors over unix domain sockets.

The codebase is properly defensive, error returns are consistently
checked, safer interfaces are consistently used over unsafe interfaces,
and the few small things I noticed aren't too unusual for software of this
complexity. It does feel more complex than necessary but I have not myself
tried to implement a similar tool, so perhaps my own mental model is not
yet complete enough.

The few things I found odd:

- allocate_inode_unlocked() if (next <= 0) -- next is unsigned

- xdp_mkstempat() should use a stronger RNG

- main() in document-portal/document-portal.c only does a single fork(),
  skips setsid()

Security team ACK for promoting xdg-desktop-portal to main.

Thanks