Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Stefan Bader]

On 19.05.2017 13:13, Guido Günther wrote:
> On Fri, May 19, 2017 at 11:18:18AM +0200, Christian Ehrhardt wrote:
>> On Fri, May 19, 2017 at 10:03 AM, Guido Günther  wrote:
>>
>>> But if we aim for a profile replace on blockcommit [1] the would't matter
>>> since the whole profile would get replaced, wouldn't it?
>>>
>>
>> Since this is based on [1][2] looping in Cédric here to share some old
>> explaiantions.
>> See especially [1] for some reasoning for 'R' in general.
>>
>> [1]:
>> http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2fc5231fbd4c20f
>> [2]:
>> http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2a0f8d9f88ca9e
> 
> Let me try to explain why I don't consider this to be a proper fix:
> 
> virsh blockcommit is invoked this leads to:
> 
> 1.) qemuDomainBlockCommit ->
> 2.) qemuDomainDiskChainElementPrepare ->
> 3.) qemuSecuritySetImageLabel ->
> 4.) AppArmorSetSecurityImageLabel (triggers profile reload only) ->
> 5.) virt-aa-helper does the profile reload ->
> 6.) failure since the image has an explicit deny rule
> 
> The path in question tries to fix this at 5.) by not adding a deny write
> rule at all but the place to fix this is 4.) since
> AppArmorSetSecurityImageLabel does not take the virStorageSourcePtr src
> element into account to create a virDomainDefPtr based on def that marks
> the image in question as 'rw' but "only" reloads the profile.


Ok, I think we got the drift but need more time to ponder about that. Christian
created a tracking bug for us and I move it to the needs-more-thinking section.

-Stefan


> 
> Cheers,
>  -- Guido
> 




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Guido Günther]

On Fri, May 19, 2017 at 11:18:18AM +0200, Christian Ehrhardt wrote:
> On Fri, May 19, 2017 at 10:03 AM, Guido Günther  wrote:
> 
> > But if we aim for a profile replace on blockcommit [1] the would't matter
> > since the whole profile would get replaced, wouldn't it?
> >
> 
> Since this is based on [1][2] looping in Cédric here to share some old
> explaiantions.
> See especially [1] for some reasoning for 'R' in general.
> 
> [1]:
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2fc5231fbd4c20f
> [2]:
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2a0f8d9f88ca9e

Let me try to explain why I don't consider this to be a proper fix:

virsh blockcommit is invoked this leads to:

1.) qemuDomainBlockCommit ->
2.) qemuDomainDiskChainElementPrepare ->
3.) qemuSecuritySetImageLabel ->
4.) AppArmorSetSecurityImageLabel (triggers profile reload only) ->
5.) virt-aa-helper does the profile reload ->
6.) failure since the image has an explicit deny rule

The path in question tries to fix this at 5.) by not adding a deny write
rule at all but the place to fix this is 4.) since
AppArmorSetSecurityImageLabel does not take the virStorageSourcePtr src
element into account to create a virDomainDefPtr based on def that marks
the image in question as 'rw' but "only" reloads the profile.

Cheers,
 -- Guido

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Cedric Bosdonnat]

Hi Christian,

On Fri, 2017-05-19 at 11:18 +0200, Christian Ehrhardt wrote:
> 
> On Fri, May 19, 2017 at 10:03 AM, Guido Günther  wrote:
> > But if we aim for a profile replace on blockcommit [1] the would't matter
> > since the whole profile would get replaced, wouldn't it?
> > 
> 
> Since this is based on [1][2] looping in Cédric here to share some old 
> explaiantions.
> See especially [1] for some reasoning for 'R' in general.
> 
> [1]: 
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2fc5231fbd4c20f
> [2]: 
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2a0f8d9f88ca9e

Sadly the bug report isn't public since it has been reported again SLES. But 
here is the
description of the bug that motivated that fix:


-- %< --
Steps to reproduce:
  * run virt-sandbox /bin/sh as root

Expected result: Run a shell in a qemu domain, apparmor enforced
Actual result: Domain fails to start

After some more debugging it happens that the problem is caused by 


  
  
  


Since commit 
http://libvirt.org/git/?p=libvirt.git;a=commit;h=d0d4b8ad76d3e8a859ee90701a21a3f003a22c1f,
 virt-aa-helper
generates a "deny /** w" rule in such cases that takes precedence over the 
allow rules.

This has several effects:
  * It hides the DENIED/ALLOWED apparmor log entries
  * It prevents qemu to write to the log file, /dev/ptmx and other important 
files to run the domain.

To see the rules, add the audit flag to /etc/apparmor.d/libvirt/TEMPLATE.qemu 
file and rerun virt-sandbox.
-- %< --

Hi hope this will answer your questions

--
Cedric

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Guido Günther]

Hi Christian,
On Fri, May 19, 2017 at 11:18:18AM +0200, Christian Ehrhardt wrote:
> On Fri, May 19, 2017 at 10:03 AM, Guido Günther  wrote:
> 
> > But if we aim for a profile replace on blockcommit [1] the would't matter
> > since the whole profile would get replaced, wouldn't it?
> >
> 
> Since this is based on [1][2] looping in Cédric here to share some old
> explaiantions.
> See especially [1] for some reasoning for 'R' in general.
> 
> [1]:
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2fc5231fbd4c20f
> [2]:
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2a0f8d9f88ca9e

Thanks for digging these out again. I know the reason for 'R' but it
seems for block commit we want s.th. different namely replacing the
profile with a 'w' when replacing the profile.

Cheers,
 -- Guido

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Christian Ehrhardt]

On Fri, May 19, 2017 at 10:03 AM, Guido Günther  wrote:

> But if we aim for a profile replace on blockcommit [1] the would't matter
> since the whole profile would get replaced, wouldn't it?
>

Since this is based on [1][2] looping in Cédric here to share some old
explaiantions.
See especially [1] for some reasoning for 'R' in general.

[1]:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2fc5231fbd4c20f
[2]:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2a0f8d9f88ca9e


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Guido Günther]

On Mon, May 15, 2017 at 06:07:12PM +0200, Stefan Bader wrote:
> On 15.05.2017 17:48, Guido Günther wrote:
> > On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote:
> >> From: Serge Hallyn 
> >>
> >> Just because a disk element only requests read access doesn't mean
> >> there may not be another readwrite request.
> >>
> >> Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031
> > 
> > The URL is wrong (drop the "ubuntu" part. From the bug report this looks
> > like a workaround:
> 
> Darn, thanks for catching this.
> 
> > 
> >  
> > https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8
> > 
> > or am I misreading this? Shouldn't we only change to 'w' on blockcommit?
> 
> As I understand it, the 'r' would implicitly add a write deny rule, so it is 
> not
> possible to override that. With the 'R' notation only a rule allowing read is
> added. Which allows to change to change to 'w' on blockcommit.

But if we aim for a profile replace on blockcommit [1] the would't matter
since the whole profile would get replaced, wouldn't it?

Cheers,
 -- Guido

[1] I have not checked that that profile replace currenlty happens with
libvirt I'm trying to figure out how we want it to work and then fix the
right place.

> 
> -Stefan
> 
> > Cheers
> >  -- Guido
> > 
> >>
> >> Signed-off-by: Christian Ehrhardt 
> >> Signed-off-by: Stefan Bader 
> >> ---
> >>  src/security/virt-aa-helper.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> >> index 5f5d1cd..d976a00 100644
> >> --- a/src/security/virt-aa-helper.c
> >> +++ b/src/security/virt-aa-helper.c
> >> @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
> >>  
> >>  if (depth == 0) {
> >>  if (disk->src->readonly)
> >> -ret = vah_add_file(buf, path, "r");
> >> +ret = vah_add_file(buf, path, "R");
> >>  else
> >>  ret = vah_add_file(buf, path, "rw");
> >>  } else {
> >> -ret = vah_add_file(buf, path, "r");
> >> +ret = vah_add_file(buf, path, "R");
> >>  }
> >>  
> >>  if (ret != 0)
> >> -- 
> >> 2.7.4
> >>
> > 
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> > 
> 
> 



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Stefan Bader]

On 15.05.2017 17:48, Guido Günther wrote:
> On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote:
>> From: Serge Hallyn 
>>
>> Just because a disk element only requests read access doesn't mean
>> there may not be another readwrite request.
>>
>> Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031
> 
> The URL is wrong (drop the "ubuntu" part. From the bug report this looks
> like a workaround:

Darn, thanks for catching this.

> 
>  https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8
> 
> or am I misreading this? Shouldn't we only change to 'w' on blockcommit?

As I understand it, the 'r' would implicitly add a write deny rule, so it is not
possible to override that. With the 'R' notation only a rule allowing read is
added. Which allows to change to change to 'w' on blockcommit.

-Stefan

> Cheers
>  -- Guido
> 
>>
>> Signed-off-by: Christian Ehrhardt 
>> Signed-off-by: Stefan Bader 
>> ---
>>  src/security/virt-aa-helper.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index 5f5d1cd..d976a00 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
>>  
>>  if (depth == 0) {
>>  if (disk->src->readonly)
>> -ret = vah_add_file(buf, path, "r");
>> +ret = vah_add_file(buf, path, "R");
>>  else
>>  ret = vah_add_file(buf, path, "rw");
>>  } else {
>> -ret = vah_add_file(buf, path, "r");
>> +ret = vah_add_file(buf, path, "R");
>>  }
>>  
>>  if (ret != 0)
>> -- 
>> 2.7.4
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Guido Günther]

On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote:
> From: Serge Hallyn 
> 
> Just because a disk element only requests read access doesn't mean
> there may not be another readwrite request.
> 
> Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031

The URL is wrong (drop the "ubuntu" part. From the bug report this looks
like a workaround:

 https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8

or am I misreading this? Shouldn't we only change to 'w' on blockcommit?
Cheers
 -- Guido

> 
> Signed-off-by: Christian Ehrhardt 
> Signed-off-by: Stefan Bader 
> ---
>  src/security/virt-aa-helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 5f5d1cd..d976a00 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
>  
>  if (depth == 0) {
>  if (disk->src->readonly)
> -ret = vah_add_file(buf, path, "r");
> +ret = vah_add_file(buf, path, "R");
>  else
>  ret = vah_add_file(buf, path, "rw");
>  } else {
> -ret = vah_add_file(buf, path, "r");
> +ret = vah_add_file(buf, path, "R");
>  }
>  
>  if (ret != 0)
> -- 
> 2.7.4
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Stefan Bader]

From: Serge Hallyn 

Just because a disk element only requests read access doesn't mean
there may not be another readwrite request.

Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031

Signed-off-by: Christian Ehrhardt 
Signed-off-by: Stefan Bader 
---
 src/security/virt-aa-helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5f5d1cd..d976a00 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
 
 if (depth == 0) {
 if (disk->src->readonly)
-ret = vah_add_file(buf, path, "r");
+ret = vah_add_file(buf, path, "R");
 else
 ret = vah_add_file(buf, path, "rw");
 } else {
-ret = vah_add_file(buf, path, "r");
+ret = vah_add_file(buf, path, "R");
 }
 
 if (ret != 0)
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list