[MIR] swtpm

Bug #1948748 reported by Steve Langasek
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
autogen (Ubuntu)
Won't Fix
Undecided
Unassigned
gnutls28 (Ubuntu)
Won't Fix
Undecided
Unassigned
libtpms (Ubuntu)
Fix Released
High
Unassigned
libvirt (Ubuntu)
Fix Released
Undecided
Unassigned
swtpm (Ubuntu)
Fix Released
High
Unassigned

Bug Description

[Availability]
Available in universe in jammy.

[Rationale]
Needed in order to provide TPM functionality to VMs through kvm/libvirt; should be a Recommends: of qemu-system-x86

[Security]
Several security bugs found and fixed in libtpms this year http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=libtpms

http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3746 currently remains unfixed in the version present in jammy (DoS bug).

[Quality assurance]
Limited history: package not present in Debian, and only in Ubuntu since jammy.

[UI standards]
N/A

[Dependencies]
swtpm and libtpms; no further dependencies outside of main.

[Standards compliance]
OK

[Maintenance]
To be maintained by the Foundations Team.

[Background information]
N/A

Related branches

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi,
thanks Steve for the work on this!

While it is true that it is "new in the Archive" it isn't entirely new to the wider ecosystem.
Plenty of people have used it from [1] and similar PPAs since then.
Some others use snaps [2]
But nowadays also snaps with way more user-base and security checks already use it directly embedded like LXD [3].

So it really isn't that much "from scratch" as it might seem at first :-)

Finally it might be worth to add [4] to background info for people unaware what this is about.

Starting to have a look ...

[1]: https://launchpad.net/~stefanberger/+archive/ubuntu/swtpm
[2]: https://snapcraft.io/swtpm-mvo
[3]: https://snapcraft.io/lxd
[4]: https://github.com/stefanberger/swtpm/wiki

Changed in swtpm (Ubuntu):
assignee: nobody → Christian Ehrhardt  (paelzer)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote (last edit ):
Download full text (6.0 KiB)

Review for Package: swtpm

The package is overall in good shape, already has plenty of users and use-cases
despite being rather new to the archive. Nevertheless some polishing seems
to be needed.

This does need a security review - even more so for any potential guest->host
attack surface, so I'll assign ubuntu-security

To make quick progress I'll already pass it on to security despite that we
are waiting for these mentioned improvements/cleanups.

MIR team ACK, under the condition of fulfillling the outlined required
and at least looking at recommended TODOs.
- List of specific binary packages to be promoted to main: swtpm, swtpm-tools
- Specific binary packages built, but NOT to be promoted to main: none

Required TODOs:
- Please cut the dependency of swtpm-tools to gnutls-bin as discussed
  as it would otherwise imply more MIRs.
- libtpms-dev doesn't exist on ppc64el and thereby IMHO blocks too many
  important use cases from being generally working. Please investigate to
  add that and/or explain why this shall be considered not a problem.
- The autopkgtest suite needs to pass (actually run) on arm64 (stalled
  by long queue)

Recommended TODOs:
- While the lib is internal, .symbols tracking usually is cheap and protects
  even internal libs from some mistakes, consider adding it.
- Version 0.7.0 seems rather close please update it later this cycle
  https://github.com/stefanberger/swtpm/issues/587
- The package should get a team bug subscriber before being promoted
  Right now I do not see it subscribed by "foundations-bugs" yet
- evaluate the possibility and impact of having "tcsd" in the build environment
- Have a look at bug 1949155 if you want to change anything for it

Already completed TODOs:
- Please fixup the user/group creation (see bug 1949060)

[Duplication]
There is no other package in main providing the same functionality.

[Dependencies]
OK:
- no other Dependencies to MIR due to this (This = bin:swtpm)
  - checked with check-mir
  - not listed in seeded-in-ubuntu
  - none of the built reverse-depends are in universe
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
  more tests now.

Problems:
- Dependencies of swtpm-tools:
  - swtpm-tools would depend on gnutls-bin which is a binary not in main
    from src:gnutls28 in main.
  - This in turn depends pkg:libopts25 on from autogen (universe)
  After chatting with Steve, we want to keep swtpm-tools to be promoted (as
  it is needed by e.g. libvirt), but cut the dependency of swtpm-tools
  to gnutls-bin.

[Embedded sources and static linking]
- no embedded source present
- no static linking
- does not have odd Built-Using entries
- not a go package, no extra constraints to consider in that regard

[Security]
OK:
- does not use webkit1,2
- does not use lib*v8 directly
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does not run a daemon as root, but is often run with too high privilegded
  (user tss), which is why we have spawned bug 1948880 ...

Read more...

Changed in swtpm (Ubuntu):
assignee: Christian Ehrhardt  (paelzer) → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI - via discussion we found that swtpm-tools will be needed, that either needs to get the dependencies adapted (to not depend on gnutls-bin or have them not depend on libopts25) or to promote those as well.

I'll re-evaluate swtpm with that in mind and update my former post (probably tomorrow).

@Steve as you are driving this case would you mind looking into the usage and dependencies for gnutls-bin if we will tackle this at the dependency level or if we need to MIR more?
I'll add further bug tasks in that regard and assign you to get this started.

Changed in gnutls28 (Ubuntu):
status: New → Incomplete
Changed in autogen (Ubuntu):
status: New → Incomplete
assignee: nobody → Steve Langasek (vorlon)
Changed in gnutls28 (Ubuntu):
assignee: nobody → Steve Langasek (vorlon)
Changed in swtpm (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → Christian Ehrhardt  (paelzer)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Interim state update in regard to swtpm-tools, gnutls-bin and libopts25.
Discussing with Steve it seems that - for now - the best option seems to be:
- to keep the ssl/tls lib relation -both seem complex and needed for various reasons e.g.
  Steve: "a lot of low-level gnutls operations, I assume because there were no available
          equivalents in libssl"
- but at the same time we seem to be able to cut the ties to gnutls-bin by
  replacing the calls to certtool (form gnutls-bin) to sometrhing openssl based, that seems
  possible and is on Steve.
- This would eliminate the need for gnutls28 + autogen MIRs

I'll update my post with the re-review content adapted to this outcome

Changed in autogen (Ubuntu):
status: Incomplete → Won't Fix
Changed in gnutls28 (Ubuntu):
status: Incomplete → Won't Fix
assignee: Steve Langasek (vorlon) → nobody
Changed in autogen (Ubuntu):
assignee: Steve Langasek (vorlon) → nobody
Changed in swtpm (Ubuntu):
assignee: Christian Ehrhardt  (paelzer) → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote (last edit ):

@Steve - while waiting for security review, as time permits please have a look at the list of required and recommended tasks for swtpm (=> see comment 2 - https://bugs.launchpad.net/ubuntu/+source/gnutls28/+bug/1948748/comments/2)

Changed in libtpms (Ubuntu):
assignee: nobody → Christian Ehrhardt  (paelzer)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :
Download full text (4.5 KiB)

Review for Package: libtpms

[Summary]
As swtpm this seems generally well packaged, and already has plenty of users.
It needs a bit polishing here and there but seems close to be promotable.

MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.

This does need a security review, so I'll assign ubuntu-security.

List of specific binary packages to be promoted to main: libtpms0

Required TODOs:
- please package the current v0.9
  https://github.com/stefanberger/libtpms/releases/tag/v0.9.0
- Fix the ppc64 FTBFS
  https://launchpadlibrarian.net/557789130/buildlog_ubuntu-impish-ppc64el.libtpms_0.8.2-1ubuntu1_BUILDING.txt.gz
- Track and resolve https://github.com/stefanberger/libtpms/issues/215
  to ensure this works well with openssl3.0 in Ubuntu 22.04

Recommended TODOs:
- Right now it has no autopkgtest, maybe - like swtpm this could at least run
  the build time tests to spot things as early as dependency-update instead of
  "on the next rebuild"?
- The package should get a team bug subscriber before being promoted

[Duplication]
There are the tpm2-tss related packages, but those are for consumption of
real (or emulated) TPMs. No other package in main providing the same
functionality of emulating/mocking/faking a TPM in software.

[Dependencies]
OK:
- no other Dependencies to MIR due to this (just libc and ssl)
  - checked with check-mir
  - not listed in seeded-in-ubuntu
  - none of the built reverse-depends are in universe
- no -dev/-debug/-doc packages that need exclusion (-dev exists but has no
  aggressive deps that would be a problem)
- No dependencies in main that are only superficially tested requiring
  more tests now.

Problems: None

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have odd Built-Using entries
- not a go package, no extra constraints to consider in that regard

Problems: None

[Security]
OK:
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port/socket
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does parse data formats
- does deal with security attestation (secure boot, tpm, signatures)
- history of CVEs does look concerning
- Also the intended use case just yells "this needs a seucrity review"

[Common blockers]
OK:
- does have a test suite that runs at build time
  - test suite fails will fail the build upon error.
- additional testing by usage in swtpm and its tests is indirectly existing
- no new python2 dependency

Problems:
- does not have a non-trivial test suite that runs as autopkgtest
- does FTBFS currently (on PPC64)
  => https://launchpadlibrarian.net/557789130/buildlog_ubuntu-impish-ppc64el.libtpms_0.8.2-1ubuntu1_BUILDING.txt.gz
  I do not see why we would not need this on this arch, so for equivalency we
  have to fix it before promoting it

[Packaging red flags]
OK:
- Ubuntu does carry a delta, but it is reasonable and maintenance under
  co...

Read more...

Changed in libtpms (Ubuntu):
assignee: Christian Ehrhardt  (paelzer) → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI to keep the info on the bug complete, Steve is working to remove the gnutls dependency.
See: https://github.com/stefanberger/swtpm/pull/620

Revision history for this message
Steve Langasek (vorlon) wrote : Re: [Bug 1948748] Re: [MIR] swtpm
Download full text (3.9 KiB)

On Fri, Oct 29, 2021 at 07:04:51AM -0000, Christian Ehrhardt  wrote:
> Review for Package: libtpms

> Required TODOs:
> - please package the current v0.9

This has been uploaded.

> - Fix the ppc64 FTBFS
> https://launchpadlibrarian.net/557789130/buildlog_ubuntu-impish-ppc64el.libtpms_0.8.2-1ubuntu1_BUILDING.txt.gz

I would argue that, given that this is a virtual implementation of hardware
that is not present on the ppc64el architecture, portability to ppc64el
should not be a blocker for MIR. I looked into the failure, but it's
non-trivial; as far as I can tell the current failure is a toolchain bug,
and if I work around it, there is another build failure (Debian bug #997969)
which is also toolchain weirdness. Neither issue indicates a problem with
the quality of the code, so I don't think this should block support of the
package on architectures where it is currently supportable.

> - Track and resolve https://github.com/stefanberger/libtpms/issues/215
> to ensure this works well with openssl3.0 in Ubuntu 22.04

A test build of libtpms 0.9.0 against openssl 3 succeeds (0.8.2 fails).

> Recommended TODOs:
> - Right now it has no autopkgtest, maybe - like swtpm this could at least run
> the build time tests to spot things as early as dependency-update
> instead of "on the next rebuild"?

I've added an autopkgtest now; it's a simple 'make check' which
unfortunately means it builds and tests against a rebuilt library instead of
against the binaries from the archive, but I think this is better than
nothing as a first pass.

> - The package should get a team bug subscriber before being promoted

Foundations is now subscribed.

> Problems:
> - important open bugs (crashers, etc) in Debian or Ubuntu
> IMHO there is one worthile to track (but no immediate action needed)
> FIPS: https://github.com/stefanberger/libtpms/issues/51

Well, as far as I'm aware Canonical has no product for FIPS certification of
a virtualization stack, so I don't see any reason that FIPS for libtpms
would be "important" for us.

On Thu, Oct 28, 2021 at 12:35:24PM -0000, Christian Ehrhardt  wrote:
> FYI - via discussion we found that swtpm-tools will be needed, that
> either needs to get the dependencies adapted (to not depend on gnutls-
> bin or have them not depend on libopts25) or to promote those as well.

> I'll re-evaluate swtpm with that in mind and update my former post
> (probably tomorrow).

swtpm 0.6.1-0ubuntu4 now uploaded to trade the gnutls-bin dependency for
openssl.

On Thu, Oct 28, 2021 at 10:07:58AM -0000, Christian Ehrhardt  wrote:
> Required TODOs:
> - Please fixup the user/group creation (see bug 1949060)

This is done.

> - libtpms-dev doesn't exist on ppc64el and thereby IMHO blocks too many
> important use cases from being generally working. Please investigate to
> add that and/or explain why this shall be considered not a problem.

Discussed above.

> - The autopkgtest suite needs to pass (actually run) on arm64 (stalled
> by long queue)

This has passed now (several times, in fact).

> Recommended TODOs:
> - While the lib is internal, .symbols tracking usually is cheap and protects
> even internal libs from some mistakes, co...

Read more...

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> > - Fix the ppc64 FTBFS
> > https://launchpadlibrarian.net/557789130/buildlog_ubuntu-impish-ppc64el.libtpms_0.8.2-1ubuntu1_BUILDING.txt.gz
...
> Neither issue indicates a problem with
> the quality of the code, so I don't think this should block support of the
> package on architectures where it is currently supportable.

Agreed, thanks for having a look at the details

> > Problems:
> > - important open bugs (crashers, etc) in Debian or Ubuntu
> > IMHO there is one worthile to track (but no immediate action needed)
> > FIPS: https://github.com/stefanberger/libtpms/issues/51
>
> Well, as far as I'm aware Canonical has no product for FIPS certification of
> a virtualization stack, so I don't see any reason that FIPS for libtpms
> would be "important" for us.

Yeah, as I said this was "track (but no immediate action needed)" and
mostly meant as a hint for awareness if we ever need to work on it.

> > Recommended TODOs:
> > - While the lib is internal, .symbols tracking usually is cheap and protects
> > even internal libs from some mistakes, consider adding it.
>
> I disagree that this is worthwhile; any changes to the symbols of an
> internal library that cause us to have to make changes to a symbols file are
> busywork.

It has helped in some odd cases, but yeah if you prefer iti without
that or consider it busy-work then I'm totally ok to leave it as is
(that is why it was only in the recommended section).

> > - evaluate the possibility and impact of having "tcsd" in the build
> environment
>
> The problem is that the trousers package is itself buggy and frequently
> fails to install, so build-depending on it for the testsuite is not an
> improvement in QA.

Ok, still thanks for checking that

>
> I believe that addresses everything except for the security review.

Thank you for resolving all the little itches and scratches on this case!
I agree, I think we are good except for the security review.

MIR team ACK (all mandatory requests from the initial review are fulfilled).

Revision history for this message
Christian Ehrhardt  (paelzer) wrote (last edit ):

Hi,
in preparation for prioritizing various MIR items blocked on security review I was setting those items of the server Team that are essential for 22.04 to "Critical + Milestone @22.04-feature-freeze".
This is a foundation item, but I think it qualifies for the same, if you agree I'd ask to please set the status accordingly.

Lukas Märdian (slyon)
Changed in libtpms (Ubuntu):
milestone: none → ubuntu-22.04-beta
Changed in swtpm (Ubuntu):
milestone: none → ubuntu-22.04-beta
Changed in libtpms (Ubuntu):
importance: Undecided → High
Changed in swtpm (Ubuntu):
importance: Undecided → High
Revision history for this message
Seth Arnold (seth-arnold) wrote :

Can an update for this issue be incorporated before a release?

https://github.com/stefanberger/swtpm/security/advisories/GHSA-2qgm-8xf4-3hqw

It's something Coverity spotted in our version, and I was very pleased to see that there's already an advisory for this :) saving me some effort.

Thanks

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks for the ping Seth
The fix for that is in:

commit 0f75b4baf272fd08e39d636e6c1f37585d752e1e
Author: Stefan Berger <email address hidden>
Date: Wed Feb 16 11:17:47 2022 -0500

    swtpm: Check header size indicator against expected size (CID 375869)

But in fact swtpm has stable branches and 0.6.3 seems to be a safe fix-only update to what we have.
I'll try to prep that to then have Foundations sign-off and upload.

Bonus: the recent upload for the apparmor profiles has failed tests (despite all pre-checks) and needs to be resolved anyway.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

List of these fix-only changes:

version 0.6.3:
  - swtpm:
    - Do not chdir(/) when using --daemon
  - swtpm-localca:
    - Re-implement variable resolution for swtpm-localca.conf
  - tests:
    - Use ${WORKDIR} in config files to test env. var replacement
  - man:
    - Add missing .config directory to path description when using ${HOME}
  - build-sys:
    - Add probing for -fstack-protector
    - configure: Fix typo TPM2 -> TMP2

version 0.6.2:
  - swtpm:
    - Check header size indicator against expected size (CVE-2022-23645)
  - swtpm-localca:
    - Test for available issuercert before creating CA
  - swtpm_setup:
    - Report stderr as returned by external tool (swtpm-localcal)
    - Fix exit code on error to be '1'.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

PPA: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4809

The former test-hang that affects current proposed migration is still present.
I'll debug that

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I have prepared the changes requested by Seth, stable fixes and an apparmor/test fix.
Can be reviewed here:
=> https://code.launchpad.net/~paelzer/ubuntu/+source/swtpm/+git/swtpm/+merge/417221

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI - I got positive feedback for my MR and in general from Foundations, I'll upload this to Jammy now. This will address bug 1950631, the security request by sarnold and a general stabilization update. => 0.6.3-0ubuntu1

Revision history for this message
Christian Ehrhardt  (paelzer) wrote (last edit ):

FYI the requested security fixes (see above), the apparmor hardening (bug 1950631) and the update to 0.6.3 to stabilize all just migrated to jammy-release (=> https://launchpad.net/ubuntu/+source/swtpm/0.6.3-0ubuntu1).

We should be back to "just" waiting for the security review and hopefully no major findings.

Revision history for this message
Seth Arnold (seth-arnold) wrote :
Download full text (4.1 KiB)

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...

Read more...

Changed in swtpm (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
status: New → In Progress
Revision history for this message
Seth Arnold (seth-arnold) wrote :

libtpms might need to pull in https://github.com/stefanberger/libtpms/commit/d78a4520ba3157087e1e438b519618f26d85fae3 -- I think without this fix, the following algorithms may not work right:

#define ALG_KDF1_SP800_56A_VALUE 0x0020
#define TPM_ALG_KDF1_SP800_56A (TPM_ALG_ID)(ALG_KDF1_SP800_56A_VALUE)
#define ALG_KDF2_VALUE 0x0021
#define TPM_ALG_KDF2 (TPM_ALG_ID)(ALG_KDF2_VALUE)
#define ALG_KDF1_SP800_108_VALUE 0x0022
#define TPM_ALG_KDF1_SP800_108 (TPM_ALG_ID)(ALG_KDF1_SP800_108_VALUE)
#define ALG_ECC_VALUE 0x0023
#define TPM_ALG_ECC (TPM_ALG_ID)(ALG_ECC_VALUE)
#define ALG_SYMCIPHER_VALUE 0x0025
#define TPM_ALG_SYMCIPHER (TPM_ALG_ID)(ALG_SYMCIPHER_VALUE)
#define ALG_CAMELLIA_VALUE 0x0026
#define TPM_ALG_CAMELLIA (TPM_ALG_ID)(ALG_CAMELLIA_VALUE)
#define ALG_SHA3_256_VALUE 0x0027
#define TPM_ALG_SHA3_256 (TPM_ALG_ID)(ALG_SHA3_256_VALUE)
#define ALG_SHA3_384_VALUE 0x0028
#define TPM_ALG_SHA3_384 (TPM_ALG_ID)(ALG_SHA3_384_VALUE)
#define ALG_SHA3_512_VALUE 0x0029
#define TPM_ALG_SHA3_512 (TPM_ALG_ID)(ALG_SHA3_512_VALUE)
#define ALG_CMAC_VALUE 0x003F
#define TPM_ALG_CMAC (TPM_ALG_ID)(ALG_CMAC_VALUE)
#define ALG_CTR_VALUE 0x0040
#define TPM_ALG_CTR (TPM_ALG_ID)(ALG_CTR_VALUE)
#define ALG_OFB_VALUE 0x0041
#define TPM_ALG_OFB (TPM_ALG_ID)(ALG_OFB_VALUE)
#define ALG_CBC_VALUE 0x0042
#define TPM_ALG_CBC (TPM_ALG_ID)(ALG_CBC_VALUE)
#define ALG_CFB_VALUE 0x0043
#define TPM_ALG_CFB (TPM_ALG_ID)(ALG_CFB_VALUE)
#define ALG_ECB_VALUE 0x0044
#define TPM_ALG_ECB (TPM_ALG_ID)(ALG_ECB_VALUE)

Thanks

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks Seth,
these tpm things are rather new so exposure to testing is low in general.
You are right, but I think we do not only need what you spotted.

In fact while reading this I found that upstream (Thanks Stefan) maintains great stable branches.
I think we should consider taking in
- https://github.com/stefanberger/libtpms/releases/tag/v0.9.1
- https://github.com/stefanberger/libtpms/releases/tag/v0.9.2
- https://github.com/stefanberger/libtpms/releases/tag/v0.9.3
- plus the patch you've found
- a ppc64 build fix from debian / upstream #297

I'll try to prepare this so that someone else review and ack (or disagree). None of this needs a FFe IMHO all just fixes - we'll want to hold back the actual upload until beta-freeze is over, but that does not prevent to prepare the fixes.

In addition to reduce delta one day I filed:
- https://github.com/stefanberger/libtpms/pull/305

I have prepared that in:
PPA: https://launchpad.net/~paelzer/+archive/ubuntu/libtpms-jammy-0.9.3
MR: https://code.launchpad.net/~paelzer/ubuntu/+source/libtpms/+git/libtpms/+merge/417854

P.S. this will also resolve the fact that it is - so far - missing on ppc64

Please anyone review and/or consume it from that PPA and let me know if you are +1 on an upload of that.

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

Changed in libtpms (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thank you Seth, so this seems all ready once the fixed version of libtpms landed in jammy.

As mentioned before I have already prepared this in
  https://code.launchpad.net/~paelzer/ubuntu/+source/libtpms/+git/libtpms/+merge/417854
It has acks there for the changes so far and should be good.
I'll now check the recent issues you opened to pull in their fixes as well.

https://github.com/stefanberger/libtpms/issues/304
  fixed in stable banch
  https://github.com/stefanberger/libtpms/commit/897c8f869754d01639741cecf9fee8f0a7b5d740
  not yet in a 0.9.4 (not tagged yet)
  I have added it on top of the planned upload for jammy

https://github.com/stefanberger/libtpms/issues/310
  fixed in master branch
  https://github.com/stefanberger/libtpms/commit/ec873cf8a2b594a4bb504429c390b880050e4aaf
  agreed to be false positive, not needed in jammy

https://github.com/stefanberger/libtpms/issues/311
  fixed in master branch
  https://github.com/stefanberger/libtpms/commit/fbf413ec862fa93879502207d279b00fddc0ef98
  agreed to be false positive, not needed in jammy

https://github.com/stefanberger/libtpms/issues/313
  No fix/discussion yet, we can pull this in later as/if needed.

Updated both
MP: https://code.launchpad.net/~paelzer/ubuntu/+source/libtpms/+git/libtpms/+merge/417854
PPA: https://launchpad.net/~paelzer/+archive/ubuntu/libtpms-jammy-0.9.3/+packages

If there are no late issues I'll upload to jammy later.
That should then complete the requirements to promote all of this in Jammy.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Rebuilds, Sanity tests and autopkgtests with the new version worked well.
Uploading libtpms to get this ready for promotion to main next week.

Note: After it is in jammy we can also hit rebuild on [1] to complete architecture parity of swtpm.

[1]: https://launchpad.net/ubuntu/+source/swtpm/0.6.3-0ubuntu1/+build/23492449

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

[1] as well as automatically already [2] built.
Let us see over the weekend if all test and migrate fine and then we can consider seeding/depending on them to promote it.

[1]: https://launchpad.net/ubuntu/+source/libtpms/0.9.3-0ubuntu1
[2]: https://launchpad.net/ubuntu/+source/swtpm/0.6.3-0ubuntu1/+build/23492449

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

libtpms [1] and even the auto unblocked ppc build of swtpm [2] migrated to jammy-release in universe.

For both we have the MIR ack, Security ack and all required todos implemented.
Time to seed or depend on this accordingly.

The one I know that was planned was a recommends from libvirt which can utilize it to provide swtpm based functionalities to VMs. Feel free to suggest propose other seeding as well, but this is the one I'm going for now.
Proposed here [3].

[1]: https://launchpad.net/ubuntu/+source/libtpms/0.9.3-0ubuntu1
[2]: https://launchpad.net/ubuntu/+source/swtpm/0.6.3-0ubuntu1
[3]: https://code.launchpad.net/~paelzer/ubuntu/+source/libvirt/+git/libvirt/+merge/418313

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Added a libvirt task to bring in the dependency.

Changed in libvirt (Ubuntu):
status: New → Fix Committed
Revision history for this message
Christian Ehrhardt  (paelzer) wrote (last edit ):

This is now showing in component mismatches:

libtpms: libtpms-dev libtpms0
 MIR: #1948748 (New)
 [Reverse-Depends: Rescued from libtpms (Uploader: paelzer), swtpm (Uploader: paelzer)]
swtpm: swtpm swtpm-tools
 MIR: #1948748 (In Progress)
 [Reverse-Depends: swtpm-tools]
 [Reverse-Recommends: libvirt-daemon-system (MAIN)]

But some more builds and autopkgtests need to conclude ...

Changed in libvirt (Ubuntu):
status: Fix Committed → In Progress
Revision history for this message
Steve Langasek (vorlon) wrote :

Override component to main
libtpms 0.9.3-0ubuntu1 in jammy: universe/misc -> main
libtpms-dev 0.9.3-0ubuntu1 in jammy amd64: universe/libdevel/optional/100% -> main
libtpms-dev 0.9.3-0ubuntu1 in jammy arm64: universe/libdevel/optional/100% -> main
libtpms-dev 0.9.3-0ubuntu1 in jammy armhf: universe/libdevel/optional/100% -> main
libtpms-dev 0.9.3-0ubuntu1 in jammy ppc64el: universe/libdevel/optional/100% -> main
libtpms-dev 0.9.3-0ubuntu1 in jammy riscv64: universe/libdevel/optional/100% -> main
libtpms-dev 0.9.3-0ubuntu1 in jammy s390x: universe/libdevel/optional/100% -> main
libtpms0 0.9.3-0ubuntu1 in jammy amd64: universe/libs/optional/100% -> main
libtpms0 0.9.3-0ubuntu1 in jammy arm64: universe/libs/optional/100% -> main
libtpms0 0.9.3-0ubuntu1 in jammy armhf: universe/libs/optional/100% -> main
libtpms0 0.9.3-0ubuntu1 in jammy ppc64el: universe/libs/optional/100% -> main
libtpms0 0.9.3-0ubuntu1 in jammy riscv64: universe/libs/optional/100% -> main
libtpms0 0.9.3-0ubuntu1 in jammy s390x: universe/libs/optional/100% -> main
swtpm 0.6.3-0ubuntu1 in jammy: universe/misc -> main
swtpm 0.6.3-0ubuntu1 in jammy amd64: universe/misc/optional/100% -> main
swtpm 0.6.3-0ubuntu1 in jammy arm64: universe/misc/optional/100% -> main
swtpm 0.6.3-0ubuntu1 in jammy armhf: universe/misc/optional/100% -> main
swtpm 0.6.3-0ubuntu1 in jammy ppc64el: universe/misc/optional/100% -> main
swtpm 0.6.3-0ubuntu1 in jammy riscv64: universe/misc/optional/100% -> main
swtpm 0.6.3-0ubuntu1 in jammy s390x: universe/misc/optional/100% -> main
swtpm-tools 0.6.3-0ubuntu1 in jammy amd64: universe/misc/optional/100% -> main
swtpm-tools 0.6.3-0ubuntu1 in jammy arm64: universe/misc/optional/100% -> main
swtpm-tools 0.6.3-0ubuntu1 in jammy armhf: universe/misc/optional/100% -> main
swtpm-tools 0.6.3-0ubuntu1 in jammy ppc64el: universe/misc/optional/100% -> main
swtpm-tools 0.6.3-0ubuntu1 in jammy riscv64: universe/misc/optional/100% -> main
swtpm-tools 0.6.3-0ubuntu1 in jammy s390x: universe/misc/optional/100% -> main
26 publications overridden.

Ready for promotion anyway, doing so. libvirt expected to come along as soon as arm64 autopkgtests catch up.

Changed in libtpms (Ubuntu):
status: New → Fix Released
Changed in swtpm (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libvirt - 8.0.0-1ubuntu6

---------------
libvirt (8.0.0-1ubuntu6) jammy; urgency=medium

  * d/control: recommend swtpm-tools (LP: #1948748)

 -- Christian Ehrhardt <email address hidden> Mon, 04 Apr 2022 07:30:15 +0200

Changed in libvirt (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
Martin Pitt (pitti) wrote :

This broke VMs with UEFI, reported as bug 1968131.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers