Merge ~sergiodj/ubuntu/+source/apparmor:bug1872564-boot_id-rule-missing into ubuntu/+source/apparmor:ubuntu/groovy-devel

[Sergio Durigan Junior]

apparmor shipped on groovy lacks a rule to allow accessing the file '/proc/sys/kernel/random/boot_id', which systemd needs to be able to read in order to start some types of services. Specifically, if a service performs a nsswitch query in order to, e.g., drop privileges, or if a service's Type is "notify", then systemd will attempt to read the value of the 'boot_id' file and apparmor will not allow it.

The fix, which is cherry-picked from upstream, is to extend the current 'abstractions/nameservice' profile and add the necessary entry to allow read access to the 'boot_id' file. While at it, and for completeness sake, I kept the full commit content intact, which means that two other systemd-related rules will be added (needed to access files under '/run/systemd').

Another simple upstream commit had to be cherry-picked in order to import the definition of the variable '@{run}'.

The upstream commits are:

https://gitlab.com/apparmor/apparmor/-/commit/454fca7
https://gitlab.com/apparmor/apparmor/-/commit/1f319c3

There's a PPA with the changes here:

https://launchpad.net/~sergiodj/+archive/ubuntu/apparmor-bug1872564

autopkgtest is still happy:

autopkgtest [20:43:50]: test command1: /bin/true
autopkgtest [20:43:50]: test command1: [-----------------------
autopkgtest [20:43:50]: test command1: -----------------------]
autopkgtest [20:43:51]: test command1: - - - - - - - - - - results - - - - - - - - - -
command1 PASS (superficial)
autopkgtest [20:43:51]: @@@@@@@@@@@@@@@@@@@@ summary
compile-policy PASS
test-installed PASS (superficial)
command1 PASS (superficial)

[Christian Ehrhardt  (paelzer)]

Generally LGTM, I also added a focal task to the bug to later SRU it once groovy is fixed.

Changelog, patch headers and such look good.

The only thing that I found is that there was a follow on fix which IMHO should be included as well:
=> https://gitlab.com/apparmor/apparmor/-/commit/ef591a67cedc1da0676b26448ea96fa8c073c253
Due to that the state is "needs fixing", but other than this is already seems to be good.

Finally I'll add a review-slot for Ubuntu-security. That isn't strictly needed, but usually a good practice on security related changes.

review: Needs Fixing

[Sergio Durigan Junior (sergiodj)]

On Monday, May 11 2020, Christian Ehrhardt  wrote:

> Generally LGTM, I also added a focal task to the bug to later SRU it once groovy is fixed.

Thanks, Christian. I was going to ask if we should mark the bug as
"found in focal" as well.

> The only thing that I found is that there was a follow on fix which IMHO should be included as well:
> => https://gitlab.com/apparmor/apparmor/-/commit/ef591a67cedc1da0676b26448ea96fa8c073c253
> Due to that the state is "needs fixing", but other than this is already seems to be good.

Ops, good catch. Thanks, I'll make sure to cherry-pick this patch as well.

> Finally I'll add a review-slot for Ubuntu-security. That isn't strictly needed, but usually a good practice on security related changes.

This was something I was also planning to ask. Andreas mentioned that
it'd be good to add a slot for ubuntu-security, but I was sure if it was
for this upload specifically.

Thanks, I'll force-push an update soon.

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Sergio Durigan Junior (sergiodj)]

On Monday, May 11 2020, Christian Ehrhardt  wrote:

> The only thing that I found is that there was a follow on fix which IMHO should be included as well:
> => https://gitlab.com/apparmor/apparmor/-/commit/ef591a67cedc1da0676b26448ea96fa8c073c253
> Due to that the state is "needs fixing", but other than this is already seems to be good.

I was wondering here how I could have missed the commit above... And
now I know! This commit was only applied to master, but I was only
considering the apparmor-2.13 branch when cherry-picking patches.

So yeah, there you go...

Thanks

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Steve Beattie (sbeattie)]

This all looks good to me, on behalf of the Ubuntu Security team.

(I'm not sure why the second fix, to add a trailing slash to the @run definitions didn't get pulled back upstream; I'll submit a merge request for that. Thanks for pointing it out!)

review: Approve

[Christian Ehrhardt  (paelzer)]

Patches are complete now and security review is in as well (Thanks Steve).

+1 Sergio

Do you have any last minute tests to complete on this or is this ready for sponsoring?

review: Approve

[Sergio Durigan Junior (sergiodj)]

On Tuesday, May 12 2020, Christian Ehrhardt  wrote:

> Patches are complete now and security review is in as well (Thanks Steve).
>
> +1 Sergio

Thanks, Christian!

> Do you have any last minute tests to complete on this or is this ready for sponsoring?

Nope, it's all done. Feel free to upload it when you can :-).

Thanks again!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Christian Ehrhardt  (paelzer)]

To ssh://git.launchpad.net/ubuntu/+source/apparmor
 * [new tag] upload/2.13.3-7ubuntu6 -> upload/2.13.3-7ubuntu6

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading apparmor_2.13.3-7ubuntu6.dsc: done.
  Uploading apparmor_2.13.3-7ubuntu6.debian.tar.xz: done.
  Uploading apparmor_2.13.3-7ubuntu6_source.buildinfo: done.
  Uploading apparmor_2.13.3-7ubuntu6_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/apparmor.install b/debian/apparmor.install
index 208b96a..46a3f63 100644
--- a/debian/apparmor.install
+++ b/debian/apparmor.install
@@ -16,6 +16,7 @@ etc/apparmor.d/tunables/kernelvars
 etc/apparmor.d/tunables/multiarch
 etc/apparmor.d/tunables/multiarch.d
 etc/apparmor.d/tunables/proc
+etc/apparmor.d/tunables/run
 etc/apparmor.d/tunables/securityfs
 etc/apparmor.d/tunables/share
 etc/apparmor.d/tunables/sys
diff --git a/debian/changelog b/debian/changelog
index dd1a5b2..eb99e70 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,17 @@
+apparmor (2.13.3-7ubuntu6) groovy; urgency=medium
+
+  * Add missing "boot_id" rule to abstractions/nameservice. (LP: #1872564)
+    - d/p/upstream-commit-454fca7-Add-run-variable.patch: Add the
+      definition for the "@{run}" variable.
+    - d/p/upstream-commit-ef591a67-Add-trailing-slash-to-the-run-variable-definition.patch:
+      Add trailing slash to the "@{run}" variable.
+    - d/p/upstream-commit-1f319c3870-abstractions-nameservice-allow-accessing-run-systemd-user.patch:
+      Add a missing rule to allow systemd to access
+      @{PROC}/sys/kernel/random/boot_id and @{run}/systemd/userdb.
+    - d/apparmor.install: Install new file 'tunables/run' under '/etc/apparmor.d'.
+
+ -- Sergio Durigan Junior <[e-mail]>  Mon, 11 May 2020 09:55:16 -0400
+
 apparmor (2.13.3-7ubuntu5) focal; urgency=medium
 
   * snapd 2.44.3+20.04 introduced an apparmor unit of its own to load snap
diff --git a/debian/patches/series b/debian/patches/series
index 04a6ed1..4910b2b 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -36,3 +36,6 @@ upstream-mr-442-gnome-user-themes.patch
 upstream-mr-443-ecryptfs-dirs.patch
 upstream-mr-445-uuidd-request.patch
 upstream-mr-464-Mesa_i915_perf_interface.patch
+upstream-commit-454fca7-Add-run-variable.patch
+upstream-commit-ef591a67-Add-trailing-slash-to-the-run-variable-definition.patch
+upstream-commit-1f319c3870-abstractions-nameservice-allow-accessing-run-systemd-user.patch
diff --git a/debian/patches/upstream-commit-1f319c3870-abstractions-nameservice-allow-accessing-run-systemd-user.patch b/debian/patches/upstream-commit-1f319c3870-abstractions-nameservice-allow-accessing-run-systemd-user.patch
new file mode 100644
index 0000000..215ac0c
--- /dev/null
+++ b/debian/patches/upstream-commit-1f319c3870-abstractions-nameservice-allow-accessing-run-systemd-user.patch
@@ -0,0 +1,37 @@
+From: Sergio Durigan Junior <[e-mail]>
+Date: Fri, 8 May 2020 10:13:24 -0400
+Subject: abstractions/nameservice: allow accessing /run/systemd/userdb/
+
+On systems with systemd 245, nss-systemd additionally queries NSS records from systemd-userdbd.service. See https://systemd.io/USER_GROUP_API/ .
+
+(cherry picked from commit 16f9f6885aff84123c0b52197f435e40d656c0e4)
+Fixes: https://gitlab.com/apparmor/apparmor/-/issues/82
+Signed-off-by: nl6720 <[e-mail]>
+Signed-off-by: John Johansen <[e-mail]>
+
+Author: nl6720 <[e-mail]>
+Origin: upstream, https://gitlab.com/apparmor/apparmor/-/commit/1f319c3870287b9a2cfa39e92344c9d35875b811
+Bug: https://gitlab.com/apparmor/apparmor/-/issues/82
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1872564
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2020-05-08
+---
+ profiles/apparmor.d/abstractions/nameservice | 5 +++++
+ 1 file changed, 5 insertions(+)
+
+diff --git a/profiles/apparmor.d/abstractions/nameservice b/profiles/apparmor.d/abstractions/nameservice
+index 4ebecfd..a04a30a 100644
+--- a/profiles/apparmor.d/abstractions/nameservice
++++ b/profiles/apparmor.d/abstractions/nameservice
+@@ -29,6 +29,11 @@
+   /var/lib/extrausers/group  r,
+   /var/lib/extrausers/passwd r,
+ 
++  # NSS records from systemd-userdbd.service
++  @{run}/systemd/userdb/ r,
++  @{run}/systemd/userdb/io.systemd.{NameServiceSwitch,Multiplexer,DynamicUser,Home} r,
++  @{PROC}/sys/kernel/random/boot_id r,
++
+   # When using sssd, the passwd and group files are stored in an alternate path
+   # and the nss plugin also needs to talk to a pipe
+   /var/lib/sss/mc/group   r,
diff --git a/debian/patches/upstream-commit-454fca7-Add-run-variable.patch b/debian/patches/upstream-commit-454fca7-Add-run-variable.patch
new file mode 100644
index 0000000..9e14672
--- /dev/null
+++ b/debian/patches/upstream-commit-454fca7-Add-run-variable.patch
@@ -0,0 +1,47 @@
+From: nl6720 <[e-mail]>
+Date: Thu, 13 Feb 2020 09:58:33 +0200
+Subject: Add "run" variable
+
+Signed-off-by: nl6720 <[e-mail]>
+(cherry picked from commit 452b5b8735e449cba29a1fb25c9bff38ba8763ec)
+
+Author: nl6720 <[e-mail]>
+Origin: upstream, https://gitlab.com/apparmor/apparmor/-/commit/454fca7483eae7b7ee613343c2c02abaa20e37e3
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1872564
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2020-05-08
+---
+ parser/apparmor.d.pod               | 1 +
+ profiles/apparmor.d/tunables/global | 1 +
+ profiles/apparmor.d/tunables/run    | 1 +
+ 3 files changed, 3 insertions(+)
+ create mode 100644 profiles/apparmor.d/tunables/run
+
+diff --git a/parser/apparmor.d.pod b/parser/apparmor.d.pod
+index db904ed..d9dff30 100644
+--- a/parser/apparmor.d.pod
++++ b/parser/apparmor.d.pod
+@@ -1279,6 +1279,7 @@ provided AppArmor policy:
+   @{apparmorfs}
+   @{sys}
+   @{tid}
++  @{run}
+   @{XDG_DESKTOP_DIR}
+   @{XDG_DOWNLOAD_DIR}
+   @{XDG_TEMPLATES_DIR}
+diff --git a/profiles/apparmor.d/tunables/global b/profiles/apparmor.d/tunables/global
+index 28d6fc6..3b6f99c 100644
+--- a/profiles/apparmor.d/tunables/global
++++ b/profiles/apparmor.d/tunables/global
+@@ -19,3 +19,4 @@
+ #include <tunables/kernelvars>
+ #include <tunables/xdg-user-dirs>
+ #include <tunables/share>
++#include <tunables/run>
+diff --git a/profiles/apparmor.d/tunables/run b/profiles/apparmor.d/tunables/run
+new file mode 100644
+index 0000000..e535d2f
+--- /dev/null
++++ b/profiles/apparmor.d/tunables/run
+@@ -0,0 +1 @@
++@{run}=/run /var/run
diff --git a/debian/patches/upstream-commit-ef591a67-Add-trailing-slash-to-the-run-variable-definition.patch b/debian/patches/upstream-commit-ef591a67-Add-trailing-slash-to-the-run-variable-definition.patch
new file mode 100644
index 0000000..97fede3
--- /dev/null
+++ b/debian/patches/upstream-commit-ef591a67-Add-trailing-slash-to-the-run-variable-definition.patch
@@ -0,0 +1,22 @@
+From: nl6720 <[e-mail]>
+Date: Thu, 20 Feb 2020 10:40:22 +0200
+Subject: Add trailing slash to the run variable definition
+
+Signed-off-by: nl6720 <[e-mail]>
+
+Author: nl6720 <[e-mail]>
+Origin: upstream, https://gitlab.com/apparmor/apparmor/-/commit/ef591a67cedc1da0676b26448ea96fa8c073c253
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1872564
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2020-05-08
+---
+ profiles/apparmor.d/tunables/run | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/profiles/apparmor.d/tunables/run b/profiles/apparmor.d/tunables/run
+index e535d2f..5b81925 100644
+--- a/profiles/apparmor.d/tunables/run
++++ b/profiles/apparmor.d/tunables/run
+@@ -1 +1 @@
+-@{run}=/run /var/run
++@{run}=/run/ /var/run/