Comment 18 for bug 1948748

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I reviewed swtpm 0.6.1-0ubuntu5 as checked into jammy. This shouldn't be
considered a full audit but rather a quick gauge of maintainability. I
especially didn't audit the fuse/cuse interface, nor suitability of the
software tpm to replace a hardware tpm.

And especially especially I didn't investigate whether cross-guests
accesses are possible, intentional, allowed, disallowed, etc.

swtpm is a software tpm 'device' intended to allow virtualized guests to
use tpm services without access to a real tpm device.

- CVE History:
  - two CVEs, very little information in our database on one; the other
    was typical C memory problems. There's probably more.
- Build-Depends?
  - libtool, debhelper-compat (= 10), libtpms-dev, libfuse-dev,
    libglib2.0-dev, libjson-glib-dev, libgmp-dev, expect, libtasn1-dev,
    socat, python3-twisted, gnutls-dev, gnutls-bin, libssl-dev, net-tools,
    gawk, softhsm2, libseccomp-dev
  - uses *both* openssl and gnutls. Odd choice.
- pre/post inst/rm scripts?
  - create swtpm user, group, /var/lib/swtpm-localca directory
  - not cleaned up in postrm
- init scripts?
  - none
- systemd units?
  - none
- dbus services?
  - none
- setuid binaries?
  - none
- binaries in PATH?
  - swtpm, swtpm_bios, swtpm_cert, swtpm_ioctl, swtpm_setup
- sudo fragments?
  - none
- polkit files?
  - none
- udev rules?
  - none
- unit tests / autopkgtests?
  - some are run during the build; I didn't inspect them
- cron jobs?
  - none
- Build logs:
  - pretty clean

- Processes spawned?
  - yes; seemed safe, except password / keys being passed to children in
    environment variables
- Memory management?
  - well.. it's got a *lot* of crafty buffer management even among C
    programs. I'm sure there's more flaws in there. Some of these tools
    might be better done in a safer language like golang or rust.
- File IO?
  - mostly looked good, under control of callers. I'm not a huge fan of
    tmpfiles made with template "XXXXXX", but that's not really a security
    concern, just annoying.
- Logging?
  - logging looked good.
- Environment variable usage?
  - Keys are passed to child processes with environment variables. This
    might be a problem.
- Use of privileged functions?
  - Moderate amounts, no problems spotted
- Use of cryptography / random number sources etc?
  - Extensive, I didn't carefully inspect.
- Use of temp files?
  - safe, but perhaps annoying with "XXXXXX" templates.
- Use of networking?
  - yes; unix domain sockets, tcp sockets, and I'm worried about the TLV
    content parsing. Bugs filed to ask for feedback.
- Use of WebKit?
  - None
- Use of PolicyKit?
  - None

- Any significant cppcheck results?
  - one memory leak, probably only a few dozen bytes each time through
- Any significant Coverity results?
  - potentially, thanks for the fix
- Any significant shellcheck results?
  - none
- Any significant bandit results?
  - none

Security team ACK for promoting swtpm to main.

I think a lot of this software would benefit from being written in an
easier, safer, language; obviously portions of it are probably best done
in C, but there's got to be more bugs in here.

Here's some notes I took while reading it, in case they're helpful:

swtpm_setup/swtpm_setup.c gl_LOGFILE -- TOCTTOU stat() vs fopen(), besides it's
opened WAY earlier when logging errors?

swtpm/swtpm_nvfile.c SWTPM_NVRAM_SetStateBlob -- data supplied on fd
allows complete memory access

https://github.com/stefanberger/swtpm/issues/678
integer wraparound in tlv functions

src/swtpm/swtpm_nvfile.c SWTPM_NVRAM_PrependHeader() integer wraparound
with length param?

src/utils/swtpm_utils.c read_file_lines() accidentally quadratic memory
allocation and pointer copying, in the number of lines of input file

src/swtpm/cuse_tpm.c swtpm_cuse_main() opening a device file to find out
if it exists is odd -- is it still the case that opening a tape device
could wait indefinitely? stat() might work better.

https://github.com/stefanberger/swtpm/issues/679
integer wraparound in tcp port parsing

seccomp rules! yay

samples/swtpm_localca.c create_localca_cert() -- passing a password via
SWTPM_ROOTCA_PASSWORD environment variable leaks it to other processes
on the system via /proc/pid/environ