[PATCH v3 3/4] apparmor: fix UUID specification

Georgia Garcia posted 4 patches 1 week, 6 days ago
[PATCH v3 3/4] apparmor: fix UUID specification
Posted by Georgia Garcia 1 week, 6 days ago
There is a common misconception when writing AppArmor policy that
[0-9]* applies * to the [0-9] class, but that's not the case. For this
example, [0-9]* matches a single digit followed by any number of
characters except for /

Create a UUID variable that uses the following format 8-4-4-4-12.

Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
---
 src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 ++++-
 src/security/apparmor/usr.sbin.libvirtd.in              | 7 +++++--
 src/security/apparmor/usr.sbin.virtqemud.in             | 6 ++++--
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
index 44645c6989..90a8b7072c 100644
--- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
+++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
@@ -1,5 +1,8 @@
 #include <tunables/global>
 
+@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f]
+@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet}
+
 profile virt-aa-helper @libexecdir@/virt-aa-helper {
   #include <abstractions/base>
   #include <abstractions/openssl>
@@ -44,7 +47,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper {
   /{usr/,}{s,}bin/apparmor_parser Ux,
 
   @sysconfdir@/apparmor.d/libvirt/* r,
-  @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw,
+  @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw,
 
   # for backingstore -- allow access to non-hidden files in @{HOME} as well
   # as storage pools
diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in
index 70e586895f..3659ddc219 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -1,4 +1,7 @@
 #include <tunables/global>
+
+@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f]
+@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet}
 @{LIBVIRT}="libvirt"
 
 profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
@@ -72,7 +75,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
   signal (send) set=("term") peer=libvirtd//qemu_bridge_helper,
 
   # allow connect with openGraphicsFD, direction reversed in newer versions
-  unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*),
+  unix (send, receive) type=stream addr=none peer=(label=libvirt-@{UUID}),
   # unconfined also required if guests run without security module
   unix (send, receive) type=stream addr=none peer=(label=unconfined),
 
@@ -115,7 +118,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
   /etc/xen/scripts/** rmix,
 
   # allow changing to our UUID-based named profiles
-  change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
+  change_profile -> @{LIBVIRT}-@{UUID},
 
   /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx -> qemu_bridge_helper,
   # child profile for bridge helper process
diff --git a/src/security/apparmor/usr.sbin.virtqemud.in b/src/security/apparmor/usr.sbin.virtqemud.in
index 42fa4813da..86b23465b6 100644
--- a/src/security/apparmor/usr.sbin.virtqemud.in
+++ b/src/security/apparmor/usr.sbin.virtqemud.in
@@ -1,5 +1,7 @@
 #include <tunables/global>
 @{LIBVIRT}="libvirt"
+@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f]
+@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet}
 
 profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
   #include <abstractions/base>
@@ -71,7 +73,7 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
   signal (send) set=(term) peer=libvirtd//qemu_bridge_helper,
 
   # allow connect with openGraphicsFD, direction reversed in newer versions
-  unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*),
+  unix (send, receive) type=stream addr=none peer=(label=libvirt-@{UUID}),
   # unconfined also required if guests run without security module
   unix (send, receive) type=stream addr=none peer=(label=unconfined),
 
@@ -109,7 +111,7 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
   /etc/libvirt/hooks/** rmix,
 
   # allow changing to our UUID-based named profiles
-  change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
+  change_profile -> @{LIBVIRT}-@{UUID},
 
   /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx -> qemu_bridge_helper,
   # child profile for bridge helper process
-- 
2.43.0
Re: [PATCH v3 3/4] apparmor: fix UUID specification
Posted by Jim Fehlig via Devel 1 week, 6 days ago
On 1/7/25 08:23, Georgia Garcia wrote:
> There is a common misconception when writing AppArmor policy that
> [0-9]* applies * to the [0-9] class, but that's not the case. For this
> example, [0-9]* matches a single digit followed by any number of
> characters except for /
> 
> Create a UUID variable that uses the following format 8-4-4-4-12.
> 
> Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
> ---
>   src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 ++++-
>   src/security/apparmor/usr.sbin.libvirtd.in              | 7 +++++--
>   src/security/apparmor/usr.sbin.virtqemud.in             | 6 ++++--
>   3 files changed, 13 insertions(+), 5 deletions(-)

This patch seems fine to me. Did you notice the issue by code inspection, or 
does it fix an observed error? If the latter, we should mention it in the commit 
message.

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

> 
> diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> index 44645c6989..90a8b7072c 100644
> --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> @@ -1,5 +1,8 @@
>   #include <tunables/global>
>   
> +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f]
> +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet}
> +
>   profile virt-aa-helper @libexecdir@/virt-aa-helper {
>     #include <abstractions/base>
>     #include <abstractions/openssl>
> @@ -44,7 +47,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper {
>     /{usr/,}{s,}bin/apparmor_parser Ux,
>   
>     @sysconfdir@/apparmor.d/libvirt/* r,
> -  @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw,
> +  @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw,
>   
>     # for backingstore -- allow access to non-hidden files in @{HOME} as well
>     # as storage pools
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in
> index 70e586895f..3659ddc219 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -1,4 +1,7 @@
>   #include <tunables/global>
> +
> +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f]
> +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet}
>   @{LIBVIRT}="libvirt"
>   
>   profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
> @@ -72,7 +75,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
>     signal (send) set=("term") peer=libvirtd//qemu_bridge_helper,
>   
>     # allow connect with openGraphicsFD, direction reversed in newer versions
> -  unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*),
> +  unix (send, receive) type=stream addr=none peer=(label=libvirt-@{UUID}),
>     # unconfined also required if guests run without security module
>     unix (send, receive) type=stream addr=none peer=(label=unconfined),
>   
> @@ -115,7 +118,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
>     /etc/xen/scripts/** rmix,
>   
>     # allow changing to our UUID-based named profiles
> -  change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
> +  change_profile -> @{LIBVIRT}-@{UUID},
>   
>     /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx -> qemu_bridge_helper,
>     # child profile for bridge helper process
> diff --git a/src/security/apparmor/usr.sbin.virtqemud.in b/src/security/apparmor/usr.sbin.virtqemud.in
> index 42fa4813da..86b23465b6 100644
> --- a/src/security/apparmor/usr.sbin.virtqemud.in
> +++ b/src/security/apparmor/usr.sbin.virtqemud.in
> @@ -1,5 +1,7 @@
>   #include <tunables/global>
>   @{LIBVIRT}="libvirt"
> +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f]
> +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet}
>   
>   profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
>     #include <abstractions/base>
> @@ -71,7 +73,7 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
>     signal (send) set=(term) peer=libvirtd//qemu_bridge_helper,
>   
>     # allow connect with openGraphicsFD, direction reversed in newer versions
> -  unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*),
> +  unix (send, receive) type=stream addr=none peer=(label=libvirt-@{UUID}),
>     # unconfined also required if guests run without security module
>     unix (send, receive) type=stream addr=none peer=(label=unconfined),
>   
> @@ -109,7 +111,7 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) {
>     /etc/libvirt/hooks/** rmix,
>   
>     # allow changing to our UUID-based named profiles
> -  change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
> +  change_profile -> @{LIBVIRT}-@{UUID},
>   
>     /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx -> qemu_bridge_helper,
>     # child profile for bridge helper process
Re: [PATCH v3 3/4] apparmor: fix UUID specification
Posted by Georgia Garcia 1 week, 5 days ago
On Tue, 2025-01-07 at 17:04 -0700, Jim Fehlig wrote:
> On 1/7/25 08:23, Georgia Garcia wrote:
> > There is a common misconception when writing AppArmor policy that
> > [0-9]* applies * to the [0-9] class, but that's not the case. For this
> > example, [0-9]* matches a single digit followed by any number of
> > characters except for /
> > 
> > Create a UUID variable that uses the following format 8-4-4-4-12.
> > 
> > Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
> > ---
> >    src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 ++++-
> >    src/security/apparmor/usr.sbin.libvirtd.in              | 7 +++++--
> >    src/security/apparmor/usr.sbin.virtqemud.in             | 6 ++++--
> >    3 files changed, 13 insertions(+), 5 deletions(-)
> 
> This patch seems fine to me. Did you notice the issue by code inspection, or 
> does it fix an observed error? If the latter, we should mention it in the commit 
> message.
> 

It was indeed by code inspection. Since the rules were broader than
needed, we wouldn't see errors related to this in normal libvirt use.
I'm just restricting it to what was the intended behavior.

Thank you,
Georgia 

> Reviewed-by: Jim Fehlig <jfehlig@suse.com>
> 
> Regards,
> Jim
Re: [PATCH v3 3/4] apparmor: fix UUID specification
Posted by Jim Fehlig via Devel 1 week, 5 days ago
On 1/8/25 04:45, Georgia Garcia wrote:
> On Tue, 2025-01-07 at 17:04 -0700, Jim Fehlig wrote:
>> On 1/7/25 08:23, Georgia Garcia wrote:
>>> There is a common misconception when writing AppArmor policy that
>>> [0-9]* applies * to the [0-9] class, but that's not the case. For this
>>> example, [0-9]* matches a single digit followed by any number of
>>> characters except for /
>>>
>>> Create a UUID variable that uses the following format 8-4-4-4-12.
>>>
>>> Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
>>> ---
>>>     src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 ++++-
>>>     src/security/apparmor/usr.sbin.libvirtd.in              | 7 +++++--
>>>     src/security/apparmor/usr.sbin.virtqemud.in             | 6 ++++--
>>>     3 files changed, 13 insertions(+), 5 deletions(-)
>>
>> This patch seems fine to me. Did you notice the issue by code inspection, or
>> does it fix an observed error? If the latter, we should mention it in the commit
>> message.
>>
> 
> It was indeed by code inspection. Since the rules were broader than
> needed, we wouldn't see errors related to this in normal libvirt use.
> I'm just restricting it to what was the intended behavior.

Thanks for confirming. I've pushed patches 1-3 since they are independent 
improvements from the issue you're trying to fix in patch 4.

Regards,
Jim