Comment 42 for bug 1854362

I reviewed targetcli-fb 1:2.1.51-0ubuntu1 as checked into focal. This
shouldn't be considered a full audit but rather a quick gauge of
maintainability.

targetcli-fb is a python package for configuring and managing the LIO
(Linux IO) generic SCSI target.

- CVE History:
  - None
- Build-Depends
  - No security sensitive build-depends:
    - debhelper, dh-python, python3-all, python3-configshell-fb,
      python3-gi, python3-rtslib-fb, python3-setuptools, python3-six
- pre/post inst/rm scripts
  - only auto-generated ones from dh_python3/dh_installsystemd
- No init scripts
- 1 systemd unit for the targetclid daemon
- No dbus services
- No setuid binaries
- binaries in PATH
  - /usr/bin/targetcli
  - /usr/bin/targetclid
- No sudo fragments
- No polkit files
- No udev rules
- No autopkgtests or unit tests
  - This makes it very difficult for the security team to ensure any
    possible security updates do not introduce regressions
- No cron jobs
- Build logs:
  - No significant errors / warnings

- No processes spawned
- Memory management is python
- File IO
  - /var/run/targetclid.sock is world-writable (0o666) so anyone can
    connect to it and there is no authentication done on the user who is
    interacting with targetclid via this socket - as such an unprivileged
    user can connect to it and send commands to targetclid which will
    execute them with no privilege checks. This is likely a security
    vulnerability. The permissions on this socket path should be explicitly
    set so that this is only writable by owner/group and not others,
    ie. 660 rather than the current 666. Since this is generally created by
    systemd, adding SocketMode=0660 to the targetclid.socket systemd unit
    should be sufficient. This has been reported upstream at
    https://github.com/open-iscsi/targetcli-fb/issues/162
  - targetclid uses the hardcoded file-path /tmp/data.txt for handling
    interaction with clients - this is a potential security vulnerability
    since if a client creates a symlink at /tmp/data.txt to some root owned
    file, targetclid would write it's own data to that target file - I
    notice this has already been fixed upstream via
    https://github.com/open-iscsi/targetcli-fb/pull/156 /
    https://github.com/open-iscsi/targetcli-fb/commit/23877ab4afbf0c2fe4092936261d92d7b7fbff11
    and so this should be patched to avoid any possible security issue as a
    result
  - Also uses hard-coded path to /var/run/targetclid.pid
  - Uses the config file ~/.targetcli
  - saveconfig commands allows to specify any resulting filename so can be
    used to overwrite arbitrary files on the system - as such there should
    probably be stricter checks on the target filename OR that targetcli
    can only ever be run as a regular user (the current location of the )
  - restoreconfig command will read from any specified config file without
    checking ownership etc so again any client to targetclid should be
    considered trusted
- Logging
  - Is via ConfigShell (from python-configshell-fb) and looks fine
- Environment variable usage
  - targetclid uses TARGETCLI_HOME to override path of ~/.targetcli and
    LISTEN_PID to support systemd-based socket activation - since the
    targetcli client first tries to create the lock file when launched, and
    this is only writable by root hence targetcli must be run as root
    anyway so this can't really be abused

- No use of privileged functions
- No use of cryptography / random number sources etc
- Use of temp files
  - See above comment about /tmp/data.txt - this should be resolved before
    being promoted to main
- No use of networking
- No use of WebKit
- No use of PolicyKit

- No significant cppcheck results
- Unknown if any significant Coverity results (waiting on Coverity license
  renewal)
- No significant shellcheck results
- No significant bandit results

As mentioned above, SocketMode should likely be specified in the systemd
socket unit (waiting on upstream to respond to the bug report). Also the
targetcli client checks whether it is running as root and if not
potentially disables some commands - this is not sufficient to stop
targetclid running privileged commands on behalf of a client - instead,
targetclid should check the rights of a client via `SCM_CREDENTIALS` so
that it cannot be tricked into performing operations on behalf of an
unprivileged user - reported upstream as
https://github.com/open-iscsi/targetcli-fb/issues/163

Security team NACK for promoting targetcli-fb to main for now due to these
two potential security issues. If at least the socket permissions can be
fixed this should mitigate the impact of the second issue (permission check
in the client) - however, ideally the daemon would be stricter on checking
permissions of clients as well and in that case I would be a bit happier to
ACK this.