1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline

Jim Fehlig posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20171130174324.3774-1-jfehlig@suse.com
examples/apparmor/libvirt-qemu | 1 +
1 file changed, 1 insertion(+)
[libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by Jim Fehlig 6 years, 4 months ago 
Noticed the following denial in audit.log when shutting down
an apparmor confined domain

type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
operation="open" profile="libvirt-66154842-e926-4f92-92f0-1c1bf61dd1ff"
name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
requested_mask="r" denied_mask="r" fsuid=469 ouid=0

Squelch the denial by allowing read access to /proc/<pid>/cmdline.

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

Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is the
qemu process. I must admit it is not clear to me why
/proc/<libvirtd-pid>/cmdline is read on domain shutdown.

 examples/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 73bdbae87..3d9eed9ec 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -25,6 +25,7 @@
   /dev/ptmx rw,
   /dev/kqemu rw,
   @{PROC}/*/status r,
+  @{PROC}/@{pid}/cmdline r,
   # Per man(5) proc, the kernel enforces that a thread may
   # only modify its comm value or those in its thread group.
   owner @{PROC}/@{pid}/task/@{tid}/comm rw,
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by Michal Privoznik 6 years, 4 months ago 
On 11/30/2017 06:43 PM, Jim Fehlig wrote:
> Noticed the following denial in audit.log when shutting down
> an apparmor confined domain
> 
> type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
> operation="open" profile="libvirt-66154842-e926-4f92-92f0-1c1bf61dd1ff"
> name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
> requested_mask="r" denied_mask="r" fsuid=469 ouid=0
> 
> Squelch the denial by allowing read access to /proc/<pid>/cmdline.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is the
> qemu process. I must admit it is not clear to me why
> /proc/<libvirtd-pid>/cmdline is read on domain shutdown.

It's result of these qemu patches:

fbe7e3327a8cfa1b08664c2cda7a0a341cf0530a
7dc9ae4339faa97e89daadb2e1098147ab4aadc8

Whenever qemu receives a signal it reports PID that sent it. However,
this doesn't help anything really - processes come and go, PIDs change.
I recall debugging some issue where qemu was dying and all I could see
was PID. I was quite certain that the signal was not sent by libvirtd.
The only way to prove it was to have those two patches in so that qemu
can now report process name among with the PID.

> 
>  examples/apparmor/libvirt-qemu | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
> index 73bdbae87..3d9eed9ec 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -25,6 +25,7 @@
>    /dev/ptmx rw,
>    /dev/kqemu rw,
>    @{PROC}/*/status r,
> +  @{PROC}/@{pid}/cmdline r,
>    # Per man(5) proc, the kernel enforces that a thread may
>    # only modify its comm value or those in its thread group.
>    owner @{PROC}/@{pid}/task/@{tid}/comm rw,
> 


ACK and safe for the freeze.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by intrigeri 6 years, 4 months ago 
Hi,

Michal Privoznik:
> On 11/30/2017 06:43 PM, Jim Fehlig wrote:
>> I must admit it is not clear to me why
>> /proc/<libvirtd-pid>/cmdline is read on domain shutdown.

> It's result of these qemu patches:

> fbe7e3327a8cfa1b08664c2cda7a0a341cf0530a
> 7dc9ae4339faa97e89daadb2e1098147ab4aadc8

> Whenever qemu receives a signal it reports PID that sent it.

Thanks a lot for investigating!

>>  examples/apparmor/libvirt-qemu | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
>> index 73bdbae87..3d9eed9ec 100644
>> --- a/examples/apparmor/libvirt-qemu
>> +++ b/examples/apparmor/libvirt-qemu
>> @@ -25,6 +25,7 @@
>>    /dev/ptmx rw,
>>    /dev/kqemu rw,
>>    @{PROC}/*/status r,
>> +  @{PROC}/@{pid}/cmdline r,
>>    # Per man(5) proc, the kernel enforces that a thread may
>>    # only modify its comm value or those in its thread group.
>>    owner @{PROC}/@{pid}/task/@{tid}/comm rw,
>> 

> ACK and safe for the freeze.

+1

(I've seen this denial for a while but did not notice any problem it
would cause, so I was wondering if we should allow this operation or
just ignore the denial & silence the logs. Now that we understand what
it is about, I agree we should allow it. Denying this access would
make it harder to debug issues in the future e.g. if QEMU ever starts
needing it for other, more critical reasons.)

Cheers,
-- 
intrigeri

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by Jamie Strandboge 6 years, 4 months ago 
On Thu, 2017-11-30 at 10:43 -0700, Jim Fehlig wrote:
> Noticed the following denial in audit.log when shutting down
> an apparmor confined domain
> 
> type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
> operation="open" profile="libvirt-66154842-e926-4f92-92f0-
> 1c1bf61dd1ff"
> name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
> requested_mask="r" denied_mask="r" fsuid=469 ouid=0
> 
> Squelch the denial by allowing read access to /proc/<pid>/cmdline.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is the
> qemu process. I must admit it is not clear to me why
> /proc/<libvirtd-pid>/cmdline is read on domain shutdown.
> 
>  examples/apparmor/libvirt-qemu | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 73bdbae87..3d9eed9ec 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -25,6 +25,7 @@
>    /dev/ptmx rw,
>    /dev/kqemu rw,
>    @{PROC}/*/status r,
> +  @{PROC}/@{pid}/cmdline r,

Note this is an information leak and allows reading potentially
sensitive information, such as passwords given on the command line. Eg:

$ cat /proc/13335/cmdline | tr '\0' ' '
sh /tmp/testme --password=sensitive

Would it be possible to use 'owner' match? Eg:

  owner @{PROC}/@{pid}/cmdline r,

Either way, I think a comment is warranted above the rule stating it is
an information leak.

-- 
Jamie Strandboge             | http://www.canonical.com--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by Jim Fehlig 6 years, 4 months ago 
On 12/01/2017 06:26 AM, Jamie Strandboge wrote:
> On Thu, 2017-11-30 at 10:43 -0700, Jim Fehlig wrote:
>> Noticed the following denial in audit.log when shutting down
>> an apparmor confined domain
>>
>> type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
>> operation="open" profile="libvirt-66154842-e926-4f92-92f0-
>> 1c1bf61dd1ff"
>> name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
>> requested_mask="r" denied_mask="r" fsuid=469 ouid=0
>>
>> Squelch the denial by allowing read access to /proc/<pid>/cmdline.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>
>> Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is the
>> qemu process. I must admit it is not clear to me why
>> /proc/<libvirtd-pid>/cmdline is read on domain shutdown.
>>
>>   examples/apparmor/libvirt-qemu | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/examples/apparmor/libvirt-qemu
>> b/examples/apparmor/libvirt-qemu
>> index 73bdbae87..3d9eed9ec 100644
>> --- a/examples/apparmor/libvirt-qemu
>> +++ b/examples/apparmor/libvirt-qemu
>> @@ -25,6 +25,7 @@
>>     /dev/ptmx rw,
>>     /dev/kqemu rw,
>>     @{PROC}/*/status r,
>> +  @{PROC}/@{pid}/cmdline r,
> 
> Note this is an information leak and allows reading potentially
> sensitive information, such as passwords given on the command line. Eg:
> 
> $ cat /proc/13335/cmdline | tr '\0' ' '
> sh /tmp/testme --password=sensitive

Good point.

> Would it be possible to use 'owner' match? Eg:
> 
>    owner @{PROC}/@{pid}/cmdline r,

I still see the denial with 'owner'. I suppose that would only work if qemu is 
invoked with libvirtd euid.

> Either way, I think a comment is warranted above the rule stating it is
> an information leak.

How about the below squashed in? Or drop this patch and live with the denial? I 
think the only side-affect is inability for qemu to report the signaling process 
name.

Regards,
Jim

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 3d9eed9ec..d4fad85a1 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -25,6 +25,9 @@
    /dev/ptmx rw,
    /dev/kqemu rw,
    @{PROC}/*/status r,
+  # When qemu is signaled to terminate, it will read cmdline of signaling
+  # process for reporting purposes. Allowing read access to a process
+  # cmdline may leak sensitive information embedded in the cmdline.
    @{PROC}/@{pid}/cmdline r,
    # Per man(5) proc, the kernel enforces that a thread may
    # only modify its comm value or those in its thread group.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by Michal Privoznik 6 years, 4 months ago 
On 12/01/2017 02:26 PM, Jamie Strandboge wrote:
> On Thu, 2017-11-30 at 10:43 -0700, Jim Fehlig wrote:
>> Noticed the following denial in audit.log when shutting down
>> an apparmor confined domain
>>
>> type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
>> operation="open" profile="libvirt-66154842-e926-4f92-92f0-
>> 1c1bf61dd1ff"
>> name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
>> requested_mask="r" denied_mask="r" fsuid=469 ouid=0
>>
>> Squelch the denial by allowing read access to /proc/<pid>/cmdline.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>
>> Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is the
>> qemu process. I must admit it is not clear to me why
>> /proc/<libvirtd-pid>/cmdline is read on domain shutdown.
>>
>>  examples/apparmor/libvirt-qemu | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/examples/apparmor/libvirt-qemu
>> b/examples/apparmor/libvirt-qemu
>> index 73bdbae87..3d9eed9ec 100644
>> --- a/examples/apparmor/libvirt-qemu
>> +++ b/examples/apparmor/libvirt-qemu
>> @@ -25,6 +25,7 @@
>>    /dev/ptmx rw,
>>    /dev/kqemu rw,
>>    @{PROC}/*/status r,
>> +  @{PROC}/@{pid}/cmdline r,
> 
> Note this is an information leak and allows reading potentially
> sensitive information, such as passwords given on the command line. Eg:
> 
> $ cat /proc/13335/cmdline | tr '\0' ' '
> sh /tmp/testme --password=sensitive

Well, I'd say that passing passwords (or any sensitive information)
through command line is doomed by definition. Anybody can read that
(doing mere ps is enough).

> 
> Would it be possible to use 'owner' match? Eg:
> 
>   owner @{PROC}/@{pid}/cmdline r,

Okay, this narrows the attack surface, but I guess that somebody else
doing `ps' on the system will be able to obtain the password anyway.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by Jim Fehlig 6 years, 4 months ago 
On 12/04/2017 04:03 AM, Michal Privoznik wrote:
> On 12/01/2017 02:26 PM, Jamie Strandboge wrote:
>> On Thu, 2017-11-30 at 10:43 -0700, Jim Fehlig wrote:
>>> Noticed the following denial in audit.log when shutting down
>>> an apparmor confined domain
>>>
>>> type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
>>> operation="open" profile="libvirt-66154842-e926-4f92-92f0-
>>> 1c1bf61dd1ff"
>>> name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
>>> requested_mask="r" denied_mask="r" fsuid=469 ouid=0
>>>
>>> Squelch the denial by allowing read access to /proc/<pid>/cmdline.
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>> ---
>>>
>>> Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is the
>>> qemu process. I must admit it is not clear to me why
>>> /proc/<libvirtd-pid>/cmdline is read on domain shutdown.
>>>
>>>   examples/apparmor/libvirt-qemu | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/examples/apparmor/libvirt-qemu
>>> b/examples/apparmor/libvirt-qemu
>>> index 73bdbae87..3d9eed9ec 100644
>>> --- a/examples/apparmor/libvirt-qemu
>>> +++ b/examples/apparmor/libvirt-qemu
>>> @@ -25,6 +25,7 @@
>>>     /dev/ptmx rw,
>>>     /dev/kqemu rw,
>>>     @{PROC}/*/status r,
>>> +  @{PROC}/@{pid}/cmdline r,
>>
>> Note this is an information leak and allows reading potentially
>> sensitive information, such as passwords given on the command line. Eg:
>>
>> $ cat /proc/13335/cmdline | tr '\0' ' '
>> sh /tmp/testme --password=sensitive
> 
> Well, I'd say that passing passwords (or any sensitive information)
> through command line is doomed by definition. Anybody can read that
> (doing mere ps is enough).
> 
>>
>> Would it be possible to use 'owner' match? Eg:
>>
>>    owner @{PROC}/@{pid}/cmdline r,
> 
> Okay, this narrows the attack surface, but I guess that somebody else
> doing `ps' on the system will be able to obtain the password anyway.

Prefixing the rule with 'owner' didn't work as noted earlier in the thread

https://www.redhat.com/archives/libvir-list/2017-December/msg00031.html

I've pushed the patch with the comment squashed in, albeit dangerously close to 
the release :-).

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
Posted by Jamie Strandboge 6 years, 4 months ago 
On Mon, 2017-12-04 at 12:03 +0100, Michal Privoznik wrote:
> On 12/01/2017 02:26 PM, Jamie Strandboge wrote:
> > On Thu, 2017-11-30 at 10:43 -0700, Jim Fehlig wrote:
> > > Noticed the following denial in audit.log when shutting down
> > > an apparmor confined domain
> > > 
> > > type=AVC msg=audit(1512002299.742:131): apparmor="DENIED"
> > > operation="open" profile="libvirt-66154842-e926-4f92-92f0-
> > > 1c1bf61dd1ff"
> > > name="/proc/1475/cmdline" pid=2958 comm="qemu-system-x86"
> > > requested_mask="r" denied_mask="r" fsuid=469 ouid=0
> > > 
> > > Squelch the denial by allowing read access to
> > > /proc/<pid>/cmdline.
> > > 
> > > Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> > > ---
> > > 
> > > Note: In the audit.log snippet, PID 1475 is libvirtd and 2958 is
> > > the
> > > qemu process. I must admit it is not clear to me why
> > > /proc/<libvirtd-pid>/cmdline is read on domain shutdown.
> > > 
> > >  examples/apparmor/libvirt-qemu | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/examples/apparmor/libvirt-qemu
> > > b/examples/apparmor/libvirt-qemu
> > > index 73bdbae87..3d9eed9ec 100644
> > > --- a/examples/apparmor/libvirt-qemu
> > > +++ b/examples/apparmor/libvirt-qemu
> > > @@ -25,6 +25,7 @@
> > >    /dev/ptmx rw,
> > >    /dev/kqemu rw,
> > >    @{PROC}/*/status r,
> > > +  @{PROC}/@{pid}/cmdline r,
> > 
> > Note this is an information leak and allows reading potentially
> > sensitive information, such as passwords given on the command line.
> > Eg:
> > 
> > $ cat /proc/13335/cmdline | tr '\0' ' '
> > sh /tmp/testme --password=sensitive
> 
> Well, I'd say that passing passwords (or any sensitive information)
> through command line is doomed by definition. Anybody can read that
> (doing mere ps is enough).
> 
> > 
> > Would it be possible to use 'owner' match? Eg:
> > 
> >   owner @{PROC}/@{pid}/cmdline r,
> 
> Okay, this narrows the attack surface, but I guess that somebody else
> doing `ps' on the system will be able to obtain the password anyway.

Sure, but what is different here is that the qemu processes are
intended to be confined/untrusted whereas other processes on the system
(eg, the user's shell) are not. Therefore, IMO, we should try to avoid
information leaks in the qemu policy where possible.

-- 
Jamie Strandboge             | http://www.canonical.com--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.