Merge ~lvoytek/ubuntu/+source/swtpm:swtpm-lp1950631-add-apparmor-jammy into ubuntu/+source/swtpm:ubuntu/devel

[Lena Voytek]

Added an apparmor profile to /usr/bin/swtpm for additional protection. Confirmed it works on its own and with libvirt, QEMU, and virt-manager. Tested with Windows 11 guest.

ppa: ppa:lvoytek/swtpm-apparmor-profile-jammy

Manual Tests on Jammy:

Runing help and version:

$ swtpm --help
$ swtpm --version

Using QEMU:

$ /usr/share/swtpm/swtpm-create-user-config-files
$ qemu-img create -f qcow2 win11.img 64G
$ mkdir /tmp/emulated_tpm
$ swtpm socket --tpmstate dir=/tmp/emulated_tpm --ctrl type=unixio,path=/tmp/emulated_tpm/swtpm-sock --log level=20 --tpm2 &
$ sudo qemu-system-x86_64 -hda win11.img -boot d -m 4096 -enable-kvm -chardev socket,id=chrtpm,path=/tmp/emulated_tpm/swtpm-sock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-tis,tpmdev=tpm0 -cdrom ~/Downloads/Win11_English_x64v1.iso

Using virt-manager

> Open virt-manager
> Click New Virtual Machine button

Step 1:
> Select "Local install media (ISO image or CDROM)
> Click Forward

Step 2:
> Click Browse and find Windows 11 iso
> Select "Automatically detect from the installation media / source"
> Click Forward

Step 3:
> Use >= 4096 MiB for Memory
> Use >= 2 CPUs
> Click Forward

Step 4:
> Select "Enable storage for this virtual machine"
> Use >= 70 GiB for storage size
> Click Forward

Step 5:
> Select "Customize configuration before install"
> Click Finish

Config Screen:
> For Overview > Firmware select UEFI x86_64: /usr/share/OVMF/OVMF_CODE_4M.secboot.fd
> For Boot Options select "SATA CDROM 1" and move it to top

> Click Add Hardware
> Select TPM with Model "TIS"

> Click "Begin Installation"

[Serge Hallyn (serge-hallyn)]

Hi,

this looks worthwhile, however it looks like as is the profile would cause me trouble. We have a project making heavy use of swtpm with kvm (not libvirt) with custom profiles. I assume that allowing full access to ${HOME} would make you uncomfortable?

[Lena Voytek (lvoytek)]

> Hi,
>
> this looks worthwhile, however it looks like as is the profile would cause me
> trouble. We have a project making heavy use of swtpm with kvm (not libvirt)
> with custom profiles. I assume that allowing full access to ${HOME} would
> make you uncomfortable?

Hello,

Thanks for the feedback. I think home access can be added since that is an important use case. If that ends up being too nonrestrictive then it can be updated later on. I will update the commits to add it in

[Christian Ehrhardt  (paelzer)]

Hi Serge, long time not seen, glad that you are still around!

@Serge:
On one hand I wonder if the owner restriction as shown now:
  owner @{HOME}/** rwk,
would work for you?
What users do your use case run swtpm as and would that work then?

@Serge;
Furthermore, how common is that setup of yours?
Even if you are unable to talk about details, is it "a very special things unlikely to happen generally around the world", or is it more like "you don't know yet, but in a year everyone will be doing that"?

If it is anywhere close to the former I'd suggest in that case it might make sense to just put a rule int he local override /etc/apparmor.d/local/usr.bin.swtpm

@Lena
Which does bring me to that as a question - IIRC dh_apparmor will automatically add a template for the local overrides in /etc/apparmor.d/local/usr.bin.swtpm - could you confirm that?
If it does not we need to fix that.
And once it does I think we then need an include here in your profile like
  # Site-specific additions and overrides. See local/README for details.
  #include <local/usr.sbin.libvirtd>
Or is that include also added automatically nowadays and I missed that feature?

[Christian Ehrhardt  (paelzer)]

@Lena
looking at this initial profile also a question about
  include <abstractions/libvirt-qemu>
Did your experiments end up with a profile very close to this abstraction so that you added it?
Or was this just the assumption that it needs to be used in libvirt/qemu scope?

Because that abstraction allows a lot that I'd expect swtpm to not need.
After all it is more like

qemu (does qemu things) - needs abstractions/libvirt-qemu
 ^
 |
 v
socket
 ^
 |
 v
swtpm (does swtpm things) does not need abstractions/libvirt-qemu

base, ssl, common used paths, that all LGTM, but the libvirt-qemu abstraction I'm not sure.
Happy to get explained why I'm wrong :-)

I hope this alignment survives, but TL;DR without studying it further the abstractions/libvirt-qemu seems too much for swtpm.

[Lena Voytek (lvoytek)]

> @Lena
> looking at this initial profile also a question about
> include <abstractions/libvirt-qemu>
> Did your experiments end up with a profile very close to this abstraction so
> that you added it?
> Or was this just the assumption that it needs to be used in libvirt/qemu
> scope?
>
> Because that abstraction allows a lot that I'd expect swtpm to not need.
> After all it is more like
>
>
> qemu (does qemu things) - needs abstractions/libvirt-qemu
> ^
> |
> v
> socket
> ^
> |
> v
> swtpm (does swtpm things) does not need abstractions/libvirt-qemu
>
> base, ssl, common used paths, that all LGTM, but the libvirt-qemu abstraction
> I'm not sure.
> Happy to get explained why I'm wrong :-)
>
> I hope this alignment survives, but TL;DR without studying it further the
> abstractions/libvirt-qemu seems too much for swtpm.

That's fair. When building the profile aa-logprof recommended I add it in because swtpm wanted to use multiple permissions from it. In its current state though, when I take the include out and put it in complain mode, it only needs the capability dac_override from it. Do you think it would be better to just add that instead, and should I try to restrict it further?

Thanks!

[Lena Voytek (lvoytek)]

> Hi Serge, long time not seen, glad that you are still around!
>
> @Serge:
> On one hand I wonder if the owner restriction as shown now:
> owner @{HOME}/** rwk,
> would work for you?
> What users do your use case run swtpm as and would that work then?
>
> @Serge;
> Furthermore, how common is that setup of yours?
> Even if you are unable to talk about details, is it "a very special things
> unlikely to happen generally around the world", or is it more like "you don't
> know yet, but in a year everyone will be doing that"?
>
> If it is anywhere close to the former I'd suggest in that case it might make
> sense to just put a rule int he local override
> /etc/apparmor.d/local/usr.bin.swtpm
>
> @Lena
> Which does bring me to that as a question - IIRC dh_apparmor will
> automatically add a template for the local overrides in
> /etc/apparmor.d/local/usr.bin.swtpm - could you confirm that?
> If it does not we need to fix that.
> And once it does I think we then need an include here in your profile like
> # Site-specific additions and overrides. See local/README for details.
> #include <local/usr.sbin.libvirtd>
> Or is that include also added automatically nowadays and I missed that
> feature?

I can confirm that the local usr.bin.swtpm gets created automatically but is a blank file by default. I could modify the install such that it gets created with helpful info though

[Simon Déziel (sdeziel)]

I made a tiny suggestion based on https://gitlab.com/apparmor/apparmor/-/wikis/DeprecateProfilePathName

[Lena Voytek (lvoytek)]

> I made a tiny suggestion based on
> https://gitlab.com/apparmor/apparmor/-/wikis/DeprecateProfilePathName

Thanks for the help! Got the profile name updated

[Christian Ehrhardt  (paelzer)]

> > I hope this alignment survives, but TL;DR without studying it further the
> > abstractions/libvirt-qemu seems too much for swtpm.
>
> That's fair. When building the profile aa-logprof recommended I add it in because swtpm wanted to use multiple permissions from it. In its current state though, when I take the include out and put it in complain mode, it only needs the capability dac_override from it. Do you think it would be better to just add that instead, and should I try to restrict it further?

Yes - This abstraction reflects a different use-case and therefore
isn't really applicable
I think removing abstractions/libvirt-qemu and instead adding what you
really need is better.

[Christian Ehrhardt  (paelzer)]

> > # Site-specific additions and overrides. See local/README for details.
> > #include <local/usr.sbin.libvirtd>
...
> I can confirm that the local usr.bin.swtpm gets created automatically but is a blank file by default. I could modify the install such that it gets created with helpful info though

The empty template is just fine IMHO - that is the default for all
files and the apparmor man pages explain all that is needed.
But adding that include I mentioned would be needed.
Also that comment above the include points to the README which also
explains how these local overrides are meant to work.

[Lena Voytek (lvoytek)]

Tested the local include and dac_override and both are working properly for the above testcases

[Serge Hallyn (serge-hallyn)]

It's not a secret thing at all, we just haven't yet open sourced
what we're doing only bc there are many moving parts. The
relevent part here is a bit like

    https://github.com/puzzleos/uefi-dev
    https://github.com/puzzleos/uefi-dev/blob/main/tools/run-sw-tpm

except written in golang (and more purpose driven).

So I think you're right about the local override being the
way. Giving swtpm full reign over $HOME is probably too much.
We should probably either

1. drop the ${HOME}/** rwk,

or pick a subdir like

2. ${HOME}/.cache/swtpm/** rwk

But whichever way you go, +1 from me, thank you.

[Lena Voytek (lvoytek)]

> It's not a secret thing at all, we just haven't yet open sourced
> what we're doing only bc there are many moving parts. The
> relevent part here is a bit like
>
> https://github.com/puzzleos/uefi-dev
> https://github.com/puzzleos/uefi-dev/blob/main/tools/run-sw-tpm
>
> except written in golang (and more purpose driven).
>
> So I think you're right about the local override being the
> way. Giving swtpm full reign over $HOME is probably too much.
> We should probably either
>
> 1. drop the ${HOME}/** rwk,
>
> or pick a subdir like
>
> 2. ${HOME}/.cache/swtpm/** rwk
>
> But whichever way you go, +1 from me, thank you.

The subdir addition seems reasonable to me because its specific to swtpm so I added that in. Thanks for the feedback!

[Serge Hallyn (serge-hallyn)]

Hi,

you say:

> The subdir addition seems reasonable to me because its specific to swtpm so I added that in.

but I'm not seeing it in the patch below. Can you double-check that the right thing got pushed?

thanks :)

[Lena Voytek (lvoytek)]

> Hi,
>
> you say:
>
> > The subdir addition seems reasonable to me because its specific to swtpm so
> I added that in.
>
> but I'm not seeing it in the patch below. Can you double-check that the right
> thing got pushed?
>
> thanks :)

Hi,

I just talked with Simon about it, and I think it may be best to go with option 1 and just reference the home directory via the local profile instead.

Thanks!

[Serge Hallyn (serge-hallyn)]

Thanks.

review: Approve

[Lena Voytek (lvoytek)]

Added in network streams and /dev/vtpmx, autopkgtests now passing successfully

[Christian Ehrhardt  (paelzer)]

It seems we do not enable the "cuse" option yet, so their need for /dev/* isn't relevant yet.
You have covered the common use cases AFAICS and IMHO it is better to start slightly too secure and amend the profile if valid use cases are reported instead of starting way too open.

+1 to this

What we need from now is (both can be started and worked on independently):
1. Get FFE Ack by the release team and this uploaded to Jammy
2. Send this to swtpm upstream

If out of #2 there are modifications to the profile we can then later SRU them.

review: Approve

[Christian Ehrhardt  (paelzer)]

FFE approved and sponsored - thanks!

diff --git a/debian/changelog b/debian/changelog
index 8f38c02..bafaa79 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+swtpm (0.6.1-0ubuntu6) jammy; urgency=medium
+
+  * Add apparmor profile to swtpm (LP: #1950631)
+    - d/usr.bin.swtpm: Create new apparmor profile
+    - d/swtpm.install: Copy apparmor profile to /etc/apparmor.d/
+    - d/rules: Deploy the swtpm apparmor profile
+    - d/control: Add dh-apparmor as a dependency
+
+ -- Lena Voytek <[e-mail]>  Fri, 18 Feb 2022 14:24:14 -0700
+
 swtpm (0.6.1-0ubuntu5) jammy; urgency=medium
 
   * debian/patches/openssl-not-certtool.patch: Use traditional format
diff --git a/debian/control b/debian/control
index 08aa42f..a62d3d8 100644
--- a/debian/control
+++ b/debian/control
@@ -5,6 +5,7 @@ Priority: optional
 Standards-Version: 4.5.1
 Build-Depends: libtool,
 	       debhelper-compat (= 10),
+	       dh-apparmor,
 	       libtpms-dev,
 	       libfuse-dev,
 	       libglib2.0-dev,
diff --git a/debian/rules b/debian/rules
index 626d191..030599d 100755
--- a/debian/rules
+++ b/debian/rules
@@ -7,6 +7,11 @@ override_dh_auto_configure:
 	dh_auto_configure -- --with-openssl --with-gnutls --without-cuse \
 		--with-tss-user=swtpm --with-tss-group=swtpm
 
+override_dh_install:
+	dh_install
+	# deploy swtpm's apparmor profile
+	dh_apparmor -pswtpm --profile-name=usr.bin.swtpm
+
 override_dh_auto_test:
 ifeq (,$(filter nocheck,$(DEB_BUILD_OPTIONS)))
 	SWTPM_TEST_SECCOMP_OPT="--seccomp action=none" make -j4 check VERBOSE=1
diff --git a/debian/swtpm.install b/debian/swtpm.install
index a948be9..aa9012a 100644
--- a/debian/swtpm.install
+++ b/debian/swtpm.install
@@ -1,3 +1,4 @@
 /usr/bin/swtpm
 /usr/share/man/man8/swtpm.8*
 /usr/lib/*/swtpm/*.so.*
+debian/usr.bin.swtpm etc/apparmor.d/
diff --git a/debian/usr.bin.swtpm b/debian/usr.bin.swtpm
new file mode 100644
index 0000000..9223918
--- /dev/null
+++ b/debian/usr.bin.swtpm
@@ -0,0 +1,24 @@
+# vim:syntax=apparmor
+# AppArmor policy for swtpm
+# Author: Lena Voytek <[e-mail]>
+# Last Modified: Fri Feb 18 10:23:53 2022
+
+#include <tunables/global>
+
+profile swtpm /usr/bin/swtpm {
+  #include <abstractions/base>
+  #include <abstractions/openssl>
+
+  # Site-specific additions and overrides. See local/README for details.
+  #include <local/usr.bin.swtpm>
+
+  capability dac_override,
+
+  network inet stream,
+  network inet6 stream,
+
+  owner /tmp/** rwk,
+  owner /usr/bin/swtpm r,
+  owner /var/lib/libvirt/swtpm/** rwk,
+  owner /dev/vtpmx rw,
+}