Comment 21 for bug 1948748

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

I reviewed libtpms 0.9.0-0ubuntu4 as checked into jammy. This shouldn't
be considered a full audit but rather a quick gauge of maintainability. I
certainly didn't carefully review if libtpms is fit for use as a software
TPM. It appears to have been programmed with care and dilligence and
the upstream author responded very quickly to the issues I filed.

- CVE History:
  - five CVEs; three need triage or fixing in jammy according to
    our database: CVE-2021-3446 CVE-2021-3623 CVE-2021-3746
- Build-Depends?
  - debhelper-compat (= 13), dh-exec, gawk, libssl-dev, libtool, pkg-config
- pre/post inst/rm scripts?
  - none
- init scripts?
  - none
- systemd units?
  - none
- dbus services?
  - none
- setuid binaries?
  - none
- binaries in PATH?
  - none
- sudo fragments?
  - none
- polkit files?
  - none
- udev rules?
  - none
- unit tests / autopkgtests?
  - it's got some, I didn't inspect them
- cron jobs?
  - none
- Build logs:
  - pretty clean, but this looked strange:
    [WARNING] Recoverable errors were encountered during 313 of these C/C++ compilation units.

- Processes spawned?
  - none
- Memory management?
  - extensive -- it looked good, but there sure is a lot of it
- File IO?
  - some, looked good
- Logging?
  - looked careful
- Environment variable usage?
  - TPM_PATH to set the path for files
- Use of privileged functions?
  - none
- Use of cryptography / random number sources etc?
  - extensive; very intricate low-level details; it all seemed normal
    enough, but it's extremely niche.
  - rand() could be used as a fallback if the openssl random number
    generator fails. That's not ideal but unlikely to be an issue in
    usual practice.
- Use of temp files?
  - none
- Use of networking?
  - none
- Use of WebKit?
  - none
- Use of PolicyKit?
  - none

- Any significant cppcheck results?
  - minor, NULL pointer dereference crash in corner case
- Any significant Coverity results?
  - minor, some false positives, some reasonable findings; Stefan
    responded very quickly.
- Any significant shellcheck results?
  - in test cases, I didn't check
- Any significant bandit results?
  - none

This is very complex code, we'll need upstream's help for anything beyond
trivial issues. It was a pleasure to raise issues with Stefan, he
approached the issues I filed quickly and eagerly. I expect it'll be easy
to work with him in the future as necessary.

Security team ACK for promoting libtpms to main. It'd be a favour to us to
pop together one more upload before release if those CVEs actually still
apply to our package.

I didn't take many notes while reviewing that stayed on my computer; only
the DES finding. That's a bit strange, but meh, how important is 3des in
TPM-land these days?

/src/tpm2/crypto/openssl/TpmToOsslDesSupport.c DES_set_key_unchecked() is
apparently unsafe, should use the checked version instead

https://github.com/stefanberger/libtpms/issues/304
https://github.com/stefanberger/libtpms/issues/310
https://github.com/stefanberger/libtpms/issues/311
https://github.com/stefanberger/libtpms/issues/313