[libvirt] [PATCH] apparmor: allow qemu abstraction to read /proc/pid/cmdline
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
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
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
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
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.
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
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
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
participants (4)
-
intrigeri -
Jamie Strandboge -
Jim Fehlig -
Michal Privoznik