[libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler

Thu Jun 15 14:45:02 UTC 2017

On Thu, Jun 15, 2017 at 04:40:49PM +0200, Pavel Hrdina wrote:
> On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote:
> > 
> > 
> > On 06/15/2017 03:11 AM, Pavel Hrdina wrote:
> > > On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
> > >>
> > >>
> > >> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
> > >>> In the case that virtlogd is used as stdio handler we pass to QEMU
> > >>> only FD to a PIPE connected to virtlogd instead of the file itself.
> > >>>
> > >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
> > >>>
> > >>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > >>> ---
> > >>>
> > >>> Notes:
> > >>>     new in v2
> > >>>
> > >>>  src/lxc/lxc_process.c            |  6 ++---
> > >>>  src/qemu/qemu_security.c         |  9 +++++--
> > >>>  src/security/security_apparmor.c |  7 ++++--
> > >>>  src/security/security_dac.c      | 54 +++++++++++++++++++++++++++++++---------
> > >>>  src/security/security_driver.h   |  6 +++--
> > >>>  src/security/security_manager.c  | 12 ++++++---
> > >>>  src/security/security_manager.h  |  6 +++--
> > >>>  src/security/security_nop.c      |  6 +++--
> > >>>  src/security/security_selinux.c  | 53 ++++++++++++++++++++++++++++++---------
> > >>>  src/security/security_stack.c    | 12 ++++++---
> > >>>  tests/securityselinuxlabeltest.c |  2 +-
> > >>>  11 files changed, 127 insertions(+), 46 deletions(-)
> > >>>
> > >>
> > >> Why is it a (!chr_seclabel && chardevStdioLogd)?  More to the point why
> > >> is (!chr_seclabel) even matter?
> > > 
> > > If you configure the label we shouldn't ignore it in some cases, that's
> > > just wrong.  If the label is set for the char device we will configure
> > > it every time even if it will fail to start the guest, it's a
> > > responsibility of the user to configure proper label if it is provided.
> > > 
> > 
> > When email doesn't convey the question... Ugh...  I'm also trying to
> > speed learn an area of the code and review at the same time.
> > 
> > If I go back to commit id 'f8b08d0e' where the original implementation
> > to add labels for chardevs was done (but has been modified for patch 1
> > to change where the label is stored), I get the impression that a label
> > should be added either from something specifically supplied for the
> > <chardev> or to use the per domain one:
> > 
> > "The source element may contain an optional seclabel to override the way
> > that labelling is done on the socket path. If this element is not
> > present, the security label is inherited from the per-domain setting."
> > 
> > So if I look at the condition "(!chr_seclabel && chardevStdioLogd)"
> > added by this patch to decide whether or not to supply the label, I'm
> > left with the impression that if for this particular chardev a label
> > doesn't exist *and* the configuration option chardevStdioLogd is true,
> > then we're going to return happy status *and* not inherit the per-domain
> > setting.
> > 
> > So the bug is then that applying the default domain label for a chardev
> > configured to use a stdio handle is incorrect?  Perhaps I didn't get
> > that from reading bz or the patch.
> 
> Yes, that's the bug.  If virtlogd is used to handle stdio for char
> devices we shouldn't relabel the @path to the default labels, the
> @path doesn't have to be accessible by the qemu process, it has to be
> accessible by the virtlogd process.
> 
> > >> IIUC, whether or not someone set the label for the chardev, for this
> > >> particular issue/config where the chardev has a <log file=$path>, we
> > >> don't want to set (or restore) the label. I feel like I'm missing
> > >> something subtle. Maybe a bit more explanation of the adjustment would
> > >> help me...
> > > 
> > > This is not for the <log file=$path/> but for the <source path=$path/>.
> > > We don't relabel $path for <log file=$path/> at all.
> > > 
> > 
> > hmm.. ah, right... I kept scrolling back and forth in the bz and the
> > docs, but missed this in the bz:
> > 
> > 3) Check the virtlogd.log:
> > error : virRotatingFileWriterEntryNew:113 : Unable to open file:
> > /var/log/libvirt/qemu/log: Permission denied
> > 
> > I guess I got lost in the power of suggestion of reading the docs
> > regarding the "optional log file" that can be associated paragraph and
> > trying to learn on the fly so that you at least get a review in a
> > somewhat timely manner ;-)
> > 
> > >> Wouldn't these changes end up selecting "any" chardev if
> > >> chardevStdioLogd ended up being set regardless of whether they were
> > >> actually using the log file?
> > > 
> > > I don't know what you mean by this sentence?
> > > 
> > 
> > Well let's see, chardevStdioLogd is set to true when meeting the two
> > conditions a qemu.conf global "cfg->stdioLogd" && a per domain or
> > emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND".
> > 
> > So, conceivably chardevStdioLogd could be true for *any* domain as long
> > as those conditions are met, right?
> 
> Yes, the two conditions are checked while starting a new domain in
> qemuProcessPrepareDomain() and stored in the private date of that
> domain.
> 
> > If you have a domain that has chardev's which are not configured to use
> > the stdio handler, then the chardevStdioLogd could still be true, right?
> 
> No, if the @chardevStdioLogd is true all char devices for that domain
> will use virtlogd.
> 
> The only issue I've just found out is that the code path for chardev
> hot-plug isn't updated to use virtlogd when it should be so for
> hot-plugged char devices we pass the path directly to QEMU.
> 
> With this patch applied the hot-plug fails if virtlogd is used because
> we don't relabel the path but we pass it directly to QEMU, this needs to
> be fixed to not introduce a regression, sigh.

Ok, I've tried it without this patch and it still fails with permission
denied so there is no regression, we just don't relabel the hot-plugged
chardev at all.  Still it should be fixed.

> Pavel
> 
> > If that's the case and the chardev doesn't have a label associated, then
> > we just return happy status and we do not inherit the per domain
> > setting. Wouldn't that be incorrect?
> > 
> > My concern is more we're making a change in a (mostly) common set of
> > functions for a (very) specific problem.
> > 
> > 
> > >> As an aside, I think there's an "oddity" when it comes to the Restore,
> > >> but I'm not sure how to put it into words exactly. If a guest is running
> > >> code prior to this set of changes, would it have successfully run a Set?
> > >> If so, then after applying this change and restarting, the label
> > >> wouldn't be reset, right?  What happens at guest shutdown - does the
> > >> label not get unset now?  Of course this is all "interaction" with
> > >> virtlogd restart that really throws a monkey wrench into things.
> > > 
> > > No, that's not correct.  The @chardevStdioLogd is stored in the status
> > > XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml).  So when
> > > the libvirtd is stopped and started with this patch applied the status
> > > XML doesn't have the @chardevStdioLogd stored in it so it will be false
> > > and we will reset the label.  The @chardevStdioLogd is updated only when
> > > the domain is started and we will store the value in the status XML
> > > only with new libvirtd and only in that case we will not set/restore
> > > the label.
> > > 
> > 
> > hmmm.. Reading the bz indicates the 'virtlogd' daemon restarting... This
> > is where all this gets a bit "odd" for me. Like I said it was a weird
> > thing to even try and explain, but I think you talked me off the ledge
> > of concern.
> > 
> > John
> > 
> > >> Also, why is the Smartcard chardev handling not using this
> > > 
> > > The smartcard doesn't ever use virtlogd as stdio handler.
> > > 
> > > Pavel
> > > 
> > 
> > --
> > libvir-list mailing list
> > libvir-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170615/86c3c8ae/attachment-0001.sig>