There is a case in which we do not want 'remember' to be
set to true in SetOwnership() calls inside the
HostdevLabelHelper() functions of both DAC and SELinux drivers.
Next patch will explain and handle that scenario.
For now, let's make virSecurityDACSetOwnership() and
virSecuritySELinuxSetHostdevLabelHelper() accept a 'remember'
flag, which will be used to set the 'remember' parameter
of their respective SetOwnership() calls. No functional
change is made.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
After one of previous commits (v5.10.0-524-gce56408e5f) there is
a variable left unused in verify_xpath_context() which breaks the
build.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Currently, when security driver is not available users are informed that
it wasn't found which can be confusing.
1. Update error message
2. Add comment to domain doc
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by Sebastian Mitterle <smitterl@redhat.com>
/dev/tap* is an invalid path but it works with lax policy.
Make it work with more accurate policy as well
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dominick Grift <dac.override@gmail.com>
The GLib g_lstat() function provides a portable impl for
Win32.
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Eliminate direct use of normal setenv/unsetenv calls in
favour of GLib's wrapper. This eliminates two gnulib
modules
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This function is currently not called for any type of storage
source that is not considered 'local' (as defined by
virStorageSourceIsLocalStorage()). Well, NVMe disks are not
'local' from that point of view and therefore we will need to
call this function more frequently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Teach virt-aa-helper how to label a qcow2 data_file, tracked internally
as externalDataStore. It should be treated the same as its sibling
disk image
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This parameter is now unused and can be removed entirely.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Moving their instance parameter to be the first one, and give consistent
ordering of other parameters across all functions. Ensure that the xml
options are passed into both functions in prep for future work.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Shared memory devices need qemu to be able to access certain paths
either for the shared memory directly (mostly ivshmem-plain) or for a
socket (mostly ivshmem-doorbell).
Add logic to virt-aa-helper to render those apparmor rules based
on the domain configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1761645
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
There are currently broken use cases, e.g. snapshotting more than one disk at
once like:
$ virsh snapshot-create-as --domain eoan --disk-only --atomic
--diskspec vda,snapshot=no --diskspec vdb,snapshot=no
--diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external
--diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external
The command above will iterate from qemuDomainSnapshotCreateDiskActive and
eventually add /test/disk1.snapshot1.qcow first (appears in the rules)
to then later add /test/disk2.snapshot1.qcow and while doing so throwing
away the former rule causing it to fail.
All other calls to (re)load_profile already use append=true when adding
rules append=false is only used when restoring rules [1].
Fix this by letting AppArmorSetSecurityImageLabel use append=true as well.
Since this is removing a (unintentional) trigger to revoke all rules
appended so far we agreed on review to do some tests, but in the tests
no rules came back on:
- hot-plug
- hot-unplug
- snapshotting
Bugs:
https://bugs.launchpad.net/libvirt/+bug/1845506https://bugzilla.redhat.com/show_bug.cgi?id=1746684
[1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
A lot of the code in AppArmorSetSecurityImageLabel is a duplicate of
what is in reload_profile, this refactors AppArmorSetSecurityImageLabel
to use reload_profile instead.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
reload_profile calls get_profile_name for no particular gain, lets
remove that call. The string isn't used in that function later on
and not registered/passed anywhere.
It can only fail if it either can't allocate or if the
virDomainDefPtr would have no uuid set (which isn't allowed).
Thereby the only "check" it really provides is if it can allocate the
string to then free it again.
This was initially added in [1] when the code was still in
AppArmorRestoreSecurityImageLabel (later moved) and even back then had
no further effect than described above.
[1]: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/security_apparmor.c;h=16de0f26f41689e0c50481120d9f8a59ba1f4073;hb=bbaecd6a8f15345bc822ab4b79eb0955986bb2fd#l487
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
While only used internally from libvirt the options still are misleading
enough to cause issues every now and then.
Group modes, options and an adding extra file and extend the wording of
the latter which had the biggest lack of clarity.
Both add a file to the end of the rules, but one re-generates the
rules from XML and the other keeps the existing rules as-is not
considering the XML content.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Glib implementation follows the ISO C99 standard so it's safe to replace
the gnulib implementation.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The function now does not return an error so we can drop it fully.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All OSes that we support have libselinux >= 2.5 except for Ubuntu 16.04
where the version is 2.4.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace all occurrences of
if (VIR_STRDUP(a, b) < 0)
/* effectively dead code */
with:
a = g_strdup(b);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Replace:
if (!s && VIR_STRDUP(s, str) < 0)
goto;
with:
if (!s)
s = g_strdup(str);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Replace all the occurrences of
ignore_value(VIR_STRDUP(a, b));
with
a = g_strdup(b);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Since commit 44e7f02915
util: rewrite auto cleanup macros to use glib's equivalent
VIR_AUTOPTR aliases to g_autoptr. Replace all of its use by the GLib
macro version.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Since commit 44e7f02915
util: rewrite auto cleanup macros to use glib's equivalent
VIR_AUTOFREE is just an alias for g_autofree. Use the GLib macros
directly instead of our custom aliases.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In order to have multiple security drivers hidden under one
virSecurity* call, we have virSecurityStack driver which holds a
list of registered security drivers and for every virSecurity*
call it iterates over the list and calls corresponding callback
in real security drivers. For instance, for
virSecurityManagerSetAllLabel() it calls
domainSetSecurityAllLabel callback sequentially in NOP, DAC and
(possibly) SELinux or AppArmor drivers. This works just fine if
the callback from every driver returns success. Problem arises
when one of the drivers fails. For instance, aforementioned
SetAllLabel() succeeds for DAC but fails in SELinux in which
case all files that DAC relabelled are now owned by qemu:qemu (or
whomever runs qemu) and thus permissions are leaked. This is even
more visible with XATTRs which remain set for DAC.
The solution is to perform a rollback on failure, i.e. call
opposite action on drivers that succeeded.
I'm providing rollback only for set calls and intentionally
omitting restore calls for two reasons:
1) restore calls are less likely to fail (they merely remove
XATTRs and chown()/setfilecon() file - all of these operations
succeeded in set call),
2) we are not really interested in restore failures - in a very
few places we check for retval of a restore function we do so
only to print a warning.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740024
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
In near future we will need to walk through the list of internal
drivers in reversed order. The simplest solution is to turn
singly linked list into a doubly linked list.
We will not need to start from the end really, so there's no tail
pointer kept.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
This function returns the name of the secdriver. Since the name
is invariant we don't really need to lock the manager - it won't
change.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
This function is in fact returning the name of the virtualization
driver that registered the security manager/driver.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
In upcoming commits, virSecurityManagerSetAllLabel() will perform
rollback in case of failure by calling
virSecurityManagerRestoreAllLabel(). But in order to do that, the
former needs to have @migrated argument so that it can be passed
to the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
The usleep function was missing on older mingw versions, but we can rely
on it existing everywhere these days. It may only support times upto 1
second in duration though, so we'll prefer to use g_usleep instead.
The commandhelper program is not changed since that can't link to glib.
Fortunately it doesn't need to build on Windows platforms either.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Add the main glib.h to internal.h so that all common code can use it.
Historically glib allowed applications to register an alternative
memory allocator, so mixing g_malloc/g_free with malloc/free was not
safe.
This was feature was dropped in 2.46.0 with:
commit 3be6ed60aa58095691bd697344765e715a327fc1
Author: Alexander Larsson <alexl@redhat.com>
Date: Sat Jun 27 18:38:42 2015 +0200
Deprecate and drop support for memory vtables
Applications are still encourged to match g_malloc/g_free, but it is no
longer a mandatory requirement for correctness, just stylistic. This is
explicitly clarified in
commit 1f24b36607bf708f037396014b2cdbc08d67b275
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Thu Sep 5 14:37:54 2019 +0100
gmem: clarify that g_malloc always uses the system allocator
Applications can still use custom allocators in general, but they must
do this by linking to a library that replaces the core malloc/free
implemenentation entirely, instead of via a glib specific call.
This means that libvirt does not need to be concerned about use of
g_malloc/g_free causing an ABI change in the public libary, and can
avoid memory copying when talking to external libraries.
This patch probes for glib, which provides the foundation layer with
a collection of data structures, helper APIs, and platform portability
logic.
Later patches will introduce linkage to gobject which provides the
object type system, built on glib, and gio which providing objects
for various interesting tasks, most notably including DBus client
and server support and portable sockets APIs, but much more too.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We mirror the labeling strategy that was used for its top image
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This will be used for recursing into externalDataStore
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Rename the existing virSecuritySELinuxRestoreImageLabelInt
to virSecuritySELinuxRestoreImageLabelSingle, and extend the new
ImageLabelInt handle externalDataStore
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This will simplify future patches and make the logic easier to follow
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The only caller always passes in a non-null parent
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
All the SetFileCon calls only differ by the label they pass in.
Rework the conditionals to track what label we need, and use a
single SetFileCon call
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
We mirror the labeling strategy that was used for its sibling
image
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This will be used for recursing into externalDataStore
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Rename the existing virSecurityDACRestoreImageLabelInt
to virSecurityDACRestoreImageLabelSingle, and extend the new
ImageLabelInt handle externalDataStore
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This will simplify future patches and make the logic easier to follow
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The only caller always passes in a non-null parent
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1755803
The /dev/tpmN file can be opened only once, as implemented in
drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any
other attempt to open the file fails. And since we're opening the
file ourselves and passing the FD to qemu we will not succeed
opening the file again when locking it for seclabel remembering.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
While in most cases we want to remember/recall label for a
chardev, there are some special ones (like /dev/tpm0) where we
don't want to remember the seclabel nor recall it. See next
commit for rationale behind.
While the easiest way to implement this would be to just add new
argument to virSecurityDACSetChardevLabel() this one is also a
callback for virSecurityManagerSetChardevLabel() and thus has
more or less stable set of arguments. Therefore, the current
virSecurityDACSetChardevLabel() is renamed to
virSecurityDACSetChardevLabelHelper() and the original function
is set to call the new one.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
So far all items on the chown/setfilecon list have the same
.remember value. But this will change shortly. Therefore, don't
try to lock paths which we won't manipulate XATTRs for.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
On Fedora, already whitelisted paths to AAVMF and OVMF binaries
are symlinks to binaries under /usr/share/edk2/. Add that directory
to the RO whitelist so virt-aa-helper-test passes
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This will simplify adding support for qcow2 external data_file
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This is closer to what security_selinux.c does, and will help add
support for qcow2 external data_files
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This mirrors the code layout in security_selinux.c. It will also make
it easier to share the checks for qcow2 external data_file support
eventually
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The virStorageSource must have everything it needs
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
There's only one caller, so open code the file_add_path behavior
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
true is always passed here, so delete the unused code path and
adjust the associated comment
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
It is the only user. Rename it to match the local style
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The AppArmor profile generated by virt-aa-helper is too strict for swtpm.
This change contains 2 small fixes:
- Relax append access to swtpm's log file to permit write access instead.
Append access is insufficient because the log is opened with O_CREAT.
- Permit swtpm to acquire a lock on its lock file.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Neither virThreadInitialize or virThreadOnExit do anything since we
dropped the Win32 threads impl, in favour of win-pthreads with:
commit 0240d94c36
Author: Daniel P. Berrange <berrange@redhat.com>
Date: Wed Jan 22 16:17:10 2014 +0000
Remove windows thread implementation in favour of pthreads
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
AppArmorGetSecurityProcessLabel copies the VM's profile name to the
label member of virSecurityLabel struct. If the profile is not loaded,
the name is set empty before calling virStrcpy to copy it. However,
virStrcpy will fail if src is empty (0 length), causing
AppArmorGetSecurityProcessLabel to needlessly fail. Simple operations
that report security driver information will subsequently fail
virsh dominfo test
Id: 248
Name: test
...
Security model: apparmor
Security DOI: 0
error: internal error: error copying profile name
Avoid copying an empty profile name when the profile is not loaded.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The Perl bindings for libvirt use the test driver for unit tests. This
tries to load the cpu_map/index.xml file, and when run from an
uninstalled build will fail.
The problem is that virFileActivateDirOverride is called by our various
binaries like libvirtd, virsh, but is not called when a 3rd party app
uses libvirt.so
To deal with this we allow the LIBVIRT_DIR_OVERRIDE=1 env variable to be
set and make virInitialize look for this. The 'run' script will set it,
so now build using this script to run against an uninstalled tree we
will correctly resolve files to the source tree.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
I guess the reason for that was the automatic interpretation/stringification of
setfilecon_errno, but the code was not nice to read and it was a bit confusing.
Also, the logs and error states get cleaner this way.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
There are some network file systems that do support XATTRs (e.g.
gluster via FUSE). And they appear to support SELinux too.
However, not really. Problem is, that it is impossible to change
SELinux label of a file stored there, and yet we claim success
(rightfully - hypervisor succeeds in opening the file). But this
creates a problem for us - from XATTR bookkeeping POV, we haven't
changed the label and thus if we remembered any label, we must
roll back and remove it.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740506
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This function is no longer needed because after previous commits
it's just an alias to virSecuritySELinuxSetFilecon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Now, that we don't need to remember if setting context is
'optional' (the argument only made
virSecuritySELinuxSetFileconImpl() return a different success
code), we can drop it from the _virSecuritySELinuxContextItem
structure as we don't need to remember it in transactions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There is no real difference between
virSecuritySELinuxSetFilecon() and
virSecuritySELinuxSetFileconOptional(). Drop the latter in favour
of the former.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The only thing that the @optional argument does is that it makes
the function return 1 instead of 0 if setting SELinux context
failed in a non-critical fashion. Drop the argument then and
return 1 in that case. This enables caller to learn if SELinux
context was set or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
After 7cfb7aab57 commit starting a domain pullutes logs with
warnings like [1]. The reason is resource files do not
have timestamp before starting a domain and after destroying
domain the timestamp is cleared. Let's check the timestamp
only if attribute with refcounter is found.
[1] warning : virSecurityValidateTimestamp:198 : Invalid XATTR timestamp detected on \
/some/path secdriver=dac
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
All code using LOCALSTATEDIR "/run" is updated to use RUNSTATEDIR
instead. The exception is the remote driver client which still
uses LOCALSTATEDIR "/run". The client needs to connect to remote
machines which may not be using /run, so /var/run is more portable
due to the /var/run -> /run symlink.
Some duplicate paths in the apparmor code are also purged.
There's no functional change by default yet since both expressions
expand to the same value.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
It may happen that we leave some XATTRs behind. For instance, on
a sudden power loss, the host just shuts down without calling
restore on domain paths. This creates a problem, because when the
host starts up again, the XATTRs are there but they don't reflect
the true state and this may result in libvirt denying start of a
domain.
To solve this, save a unique timestamp (host boot time) among
with our XATTRs.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741140
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
If user has two domains, each have the same disk (configured for
RW) but each runs with different seclabel then we deny start of
the second domain because in order to do that we would need to
relabel the disk but that would cut the first domain off. Even if
we did not do that, qemu would fail to start because it would be
unable to lock the disk image for the second time. So far, this
behaviour is expected. But what is not expected is that we
increase the refcounter in XATTRs and leave it like that.
What happens is that when the second domain starts,
virSecuritySetRememberedLabel() is called, and since there are
XATTRs from the first domain it increments the refcounter and
returns it (refcounter == 2 at this point). Then callers
(virSecurityDACSetOwnership() and
virSecuritySELinuxSetFileconHelper()) realize that refcounter is
greater than 1 and desired seclabel doesn't match the one the
disk image already has and an error is produced. But the
refcounter is never decremented.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740024
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Apparently /proc/self is automatically converted to /proc/@{pid}
before checking rules, which makes spelling it out explicitly
redundant.
Suggested-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Since
commit 432faf259b
Author: Michal Privoznik <mprivozn@redhat.com>
Date: Tue Jul 2 19:49:51 2019 +0200
virCommand: use procfs to learn opened FDs
When spawning a child process, between fork() and exec() we close
all file descriptors and keep only those the caller wants us to
pass onto the child. The problem is how we do that. Currently, we
get the limit of opened files and then iterate through each one
of them and either close() it or make it survive exec(). This
approach is suboptimal (although, not that much in default
configurations where the limit is pretty low - 1024). We have
/proc where we can learn what FDs we hold open and thus we can
selectively close only those.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
v5.5.0-173-g432faf259b
programs using the virCommand APIs on Linux need read access to
/proc/self/fd, or they will fail like
error : virCommandWait:2796 : internal error: Child process
(LIBVIRT_LOG_OUTPUTS=3:stderr /usr/lib/libvirt/virt-aa-helper -c
-u libvirt-b20e9a8e-091a-45e0-8823-537119e98bc6) unexpected exit
status 1: libvirt: error : cannot open directory '/proc/self/fd':
Permission denied
virt-aa-helper: error: apparmor_parser exited with error
Update the AppArmor profile for virt-aa-helper so that read access
to the relevant path is granted.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The way we're processing the return status, using WIFEXITED() and
friends, only works when we have the raw return status; however,
virCommand defaults to processing the return status for us. Call
virCommandRawStatus() before virCommandRun() so that we get the raw
return status and the logic can actually work.
This results in guest startup failures caused by AppArmor issues
being reported much earlier: for example, if virt-aa-helper exits
with an error we're now reporting
error: internal error: cannot load AppArmor profile 'libvirt-b20e9a8e-091a-45e0-8823-537119e98bc6'
instead of the misleading
error: internal error: Process exited prior to exec: libvirt:
error : unable to set AppArmor profile 'libvirt-b20e9a8e-091a-45e0-8823-537119e98bc6'
for '/usr/bin/qemu-system-x86_64': No such file or directory
Suggested-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Right now we're using the virRun() convenience API, but that
doesn't allow the kind of control we want. Use the virCommand
APIs directly instead.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If there are two paths on the list that are the same we need to
lock it only once. Because when we try to lock it the second time
then open() fails. And if it didn't, locking it the second time
would fail for sure. After all, it is sufficient to lock all
paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Document why we need to sort paths while it's still fresh in my
memory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
VHD images can be used as any other, so we should add them to the list
of types that virt-aa-helper can read when creating the per-guest rules
for backing files.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
A simple helper function that would be used from DAC and SELinux
drivers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The purpose of this API is to allow caller move XATTRs (or remove
them) from one file to another. This will be needed when moving
top level of disk chain (either by introducing new HEAD or
removing it).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This effectively reverts d7420430ce and adds new code.
Here is the problem: Imagine a file X that is to be shared
between two domains as a disk. Let the first domain (vm1) have
seclabel remembering turned on and the other (vm2) has it turned
off. Assume that both domains will run under the same user, but
the original owner of X is different (i.e. trying to access X
without relabelling leads to EPERM).
Let's start vm1 first. This will cause X to be relabelled and to
gain new attributes:
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.dac="$originalOwner"
When vm2 is started, X will again be relabelled, but since the
new label is the same as X already has (because of vm1) nothing
changes and vm1 and vm2 can access X just fine. Note that no
XATTR is changed (especially the refcounter keeps its value of 1)
because the vm2 domain has the feature turned off.
Now, vm1 is shut off and vm2 continues running. In seclabel
restore process we would get to X and since its refcounter is 1
we would restore the $originalOwner on it. But this is unsafe to
do because vm2 is still using X (remember the assumption that
$originalOwner and vm2's seclabel are distinct?).
The problem is that refcounter stored in XATTRs doesn't reflect
the actual times a resource is in use. Since I don't see any easy
way around it let's just not store original owner on shared
resources. Shared resource in world of domain disks is:
- whole backing chain but the top layer,
- read only disk (we don't require CDROM to be explicitly
marked as shareable),
- disk marked as shareable.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Just like previous commit allowed to enable or disable owner
remembering for each individual path, do the same for SELinux
driver. This is going to be needed in the next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
One caller in particular (virSecurityDACSetImageLabelInternal)
will want to have the feature turned on only in some cases.
Introduce @remember member to _virSecurityDACChownItem to track
whether caller wants to do owner remembering or not.
The actual remembering is then enabled if both caller wanted it
and the feature is turned on in the config file.
Technically, we could skip over paths that don't have remember
enabled when creating a list of paths to lock. We won't touch
their XATTRs after all. Well, I rather play it safe and keep them
on the locking list for now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Both DAC and SELinux drivers support transactions. Each item on
the transaction list consists of various variables and @restore
is one of them. Document it so that as the list of variables grow
it's easier to spot which variable does what.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The way that virSecurityDACRecallLabel is currently written is
that if XATTRs are not supported for given path to the caller
this is not different than if the path is still in use. The value
of 1 is returned which makes secdrivers skip label restore.
This is clearly a bug as we are not restoring labels on say NFS
even though previously we were.
Strictly speaking, changes to virSecurityDACRememberLabel are not
needed, but they are done anyway so that getter and setter behave
in the same fashion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The way that security drivers use XATTR is kind of verbose. If
error reporting was left for caller then the caller would end up
even more verbose.
There are two places where we do not want to report error if
virFileGetXAttr fails. Therefore virFileGetXAttrQuiet is
introduced as an alternative that doesn't report errors.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Just like it's DAC counterpart is doing,
virSecuritySELinuxRestoreAllLabel() could print @migrated in the
debug message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit a3ab6d42 changed the libvirtd profile to a named profile
but neglected to accommodate the change in the qemu profile
ptrace and signal rules.
Later on 4ec3cf9a fixed that for ptrace and signal but openGraphicsFD
is still missing.
As a result, libvirtd is unable to open UI on libvirt >=5.1 e.g. with
virt-manager.
Add openGraphicsFD rule that references the libvirtd profile
by name in addition to full binary path.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1833040
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Various binaries are statically linking to libvirt_util.la and
other intermediate libraries we build. These intermediate libs
all get built into the main libvirt.so shared library eventually,
so we can dynamically link to that instead and reduce the on disk
footprint.
In libvirt-daemon RPM:
virtlockd: 1.6 MB -> 153 KB
virtlogd: 1.6 MB -> 157 KB
libvirt_iohelper: 937 KB -> 23 KB
In libvirt-daemon-driver-network RPM:
libvirt_leaseshelper: 940 KB -> 26 KB
In libvirt-daemon-driver-storage-core RPM:
libvirt_parthelper: 926 KB -> 21 KB
IOW, about 5.6 MB total space saving in a build done on Fedora 30
x86_64 architecture.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
