Re: [libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files
On Thu, Aug 16, 2018 at 23:03:40 +0300, Povilas Kanapickas wrote: > On 16/08/2018 10:38, Peter Krempa wrote: > > To fix this you should record the backing format [1] into your overlay > > image. If we'd relax the code we'd face the regression in the security > > fix we've done. > > > > [1] qemu-img creage -f qcow2 -F qcow2 -b backing-qcow2 overlay.qcow2 > > > > -F option specifies the format of the backing file > > > > Thanks a lot for your explanation, now I see that my proposal does not > make any sense. Your suggestion works fine and virt-aa-helper produces > correct output. > > Do you think this situation should ideally be diagnosed by higher-level > tools such as virt-manager which right now emit a generic permission > denied error? The current way things are implemented is that we don't even try to probe the type of the backing file if the format is not specified so the libvirt code is not sure whether the backing image is raw or not. We certainly can propagate the fact inside libvirt but we can't fail startup of the VM in such case since the image might have been raw in fact which would be safe to do and the VM will start. Reporting warnings is generally possible but they end up in the logs only which is not entirely obvious. In case of virt-manager it's not as easy. virt-manager needs to be able to operate on remote connections (thus not be able to inspect the files present locally if they are not present in a libvirt storage pool) this means that since it will not get an error from libvirt and is not able to inspect the files it's hard to implement such a warning. > Maybe virt-aa-helper could also emit a comment into the apparmor profile > saying something like "image.img has a backing image xyz.img but it was > not probed because its format is not recorded into the overlay image"? As this is using libvirt's image detection code it may be possible to add a field in the virStorageSource structure where we note that the format was assumed as raw due to failed format detection. The virt-aa-helper then could print that message in such case. From libvirt's point of view the warning could be recorded in the VM log file. The problem still is that it may work in some cases. Or even the VM may start, but the contents of the disk will be corrupted. Currently we just don't allow qemu to use any of the backing files if the format detection has failed, but that does not mean that qemu will not attempt to open it as qcow2. On the other hand with the upcomming -blockdev work, we will be able to tell qemu to open the image as raw instead so the guest will most probably get garbled content. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files
On 16/08/2018 10:38, Peter Krempa wrote: > To fix this you should record the backing format [1] into your overlay > image. If we'd relax the code we'd face the regression in the security > fix we've done. > > [1] qemu-img creage -f qcow2 -F qcow2 -b backing-qcow2 overlay.qcow2 > > -F option specifies the format of the backing file > Thanks a lot for your explanation, now I see that my proposal does not make any sense. Your suggestion works fine and virt-aa-helper produces correct output. Do you think this situation should ideally be diagnosed by higher-level tools such as virt-manager which right now emit a generic permission denied error? Maybe virt-aa-helper could also emit a comment into the apparmor profile saying something like "image.img has a backing image xyz.img but it was not probed because its format is not recorded into the overlay image"? Regards, Povilas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files
Hi,
I've looked into why apparmor profiles do not contain exceptions for
backing files of images which later leads to permission errors due to
apparmor containment. As of newest libvirt git master, only the first
level backing image is included, the subsequent images are omitted.
Below is my investigation of why this issue happens. It is based on
libvirt git master as of 2018-08-15 (e3e48d7cb82d58). The possible fix
is easy, but I don't have the background how to write tests for it, so
it's best that someone who knows the project well completes the fix
In my case I have the following file hierarchy:
- image-master.qcow2 (has backing file image-backing-1.qcow2)
- image-backing-1.qcow2 (has backing file image-backing-2.qcow2
- image-backing-2.qcow2
The apparmor profiles are created by the virt-aa-helper tool. The
get_files function (src/security/virt-aa-helper.c:949) creates a list of
files that need to be accessed by the virtual machine. The call to
virDomainDiskDefForeachPath() is responsible for creating the list of
files required to access each disk given the disk metadata.
The disk->src argument contains the source file. The expected file
hierarchy would be this:
disk->src->path == path/to/image-master.qcow2
disk->src->backingStore->path == path/to/image-backing-1.qcow2
disk->src->backingStore->backingStore->path == path/to/image-backing-2.qcow2
Unfortunately only the first two levels are present and
disk->src->backingStore->backingStore points to a dummy object.
The backing store details are filled in virStorageFileGetMetadata()
call. It calls into virStorageFileGetMetadataRecurse
(src/util/virstoragefile.c:4789) which will collect metadata for a
single image and recurse into itself for backing files.
For us, the following part of virStorageFileGetMetadataRecurse is
important (simplified for brevity):
```
virStorageFileGetMetadataInternal(src, ..., );
if (src->backingStoreRaw) {
backingStore = ...
if (backingFormat == VIR_STORAGE_FILE_AUTO)
backingStore->format = VIR_STORAGE_FILE_RAW; [1]
else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
backingStore->format = VIR_STORAGE_FILE_AUTO;
else
backingStore->format = backingFormat;
virStorageFileGetMetadataRecurse(backingStore, ...) [2]
}
```
The crux of the issue seems that the call to
virStorageFileGetMetadataInternal() for the image-master.qcow2 image
will set the `backingFormat` return value to
VIR_STORAGE_FILE_AUTO. The code responsible is in qcowXGetBackingStore
(src/util/virstoragefile.c:491) called via
`fileTypeInfo[meta->format].getBackingStore(...)` at
src/util/virstoragefile.c:1042.
It backingFormat is then downgraded to VIR_STORAGE_FILE_RAW at
src/util/virstoragefile.c:4854 ([1] above). This causes the next recurse
call to
virStorageFileGetMetadataRecurse() ([2] above) to not investigate the
backing images at all in virStorageFileGetMetadataInternal() as
fileTypeInfo[VIR_STORAGE_FILE_RAW].getBackingStore will be NULL.
The possible solution is to return VIR_STORAGE_FILE_AUTO_SAFE from
qcowXGetBackingStore. It probably does not make much sense to prevent
probing of qcow2 backing images as we probe the first level of backing
images anyway, so that return value only affect second and subsequent
levels of backing images. Looking into the implementation of
qcowXGetBackingStore it also does not seem that it performs any
operations that are unsafe by design.
Currently VIR_STORAGE_FILE_AUTO is used only in qedGetBackingStore and
it does not seem that probing of qcow images is unsafe enough
What do the developers think about this? Could someone complete the fix
with tests or advise me about the testing frameworks if there's not
enough time?
Thanks a lot!
Povilas
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files
On Wed, Aug 15, 2018 at 20:56:35 +0300, Povilas Kanapickas wrote:
> Hi,
Hi,
>
> I've looked into why apparmor profiles do not contain exceptions for
> backing files of images which later leads to permission errors due to
> apparmor containment. As of newest libvirt git master, only the first
> level backing image is included, the subsequent images are omitted.
>
> Below is my investigation of why this issue happens. It is based on
> libvirt git master as of 2018-08-15 (e3e48d7cb82d58). The possible fix
> is easy, but I don't have the background how to write tests for it, so
> it's best that someone who knows the project well completes the fix
>
> In my case I have the following file hierarchy:
> - image-master.qcow2 (has backing file image-backing-1.qcow2)
> - image-backing-1.qcow2 (has backing file image-backing-2.qcow2
> - image-backing-2.qcow2
>
> The apparmor profiles are created by the virt-aa-helper tool. The
> get_files function (src/security/virt-aa-helper.c:949) creates a list of
> files that need to be accessed by the virtual machine. The call to
> virDomainDiskDefForeachPath() is responsible for creating the list of
> files required to access each disk given the disk metadata.
>
> The disk->src argument contains the source file. The expected file
> hierarchy would be this:
>
> disk->src->path == path/to/image-master.qcow2
> disk->src->backingStore->path == path/to/image-backing-1.qcow2
> disk->src->backingStore->backingStore->path == path/to/image-backing-2.qcow2
>
> Unfortunately only the first two levels are present and
> disk->src->backingStore->backingStore points to a dummy object.
Yes, this is expected behaviour according to your analysis below.
>
> The backing store details are filled in virStorageFileGetMetadata()
> call. It calls into virStorageFileGetMetadataRecurse
> (src/util/virstoragefile.c:4789) which will collect metadata for a
> single image and recurse into itself for backing files.
>
> For us, the following part of virStorageFileGetMetadataRecurse is
> important (simplified for brevity):
>
> ```
> virStorageFileGetMetadataInternal(src, ..., );
>
> if (src->backingStoreRaw) {
> backingStore = ...
>
> if (backingFormat == VIR_STORAGE_FILE_AUTO)
> backingStore->format = VIR_STORAGE_FILE_RAW; [1]
> else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
> backingStore->format = VIR_STORAGE_FILE_AUTO;
> else
> backingStore->format = backingFormat;
>
> virStorageFileGetMetadataRecurse(backingStore, ...) [2]
> }
> ```
>
> The crux of the issue seems that the call to
> virStorageFileGetMetadataInternal() for the image-master.qcow2 image
> will set the `backingFormat` return value to VIR_STORAGE_FILE_AUTO. The
This means that the image-master.qcow2 image does NOT have the backing
format recorded in the metadata and thus we select the automatic format.
The automatic format is insecure though. A mailicious VM could write a
qcow2 header into an otherwise raw file and then the hypervisor would
happily open that file for you as a backing image.
We've swithched off automatic image probing a long time ago and actually
removed all the corresponding code some time ago.
> code responsible is in qcowXGetBackingStore
> (src/util/virstoragefile.c:491) called via
> `fileTypeInfo[meta->format].getBackingStore(...)` at
> src/util/virstoragefile.c:1042.
>
> It backingFormat is then downgraded to VIR_STORAGE_FILE_RAW at
> src/util/virstoragefile.c:4854 ([1] above). This causes the next recurse
> call to virStorageFileGetMetadataRecurse() ([2] above) to not
> investigate the backing images at all in
> virStorageFileGetMetadataInternal() as
> fileTypeInfo[VIR_STORAGE_FILE_RAW].getBackingStore will be NULL.
This is correct and desired behaviour if the user does not configure the
backing store format explicitly.
> The possible solution is to return VIR_STORAGE_FILE_AUTO_SAFE from
> qcowXGetBackingStore. It probably does not make much sense to prevent
> probing of qcow2 backing images as we probe the first level of backing
> images anyway, so that return value only affect second and subsequent
The first level is probed as the user has to declare the format in the
XML file and thus we can use that as a authorized value.
In that case we open the file as qcow2 and read the 'backing_file' and
'backing_format' headers.
The 'backing_format' header is then used as an authoritative format for
the subsequent layers.
If the image is missing that value we use AUTO and stop probing there
due to the reasons above.
> levels of backing images. Looking into the implementation of
> qcowXGetBackingStore it also does not seem that it performs any
> operations that are unsafe by design.
I don't think there's a good enough reason to relax this check and I did
not even think about the security implications.
The issue does not happen if you use images which were used somehow in
the libvirt VM lifecycle since the snapshot operation as we use
explicitly
[libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files
Hi,
I've looked into why apparmor profiles do not contain exceptions for
backing files of images which later leads to permission errors due to
apparmor containment. As of newest libvirt git master, only the first
level backing image is included, the subsequent images are omitted.
Below is my investigation of why this issue happens. It is based on
libvirt git master as of 2018-08-15 (e3e48d7cb82d58). The possible fix
is easy, but I don't have the background how to write tests for it, so
it's best that someone who knows the project well completes the fix
In my case I have the following file hierarchy:
- image-master.qcow2 (has backing file image-backing-1.qcow2)
- image-backing-1.qcow2 (has backing file image-backing-2.qcow2
- image-backing-2.qcow2
The apparmor profiles are created by the virt-aa-helper tool. The
get_files function (src/security/virt-aa-helper.c:949) creates a list of
files that need to be accessed by the virtual machine. The call to
virDomainDiskDefForeachPath() is responsible for creating the list of
files required to access each disk given the disk metadata.
The disk->src argument contains the source file. The expected file
hierarchy would be this:
disk->src->path == path/to/image-master.qcow2
disk->src->backingStore->path == path/to/image-backing-1.qcow2
disk->src->backingStore->backingStore->path == path/to/image-backing-2.qcow2
Unfortunately only the first two levels are present and
disk->src->backingStore->backingStore points to a dummy object.
The backing store details are filled in virStorageFileGetMetadata()
call. It calls into virStorageFileGetMetadataRecurse
(src/util/virstoragefile.c:4789) which will collect metadata for a
single image and recurse into itself for backing files.
For us, the following part of virStorageFileGetMetadataRecurse is
important (simplified for brevity):
```
virStorageFileGetMetadataInternal(src, ..., );
if (src->backingStoreRaw) {
backingStore = ...
if (backingFormat == VIR_STORAGE_FILE_AUTO)
backingStore->format = VIR_STORAGE_FILE_RAW; [1]
else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
backingStore->format = VIR_STORAGE_FILE_AUTO;
else
backingStore->format = backingFormat;
virStorageFileGetMetadataRecurse(backingStore, ...) [2]
}
```
The crux of the issue seems that the call to
virStorageFileGetMetadataInternal() for the image-master.qcow2 image
will set the `backingFormat` return value to VIR_STORAGE_FILE_AUTO. The
code responsible is in qcowXGetBackingStore
(src/util/virstoragefile.c:491) called via
`fileTypeInfo[meta->format].getBackingStore(...)` at
src/util/virstoragefile.c:1042.
It backingFormat is then downgraded to VIR_STORAGE_FILE_RAW at
src/util/virstoragefile.c:4854 ([1] above). This causes the next recurse
call to virStorageFileGetMetadataRecurse() ([2] above) to not
investigate the backing images at all in
virStorageFileGetMetadataInternal() as
fileTypeInfo[VIR_STORAGE_FILE_RAW].getBackingStore will be NULL.
The possible solution is to return VIR_STORAGE_FILE_AUTO_SAFE from
qcowXGetBackingStore. It probably does not make much sense to prevent
probing of qcow2 backing images as we probe the first level of backing
images anyway, so that return value only affect second and subsequent
levels of backing images. Looking into the implementation of
qcowXGetBackingStore it also does not seem that it performs any
operations that are unsafe by design.
Currently VIR_STORAGE_FILE_AUTO is used only in qedGetBackingStore and
it does not seem that probing of qcow images is unsafe enough
What do the developers think about this? Could someone complete the fix
with tests or advise me about the testing frameworks if there's not
enough time?
Thanks a lot!
Povilas
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
