Comment 17 for bug 1709164

- 1 closed CVE in our CVE database CVE-2017-5226 (LP #1657357)
  - Fixed in a timely fashion but by updating to a version which is not ideal
- Provides ability to launch other applications within a sandbox via (user)
  namespaces and bind mounts etc.
- Build-Depends: libcap-dev, libselinux1-dev
- Does not daemonize
- No use of udev
- No pre/post inst/rm scripts
- No initscripts / systemd unit files
- No DBus services
- 1 setuid file:
  rwsr-xr-x root/root 59496 2018-07-12 18:33 ./usr/bin/bwrap
  - Note: the Ubuntu kernel supports unprivileged user namespaces so there is
    no reason for bwrap to be setuid - I have tested this also without setuid
    and it works as expected. As such I strongly suggest we patch out the part
    of the debian packaging which makes this setuid.
- binaries added to the PATH
  rwsr-xr-x root/root 59496 2018-07-12 18:33 ./usr/bin/bwrap
- No sudo fragments
- No udev rules
- System tests exist but are not run as part of package build
  - no unit tests
- No cronjobs
- Clean build logs - no warnings during build

- Subprocesses are spawned as that is the core functionality of bubblewrap
- Memory management looks good, no obvious issues and all memory allocations
  are checked and appropriately handled with good defensive programming.
- Does not directly use any environment variables but does use setenv to allow
  caller to set vars in child process environment
- Uses privileged operations to setup namespace then drops privileges before
  executing child process. Correctly detects when running as setuid root and
  correctly drops down to the saved user-id before executing the resulting
  child process so should not be a problem when using new user namespace even
  though is setuid root.
- No cryptography
- No network connections
- Uses temporary files to be able to pass in files to the subprocesses
  namespace - uses umask(0) and mkstemp() so this looks to be secure
- No WebKit
- No JavaScript
- No PolicyKit
- Clean cppcheck
  - 1 false positive error for a memory leak - cppcheck is confused due to the
    use of gcc's cleanup attribute to automatically free the memory when it
    goes out of scope)
- scan-build (6.0):
  - 4 warnings:
    - 1 false positive memory leak (confusion due to gcc cleanup attribute)
    - 1 false positive dead assignment (confusion due to gcc cleanup attribute)
    - 2 warnings about passing possible NULL pointers to functions which expect
      non-NULL values
      - 1 for creat(), 1 for symlink()
      - both should be impossible since current call sites never provide the
        preconditions to allow these to occur
      - so both are false positives
        - would be useful to have some assertions to convey this impossible
          state

So the only real concerns I have is that the system-level tests do not seem to
be integrated with the package build process in any way, and that it is setuid
root which does not seem necessary as our kernels enable unprivileged user
namespaces. So I am happy to ACK this from the security team if both of these
can be investigated and resolved (ie. add the tests to the build process and
drop the adding of setuid permission during packaging).

It would also be worthwhile looking into adding an AppArmor profile to try and
provide additional hardening - and in particular if there is a good reason to
keep it setuid then I would definitely recommend adding an AppArmor profile to
ensure that we restrict a setuid bubblewrap from only doing the intended
functionality and not stepping outside its bounds (ie. messing with system
AppArmor policy etc. if it were somehow to be compromised).