1. Patchew
  2. Libvirt
  3. View series

[PATCH V2] security: Ensure file exists before attempting to restore label

Jim Fehlig posted 1 patch 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20240401221535.5510-1-jfehlig@suse.com
src/security/security_dac.c     |  3 +++
src/security/security_selinux.c |  2 ++
tests/qemusecuritymock.c        | 18 ++++++++++++++++++
3 files changed, 23 insertions(+)
[PATCH V2] security: Ensure file exists before attempting to restore label
Posted by Jim Fehlig 1 month ago 
When performing an install, it's common for tooling such as virt-install
to remove the install kernel/initrd once they are successfully booted and
the domain has been redefined to boot without them. After the installation
is complete and the domain is rebooted/shutdown, the DAC and selinux
security drivers attempt to restore labels on the now deleted files. It's
harmles wrt functionality, but results in error messages such as

Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir>
Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo>
Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction

Add a check for file existence to the virSecurity*RestoreFileLabel functions,
and avoid relabeling if the file is no longer available. Skipping the restore
caused failures in qemusecuritytest, which mocks stat, chown, etc as part of
ensuring the security drivers properly restore labels. virFileExists is now
mocked in qemusecuritymock.c to return true when passed a file previously
seen by the mocked stat, chown, etc functions.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/security/security_dac.c     |  3 +++
 src/security/security_selinux.c |  2 ++
 tests/qemusecuritymock.c        | 18 ++++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 567be4bd23..4e850e219e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -825,6 +825,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr,
         virStorageSourceIsLocalStorage(src))
         path = src->path;
 
+    if (!virFileExists(path))
+        return 0;
+
     /* Be aware that this function might run in a separate process.
      * Therefore, any driver state changes would be thrown away. */
 
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index b49af26e49..aaec34ff8b 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1488,6 +1488,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr,
      */
     if (!path)
         return 0;
+    if (!virFileExists(path))
+        return 0;
 
     VIR_INFO("Restoring SELinux context on '%s'", path);
 
diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c
index 80d59536b1..bb0a85544b 100644
--- a/tests/qemusecuritymock.c
+++ b/tests/qemusecuritymock.c
@@ -382,6 +382,24 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
 }
 
 
+bool virFileExists(const char *path G_GNUC_UNUSED)
+{
+    VIR_LOCK_GUARD lock = virLockGuardLock(&m);
+
+    if (getenv(ENVVAR) == NULL)
+        return access(path, F_OK) == 0;
+
+    init_hash();
+    if (virHashHasEntry(chown_paths, path))
+        return true;
+
+    if (virHashHasEntry(selinux_paths, path))
+        return true;
+
+    return false;
+}
+
+
 typedef struct _checkOwnerData checkOwnerData;
 struct _checkOwnerData {
     GHashTable *paths;
-- 
2.44.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH V2] security: Ensure file exists before attempting to restore label
Posted by Michal Prívozník 3 weeks, 3 days ago 
On 4/2/24 00:14, Jim Fehlig wrote:
> When performing an install, it's common for tooling such as virt-install
> to remove the install kernel/initrd once they are successfully booted and
> the domain has been redefined to boot without them. After the installation
> is complete and the domain is rebooted/shutdown, the DAC and selinux
> security drivers attempt to restore labels on the now deleted files. It's
> harmles wrt functionality, but results in error messages such as
> 
> Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir>
> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo>
> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
> 
> Add a check for file existence to the virSecurity*RestoreFileLabel functions,
> and avoid relabeling if the file is no longer available. Skipping the restore
> caused failures in qemusecuritytest, which mocks stat, chown, etc as part of
> ensuring the security drivers properly restore labels. virFileExists is now
> mocked in qemusecuritymock.c to return true when passed a file previously
> seen by the mocked stat, chown, etc functions.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/security/security_dac.c     |  3 +++
>  src/security/security_selinux.c |  2 ++
>  tests/qemusecuritymock.c        | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+)
> 


Looks better now, thanks!

> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 567be4bd23..4e850e219e 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -825,6 +825,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr,
>          virStorageSourceIsLocalStorage(src))
>          path = src->path;
>  
> +    if (!virFileExists(path))
> +        return 0;
> +
>      /* Be aware that this function might run in a separate process.
>       * Therefore, any driver state changes would be thrown away. */
>  
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index b49af26e49..aaec34ff8b 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1488,6 +1488,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr,
>       */
>      if (!path)
>          return 0;
> +    if (!virFileExists(path))
> +        return 0;
>  
>      VIR_INFO("Restoring SELinux context on '%s'", path);
>  
> diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c
> index 80d59536b1..bb0a85544b 100644
> --- a/tests/qemusecuritymock.c
> +++ b/tests/qemusecuritymock.c
> @@ -382,6 +382,24 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
>  }
>  
>  
> +bool virFileExists(const char *path G_GNUC_UNUSED)

This argument is used, so please drop the unused annotation.

> +{
> +    VIR_LOCK_GUARD lock = virLockGuardLock(&m);
> +
> +    if (getenv(ENVVAR) == NULL)
> +        return access(path, F_OK) == 0;

I wonder whether we should call real virFileExists() here. That way, if
a test would chain mocks they can work (though both would probably be
mocking access()). I'll leave it up to you whether you want to squash in
the following:

diff --git i/tests/qemusecuritymock.c w/tests/qemusecuritymock.c
index bb0a85544b..92e7c9ac26 100644
--- i/tests/qemusecuritymock.c
+++ w/tests/qemusecuritymock.c
@@ -66,6 +66,8 @@ static int (*real_close)(int fd);
 static int (*real_setfilecon_raw)(const char *path, const char *context);
 static int (*real_getfilecon_raw)(const char *path, char **context);
 #endif
+static bool (*real_virFileExists)(const char *file);
+
 
 
 /* Global mutex to avoid races */
@@ -123,6 +125,7 @@ init_syms(void)
     VIR_MOCK_REAL_INIT(setfilecon_raw);
     VIR_MOCK_REAL_INIT(getfilecon_raw);
 #endif
+    VIR_MOCK_REAL_INIT(virFileExists);
 
     /* Intentionally not calling init_hash() here */
 }
@@ -382,12 +385,12 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
 }
 
 
-bool virFileExists(const char *path G_GNUC_UNUSED)
+bool virFileExists(const char *path)
 {
     VIR_LOCK_GUARD lock = virLockGuardLock(&m);
 
     if (getenv(ENVVAR) == NULL)
-        return access(path, F_OK) == 0;
+        return real_virFileExists(path);
 
     init_hash();
     if (virHashHasEntry(chown_paths, path))


Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH V2] security: Ensure file exists before attempting to restore label
Posted by Jim Fehlig 3 weeks, 2 days ago 
On 4/8/24 6:18 AM, Michal Prívozník wrote:
> On 4/2/24 00:14, Jim Fehlig wrote:
>> When performing an install, it's common for tooling such as virt-install
>> to remove the install kernel/initrd once they are successfully booted and
>> the domain has been redefined to boot without them. After the installation
>> is complete and the domain is rebooted/shutdown, the DAC and selinux
>> security drivers attempt to restore labels on the now deleted files. It's
>> harmles wrt functionality, but results in error messages such as
>>
>> Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir>
>> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo>
>> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
>>
>> Add a check for file existence to the virSecurity*RestoreFileLabel functions,
>> and avoid relabeling if the file is no longer available. Skipping the restore
>> caused failures in qemusecuritytest, which mocks stat, chown, etc as part of
>> ensuring the security drivers properly restore labels. virFileExists is now
>> mocked in qemusecuritymock.c to return true when passed a file previously
>> seen by the mocked stat, chown, etc functions.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>   src/security/security_dac.c     |  3 +++
>>   src/security/security_selinux.c |  2 ++
>>   tests/qemusecuritymock.c        | 18 ++++++++++++++++++
>>   3 files changed, 23 insertions(+)
>>
> 
> 
> Looks better now, thanks!
> 
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 567be4bd23..4e850e219e 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -825,6 +825,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr,
>>           virStorageSourceIsLocalStorage(src))
>>           path = src->path;
>>   
>> +    if (!virFileExists(path))
>> +        return 0;
>> +
>>       /* Be aware that this function might run in a separate process.
>>        * Therefore, any driver state changes would be thrown away. */
>>   
>> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>> index b49af26e49..aaec34ff8b 100644
>> --- a/src/security/security_selinux.c
>> +++ b/src/security/security_selinux.c
>> @@ -1488,6 +1488,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr,
>>        */
>>       if (!path)
>>           return 0;
>> +    if (!virFileExists(path))
>> +        return 0;
>>   
>>       VIR_INFO("Restoring SELinux context on '%s'", path);
>>   
>> diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c
>> index 80d59536b1..bb0a85544b 100644
>> --- a/tests/qemusecuritymock.c
>> +++ b/tests/qemusecuritymock.c
>> @@ -382,6 +382,24 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
>>   }
>>   
>>   
>> +bool virFileExists(const char *path G_GNUC_UNUSED)
> 
> This argument is used, so please drop the unused annotation.
> 
>> +{
>> +    VIR_LOCK_GUARD lock = virLockGuardLock(&m);
>> +
>> +    if (getenv(ENVVAR) == NULL)
>> +        return access(path, F_OK) == 0;
> 
> I wonder whether we should call real virFileExists() here. That way, if
> a test would chain mocks they can work (though both would probably be
> mocking access()). I'll leave it up to you whether you want to squash in
> the following:

I considered calling the real virFileExists, but was being lazy since the impl 
is a one-liner :-). Agree it's better, for consistency if nothing else. I've 
squashed in your suggestion and pushed the patch.

Thanks!
Jim
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH V2] security: Ensure file exists before attempting to restore label
Posted by Jim Fehlig 3 weeks, 2 days ago 
On 4/8/24 10:48 AM, Jim Fehlig wrote:
> On 4/8/24 6:18 AM, Michal Prívozník wrote:
>> On 4/2/24 00:14, Jim Fehlig wrote:
>>> When performing an install, it's common for tooling such as virt-install
>>> to remove the install kernel/initrd once they are successfully booted and
>>> the domain has been redefined to boot without them. After the installation
>>> is complete and the domain is rebooted/shutdown, the DAC and selinux
>>> security drivers attempt to restore labels on the now deleted files. It's
>>> harmles wrt functionality, but results in error messages such as
>>>
>>> Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): 
>>> unable to stat: /var/lib/libvirt/boot/vir>
>>> Mar 08 12:40:37 virtqemud[5639]: unable to stat: 
>>> /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo>
>>> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
>>>
>>> Add a check for file existence to the virSecurity*RestoreFileLabel functions,
>>> and avoid relabeling if the file is no longer available. Skipping the restore
>>> caused failures in qemusecuritytest, which mocks stat, chown, etc as part of
>>> ensuring the security drivers properly restore labels. virFileExists is now
>>> mocked in qemusecuritymock.c to return true when passed a file previously
>>> seen by the mocked stat, chown, etc functions.
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>> ---
>>>   src/security/security_dac.c     |  3 +++
>>>   src/security/security_selinux.c |  2 ++
>>>   tests/qemusecuritymock.c        | 18 ++++++++++++++++++
>>>   3 files changed, 23 insertions(+)
>>>
>>
>>
>> Looks better now, thanks!
>>
>>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>>> index 567be4bd23..4e850e219e 100644
>>> --- a/src/security/security_dac.c
>>> +++ b/src/security/security_dac.c
>>> @@ -825,6 +825,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager 
>>> *mgr,
>>>           virStorageSourceIsLocalStorage(src))
>>>           path = src->path;
>>> +    if (!virFileExists(path))
>>> +        return 0;
>>> +
>>>       /* Be aware that this function might run in a separate process.
>>>        * Therefore, any driver state changes would be thrown away. */
>>> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>>> index b49af26e49..aaec34ff8b 100644
>>> --- a/src/security/security_selinux.c
>>> +++ b/src/security/security_selinux.c
>>> @@ -1488,6 +1488,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager 
>>> *mgr,
>>>        */
>>>       if (!path)
>>>           return 0;
>>> +    if (!virFileExists(path))
>>> +        return 0;
>>>       VIR_INFO("Restoring SELinux context on '%s'", path);
>>> diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c
>>> index 80d59536b1..bb0a85544b 100644
>>> --- a/tests/qemusecuritymock.c
>>> +++ b/tests/qemusecuritymock.c
>>> @@ -382,6 +382,24 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
>>>   }
>>> +bool virFileExists(const char *path G_GNUC_UNUSED)
>>
>> This argument is used, so please drop the unused annotation.
>>
>>> +{
>>> +    VIR_LOCK_GUARD lock = virLockGuardLock(&m);
>>> +
>>> +    if (getenv(ENVVAR) == NULL)
>>> +        return access(path, F_OK) == 0;
>>
>> I wonder whether we should call real virFileExists() here. That way, if
>> a test would chain mocks they can work (though both would probably be
>> mocking access()). I'll leave it up to you whether you want to squash in
>> the following:
> 
> I considered calling the real virFileExists, but was being lazy since the impl 
> is a one-liner :-). Agree it's better, for consistency if nothing else. I've 
> squashed in your suggestion and pushed the patch.

Weird. Calling the real virFileExists results in a segfault in some of the CI 
jobs, e.g.

https://gitlab.com/libvirt/libvirt/-/jobs/6574951832

I wasn't able to quickly reproduce it on my development setup and don't have 
time ATM to investigate further. Calling access(2) instead of the real 
virFileExists, as the original patch did, avoids the CI failures

https://gitlab.com/jfehlig/libvirt/-/pipelines/1244895325

(Note: the centos-stream-8 failure is unrelated and due to download issues.)

I'll push a build-breaker fix shortly.

Regards,
Jim

_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH V2] security: Ensure file exists before attempting to restore label
Posted by Michal Prívozník 3 weeks, 2 days ago 
On 4/8/24 23:10, Jim Fehlig wrote:
> On 4/8/24 10:48 AM, Jim Fehlig wrote:
>> On 4/8/24 6:18 AM, Michal Prívozník wrote:
>>> On 4/2/24 00:14, Jim Fehlig wrote:
>>>> When performing an install, it's common for tooling such as
>>>> virt-install
>>>> to remove the install kernel/initrd once they are successfully
>>>> booted and
>>>> the domain has been redefined to boot without them. After the
>>>> installation
>>>> is complete and the domain is rebooted/shutdown, the DAC and selinux
>>>> security drivers attempt to restore labels on the now deleted files.
>>>> It's
>>>> harmles wrt functionality, but results in error messages such as
>>>>
>>>> Mar 08 12:40:37 virtqemud[5639]: internal error: child reported
>>>> (status=125): unable to stat: /var/lib/libvirt/boot/vir>
>>>> Mar 08 12:40:37 virtqemud[5639]: unable to stat:
>>>> /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo>
>>>> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager
>>>> transaction
>>>>
>>>> Add a check for file existence to the virSecurity*RestoreFileLabel
>>>> functions,
>>>> and avoid relabeling if the file is no longer available. Skipping
>>>> the restore
>>>> caused failures in qemusecuritytest, which mocks stat, chown, etc as
>>>> part of
>>>> ensuring the security drivers properly restore labels. virFileExists
>>>> is now
>>>> mocked in qemusecuritymock.c to return true when passed a file
>>>> previously
>>>> seen by the mocked stat, chown, etc functions.
>>>>
>>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>>> ---
>>>>   src/security/security_dac.c     |  3 +++
>>>>   src/security/security_selinux.c |  2 ++
>>>>   tests/qemusecuritymock.c        | 18 ++++++++++++++++++
>>>>   3 files changed, 23 insertions(+)
>>>>
>>>
>>>
>>> Looks better now, thanks!
>>>
>>>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>>>> index 567be4bd23..4e850e219e 100644
>>>> --- a/src/security/security_dac.c
>>>> +++ b/src/security/security_dac.c
>>>> @@ -825,6 +825,9 @@
>>>> virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr,
>>>>           virStorageSourceIsLocalStorage(src))
>>>>           path = src->path;
>>>> +    if (!virFileExists(path))
>>>> +        return 0;
>>>> +
>>>>       /* Be aware that this function might run in a separate process.
>>>>        * Therefore, any driver state changes would be thrown away. */
>>>> diff --git a/src/security/security_selinux.c
>>>> b/src/security/security_selinux.c
>>>> index b49af26e49..aaec34ff8b 100644
>>>> --- a/src/security/security_selinux.c
>>>> +++ b/src/security/security_selinux.c
>>>> @@ -1488,6 +1488,8 @@
>>>> virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr,
>>>>        */
>>>>       if (!path)
>>>>           return 0;
>>>> +    if (!virFileExists(path))
>>>> +        return 0;
>>>>       VIR_INFO("Restoring SELinux context on '%s'", path);
>>>> diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c
>>>> index 80d59536b1..bb0a85544b 100644
>>>> --- a/tests/qemusecuritymock.c
>>>> +++ b/tests/qemusecuritymock.c
>>>> @@ -382,6 +382,24 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
>>>>   }
>>>> +bool virFileExists(const char *path G_GNUC_UNUSED)
>>>
>>> This argument is used, so please drop the unused annotation.
>>>
>>>> +{
>>>> +    VIR_LOCK_GUARD lock = virLockGuardLock(&m);
>>>> +
>>>> +    if (getenv(ENVVAR) == NULL)
>>>> +        return access(path, F_OK) == 0;
>>>
>>> I wonder whether we should call real virFileExists() here. That way, if
>>> a test would chain mocks they can work (though both would probably be
>>> mocking access()). I'll leave it up to you whether you want to squash in
>>> the following:
>>
>> I considered calling the real virFileExists, but was being lazy since
>> the impl is a one-liner :-). Agree it's better, for consistency if
>> nothing else. I've squashed in your suggestion and pushed the patch.
> 
> Weird. Calling the real virFileExists results in a segfault in some of
> the CI jobs, e.g.
> 
> https://gitlab.com/libvirt/libvirt/-/jobs/6574951832

Ah, what I forgot in my code is a call to init_syms(). Like this:

bool virFileExists(const char *path)
{
    VIR_LOCK_GUARD lock = virLockGuardLock(&m);

    if (getenv(ENVVAR) == NULL) {
        init_syms();

        return real_virFileExists(path);

> 
> I wasn't able to quickly reproduce it on my development setup and don't
> have time ATM to investigate further. Calling access(2) instead of the
> real virFileExists, as the original patch did, avoids the CI failures


I guess it's because on Linux some other mocked function is called first
and thus real_virFileExists is initialized properly.

I've posted a patch here:

https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/J3VKCRJQ4BLWHW4KTVAXKDPPKHMWMOUG/

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.